-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixing the behaviours of SQL Hooks and Operators finally #27912
Conversation
cc: @alexott |
c60d7f4
to
9f43079
Compare
cc: @alexott @kazanzhy - I have two important doubts about processing of
I followed original implementations I think and I left I think intact original behviour of stripping in both implementationations. Look at the parameterized tests - and the "DBApiHook" behaves now differently than Databricks Hook in this regard - the Databricks hook always splits the Where the standard DBApiHook does not:
https://peps.python.org/pep-0249/#Connection.close
I am no sure what was the reasoning behind those differences - but from what I see those differences now between those two "run" methods. Question is - whether we have good reasons to behave differently for those cases? |
9f43079
to
033ea0e
Compare
033ea0e
to
71c4e13
Compare
25e215b
to
6f39bb6
Compare
OK. There was some permission issue - but I think this one shoudl be ready to go. I added some descriptions and explanations about the behaviours and 🤞 it will pass all the docs and tests |
Let me do a checkout in my environment and test. |
8bf2f52
to
8068404
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much work on that PR! Nothing to say on my side, very solid! Thanks for adding all these tests (on top of the other changes), it adds a lot of robustness!
Indeed. I added already few small changes/clarifications after the initial push and adding all tests and it felt soooooooo reassuring to see green checkmark for all tests after my changes. |
BTW. I REALLY ❤️ Pytest style of tests vs. the "classic" unit.TestCase. Partucularrly fixtures - when you get a grasp of them, parameterization and the integration of those running tests within IDEs are just great. |
All looks good :) |
This is the more bold fix to common.sql and related providers. It implements more comprehensive tests and describes the intended behaviours in much more explicit way, also it simplifies the notion of "scalar" behaviour by introducing a new method to check on when scalar return is expected. Comprehensive suite of tests have been added (the original tests were converted to Pytest test and parameterized was used to test all the combinations of parameters and returned values.
8068404
to
7bae5c4
Compare
It's holiday in us and I'm traveling, won't be able to review until next week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you again!
@@ -32,7 +32,9 @@ This release fixes a few errors that were introduced in common.sql operator whil | |||
* ``_process_output`` method in ``SQLExecuteQueryOperator`` has now consistent semantics and typing, it | |||
can also modify the returned (and stored in XCom) values in the operators that derive from the | |||
``SQLExecuteQueryOperator``). | |||
* last description of the cursor whether to return scalar values are now stored in DBApiHook | |||
* descriptions of all returned results are stored as descriptions property in the DBApiHook |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(could be done later) maybe it would be a bit easier to read if descriptions
and last_description
are formatted as code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. We can update it later :)
@@ -190,11 +189,17 @@ class SQLExecuteQueryOperator(BaseSQLOperator): | |||
Executes SQL code in a specific database | |||
:param sql: the SQL code or string pointing to a template file to be executed (templated). | |||
File must have a '.sql' extensions. | |||
|
|||
When implementing a specific Operator, you can also implement `_process_output` method in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use double backquotes in docstrings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely :). I think it will work anyway in docstring (we'll see in the API docs) but yeah - we can fix it as follow-up
BTW. Release of rc3 is coming . |
Interesting note: the 3.3.0 DatabricksSQLOperator had a parameter for control writing to xcom. It appears to be removed in 4.0.0. This is causing the dump of a lot of PII into XCOM for my use case. |
@BillCM |
The change apache#27912 fixed and unified behaviour of DBApiHooks across the board, but it missed two places where sql was mis-used and overridden in exasol and snowflake hooks. The check for "sql" type did not use the original sql parameter value but the one that was overridden later in the run method implementation. The fix is the same as applied in Databricks Hook and DBAPI generic run methods - using consistent typing and separate variable to convert the sql string into sql list. Related: astronomer/astro-sdk#1324
…27997) The change #27912 fixed and unified behaviour of DBApiHooks across the board, but it missed two places where sql was mis-used and overridden in exasol and snowflake hooks. The check for "sql" type did not use the original sql parameter value but the one that was overridden later in the run method implementation. The fix is the same as applied in Databricks Hook and DBAPI generic run methods - using consistent typing and separate variable to convert the sql string into sql list. Related: astronomer/astro-sdk#1324
…pache#27997) The change apache#27912 fixed and unified behaviour of DBApiHooks across the board, but it missed two places where sql was mis-used and overridden in exasol and snowflake hooks. The check for "sql" type did not use the original sql parameter value but the one that was overridden later in the run method implementation. The fix is the same as applied in Databricks Hook and DBAPI generic run methods - using consistent typing and separate variable to convert the sql string into sql list. Related: astronomer/astro-sdk#1324
This is the more bold fix to common.sql and related providers. It implements more comprehensive tests and describes the intended behaviours in much more explicit way, also it simplifies the notion of "scalar" behaviour by introducing a new method to check on when scalar return is expected.
Comprehensive suite of tests have been added (the original tests were converted to Pytest test and parameterized was used to test all the combinations of parameters and returned values.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.