-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Default split_statements to True for Presto and Trino Hook #47186
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
base: main
Are you sure you want to change the base?
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
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.
Looks good! Would you mind adding a test for this change? Thanks!
@jason810496 Thank you for the review! Added test to check default split_statements in run method. Not sure if any other tests are needed? |
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.
Thanks for the changes! We should include a test case per overload. Because there are major differences for example one is taking Callable while the other one is None for handler
. Something like initialising the hook with these and testing the run
method. I think we should follow this for both hooks.
Thank you @bugraoz93 for the direction! This makes total sense! I'm new to contributing to Airflow -- is there any provider example I can follow to add tests? |
The tests depend on the case and in this case, I believe it should be under If you would like specific examples, please take a look at them here. I believe there are examples where airflow/providers/google/src/airflow/providers/google/cloud/hooks/gcs.py Lines 283 to 409 in 201baf9
Tests airflow/providers/google/tests/unit/google/cloud/hooks/test_gcs.py Lines 840 to 913 in 4651ccc
|
Hi, this is my first "contribution", even if it is just a suggestion. I'm clearly new to this, but I was wondering if some users has some statement like
or I'm not sure if it's a good solution, but providing I'm still looking for the split_statement's definition, but I think it is splitting str with semicolon and iterables by their elements, so it might be an intuitive way to code : you use an iterable if you want it to be split, otherwise, you use a single string (for a single statement) ["DESCRIBE ... ;","SELECT ...;","BEGIN; ... ; COMMIT;"] --> split_statement = True I'm not a native English speaker, so tell me if i'm not easy to understand, and i'll try to explain my point of view. Good job anyway, you pointed out something interesting. Thanks to this issue, I learnt some new things like @overload, which I had already encountered in Java but never explored. Java wasn't in my career path, but seeing it in Python made me look for explanations. I'm still learning, and this was a great opportunity to expand my beginner knowledge. See you ! |
Hi, @willshen99 it seems like this PR has been inactive for a while. Could I help with this? |
Feel free to create a new one :) |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
@willshen99 if you can rebase and resolve conflicts I think we can merge this PR after tests passes |
@eladkal Thanks for the review! I have resolved conflicts. |
Tests are failing |
Fixed the missing import. Sorry about that. |
I think we should have a test in Trino and Presto to make sure that when you can change |
@eladkal Thanks for the suggestion. This makes total sense! I'm new to airflow project, and I need some help on adding test cases. I don't think the test case I added work as expected. |
You need to enable pre-commits so it will automaticly fix static checks probelms like wrong import order. Check the contribution guide, it explain how to enable it |
Some of the tests arw also failing:
|
closes #47167