You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/12/07 19:15:18 UTC
[GitHub] [airflow] jon-evergreen opened a new pull request, #28202: Fix template rendering for Common SQL operators
jon-evergreen opened a new pull request, #28202:
URL: https://github.com/apache/airflow/pull/28202
Closes: #28195
This patch fixes all the common SQL operators I could find which showed
the same problem as reported in #28195, that statements are generated
"too early", before the templated variables have been applied. I think
all changes should have tests which break without the fix. Some of
these tests are quiet brittle in that they mock complex nested SQL which
is not ideal.
This patch also adds "table" as a templated field for the
`SQLTableCheckOperator` and `SQLColumnCheckOperator`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1342890855
Yeah. Cool. This is exactly why we have tests for :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
jon-evergreen commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344327662
I did install the hooks after the first commit which got bounced. And when I make changes to the `sql.py` I get this in the pre-commit logs:
```
Check and update common.sql API stubs..............................(no files to check)Skipped
```
I think the precommit definition needs to change from
```
^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.py[i]$
```
to
```
^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$
```
to have it run on changes to both `sql.py` and `sql.pyi`, as the static checks action is doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344869409
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
jon-evergreen commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1342490499
Yes, sorry, I missed those yesterday.
This code took the slightly more complex approach of moving things around such that the SQL gets generated only in the execute method. I did notice some of the operators take the approach of templating what would be `self.sql`, which allows templating basically everywhere in the SQL, and would also mean the full sql statement would appear in the "Rendered" tab in the UI, which could be nice. Though it also means you don't get the `templated_fields` highlighting that e.g. you can template the `partition_clause` argument. Though it would technically be more work for the task, I guess you could template all the things currently templated more for a documentation PoV _and_ the `sql` field which the operator generates itself; the extra compute time would be relatively small, I imagine?
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344192979
Static checks need to be fixed :( @jon-evergreen - I recommend to install pre-commit, it will fix everything for you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344821232
> ```
> ^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$
> ```
> to have it run on changes to both `sql.py` and `sql.pyi`, as the static checks action is doing.
^scripts/ci/pre_commit/pre_commit_update_common_sql_api\.py|^airflow/providers/common/sql/.*\.pyi?$
Correct 🤦 . The `[i]` is glob not regexp :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
jon-evergreen commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1342547253
Ah, the static checks for BigQuery are failing because there's no longer a `self.sql` attribute. This suggests to me maybe I should keep `self.sql` around, and have that be templated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jon-evergreen commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
jon-evergreen commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1344578092
Also, in case it wasn't clear, I have now checked in the updated `sql.pyi` file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
potiuk merged PR #28202:
URL: https://github.com/apache/airflow/pull/28202
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1341468595
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #28202: Fix template rendering for Common SQL operators
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #28202:
URL: https://github.com/apache/airflow/pull/28202#issuecomment-1341594893
Static checks I am afraid
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org