You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/04/21 01:02:36 UTC
[GitHub] [airflow] hussein-awala opened a new pull request, #30784: [WIP] Add a new parameter to SQL operators to specify conn id field
hussein-awala opened a new pull request, #30784:
URL: https://github.com/apache/airflow/pull/30784
related: #30778
curretly for the operators sub class of `BaseOperator`, we provide the field value to super as `conn_id` which is a template field, and the field is rendered in execution time and everything works fine, but in the `rendered-templates` view, we have the original field name with the jinja template before rendering.
This PR add a new field for `BaseOperator` to specify the conn field name, and use it directly from the ancestor with needing to provide it to parent class, then we can add the new conn field name to `template_fields` to render it.
Before:
After:
And they both works fine in the hook.
P.S: I just update the not deprecated operators, and this change is b/c, so if the new field is not defined, and the `conn_id` is provider to parent class, it will work as before.
--
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] github-actions[bot] commented on pull request #30784: Add a new parameter to SQL operators to specify conn id field
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30784:
URL: https://github.com/apache/airflow/pull/30784#issuecomment-1581685706
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.
--
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 #30784: Add a new parameter to SQL operators to specify conn id field
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #30784:
URL: https://github.com/apache/airflow/pull/30784
--
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] hussein-awala commented on pull request #30784: Add a new parameter to SQL operators to specify conn id field
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #30784:
URL: https://github.com/apache/airflow/pull/30784#issuecomment-1666906282
@potiuk This PR is ready and fully b/c:
- If we use an old version of sub providers with new version of common SQL -> BaseSqlOperator will not find the new attribute `conn_id_field` in the sub class and it will fallback to the default one `conn_id` which is provided as param.
- If we use a new version of sub provider with old version of common SQL -> `conn_id_field` will be ignored, and the BaseSqlOperator will use the provided param.
- If we use new versions for both -> the provided param `conn_id` will be ignored, and the BaseSqlOperator will get the attr provided in `conn_id_field`, and the issue will be fixed.
--
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 #30784: Add a new parameter to SQL operators to specify conn id field
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #30784:
URL: https://github.com/apache/airflow/pull/30784#issuecomment-1519112616
I am afraid this change is not backwards-compatible with the old common.sql providers. We have to remember that we have potentially complex relation between SQL providers and common.sql - and one can have common.sql 1.0 and latest databricks for example.
There are two ways of handling it:
1) make the change backwards compatible for each provider (it will complicate the code of each of them a bit and defeat the purpose of the change IMHO)
2) update all SQL providers to `common-sql >= 1.5.0` and set common.sql version to 1.5.0 for the next release (much better IMHO)
BTW. This is the main reason why we want to make sure common.sql provider is 100% backwards compatible - for all the providers - because the moment we make common.sql == 2.0.0, it with some breaking chnages, it will mean that if one provider will start using features from 2.0 and require it, all the other providers will also need to be upgraded which might be undesireable and we loose the "you can upgrade/downgrde providers independently" property.
--
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