Fixing JdbcOperator non-SELECT statement run

[kazanzhy]

Closes: #25388

I added a very unpleasant bug in #23817 to close #19313.
It was adding fetchall to JdbcHook in JdbcOperator.
But error raises when running not select statements as described in #25388

My main question for other maintainers is which solution is better

• adding parameter fetch_results: bool
• give users the ability to pass their own handlers

Anyway, for more compatibility and unification we have to decide the default behavior of SQL operators in terms of returning results.

My opinion is adding handler parameter and no Xcom push by default, like:
handler: Optional[Callable] = None

[kazanzhy]

Hi @cdabella @nkyuray
Probably, changes that fixed #19313 will be rolled back but with adding some flexibility

[kazanzhy]

@akashgangulyhf this PR is devoted to #25388. Please join to discussions

[josh-fell]

My main question for other maintainers is which solution is better

• adding parameter fetch_results: bool
• give users the ability to pass their own handlers

It might not be what is better, but perhaps why not do both? Have users control if they want to fetch results from the query they execute and the option to customize how to handle said results (with fetch_all_handler as a default even).

[eladkal]

Why do we need this parameter?
This is operator not hook so it looks like fetch_results = do_xcom_push ?
Did I miss something?

[kazanzhy]

No, you're right. I missed that no additional parameter is needed if do_xcom_push is already present

[uranusjr]

Suggested change

	handler: Callable = fetch_all_handler,
	handler: Callable[[CursorResult], Any] = fetch_all_handler,

(plus needed imports)

[FanatoniQ]

I dont think we should type hint here since the fetchall handler must not be sqlalchemy but bdapi2 compliant (DbApiHook.run does not use sqlalchemy).

Does the CursorResult import comes from jaydebeapi ?

[uranusjr]

It should probably be Callable[[Any], Any] then? The goal is to make sure the handler accepts exactly one argument, and returns something.

[kazanzhy]

Yes, Callable[[Any], Any] should be enough, because initially, I thought about:

from airflow.typing_compat import Protocol

class Cursor(Protocol):
    description: Optional[Tuple]
    fetchall: Callable[[], List[Tuple]]

handler: Callable[[Cursor], Optional[List[Tuple]]]

[potiuk]

Some intermittent issues. Re-running.

[FanatoniQ]

@potiuk @kazanzhy Didn't we fix #25388 with #25430 ? fetch_all_handler should be fixed now

[potiuk]

Yep. I think it's not technically needed now - the effect is the same, though the difference is that it allows to override the handler in JDBC, which might be a useful thing.

[FanatoniQ]

Yep. I think it's not technically needed now - the effect is the same, though the difference is that it allows to override the handler in JDBC, which might be a useful thing.

Ok I agree.

Quick question: shouldn't this handler parameter, and sql operator run methods return value feature used alongside do_xcom_push be coded in a common parent SQL class ? I found the other day that MsSqlOperator and MySqlOperator run methods do not return the result...

Is that planned ? I am asking after seeing this commit's message: "move all old sql operator...": acab8f5

[potiuk]

Is that planned ? I am asking after seeing this commit's message: "move all old sql operator...": acab8f5

Maybe - if you want to add such refactor - feel free :)

[kazanzhy]

@FanatoniQ I'm already working on #25259.
It might answer your question :)