-
Notifications
You must be signed in to change notification settings - Fork 40
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
Render sql code with parameters in BaseSQLDecoratedOperator #897
Conversation
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.
@feluelle I think it could be confusing to end-users to have three methods for creating a table from a SQL statement:
transform
transform_file
transform_sql
I can see the need for having a decorator (transform
) x a task, and I understand you didn't want to introduce any breaking changes, but it is confusing overall. What do you think of:
1, Adding a deprecation warning in transform_file
2. Name the new task transform_task
(or something else) - so it is clear the difference in comparison to transform
- and it could be used both to load from a file and a string?
3. Should we have some tests?
Yes, actually this makes more sense. |
c27ebd3
to
bef3b1d
Compare
91a2bdd
to
8bdec73
Compare
Codecov ReportBase: 92.57% // Head: 93.50% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #897 +/- ##
==========================================
+ Coverage 92.57% 93.50% +0.93%
==========================================
Files 91 91
Lines 4810 4837 +27
Branches 476 480 +4
==========================================
+ Hits 4453 4523 +70
+ Misses 260 225 -35
+ Partials 97 89 -8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
fc85f21
to
cc19916
Compare
cc19916
to
c58ba13
Compare
047f0c4
to
9f615d9
Compare
- add sql and parameters to BaseSQLDecoratedOperator template_fields - add .sql to BaseSQLDecoratedOperator template_ext - add check for template_fields and template_ext
8b70599
to
5646f46
Compare
for more information, see https://pre-commit.ci
I've worked on this PR independently and with @feluelle, and the tests are now passing. I also validated that both Example of Since it was long approved, I'm merging this PR as soon as the CI checks pass - before it completes its five months anniversary. We can continuously improve and iterate over it with time. |
Co-authored-by: Felix Uellendall <feluelle@users.noreply.github.com>
This PR has been long standing and was approved a while ago, we can iterate and improve the implementation over time.
) Allow users to view the SQL statements, regardless of whether they are part of the DAG definition or declared on a separate file, within the Airflow UI task "Rendered Template" field. This change affects `aql.transform`, `aql.transform_file` and `aql.dataframe`, since it is implemented on the `BaseSQLDecoratedOperator`. The following changes were made: - add sql and parameters to BaseSQLDecoratedOperator template_fields - add .sql to BaseSQLDecoratedOperator template_ext - add check for template_fields and template_ext closes: #1003 depends on: #1004 Co-authored-by: Tatiana Al-Chueyr <tatiana.alchueyr@gmail.com>
Description
What is the current behavior?
Currently, when using operators inheriting from
BaseSQLDecoratedOperator
we are unable to view the rendered SQL in the UI.closes: #1003
depends on: #1004
What is the new behavior?
Does this introduce a breaking change?
No.
Checklist