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 2021/09/02 21:04:14 UTC
[GitHub] [airflow] tnyz opened a new pull request #17998: Fix error when create external table
tnyz opened a new pull request #17998:
URL: https://github.com/apache/airflow/pull/17998
destination_project_dataset_table is being used in l1201 but cannot pass init check at 1154
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] tnyz commented on a change in pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on a change in pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#discussion_r704607772
##########
File path: airflow/providers/google/cloud/operators/bigquery.py
##########
@@ -1119,7 +1119,6 @@ def __init__(
# BQ config
kwargs_passed = any(
[
- destination_project_dataset_table,
Review comment:
good call
--
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 #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-912055376
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 edited a comment on pull request #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-913113019
Please read the docs. The new "table_resource" when provided, replaces the usage of many parameters and they are ignored if specified. Whenever you use "table_resource" all provided parameters should be passed through "table_resource" rather than through individual parameters. In this particular case the right way of passing destination_dataset_table is via "table_resource"'s "tableReference" object (https://cloud.google.com/bigquery/docs/reference/rest/v2/TableReference) - where you have to specify project_id, datasetId, table_id (previously this was all passed by single "destination_project_dataset_table" string which was
"(project.)dataset.table'".
If you pass both "table_resource" and "destination_project_dataset_table" and "table_resource", the one passed by "destination_project_dataset_table" will be ignored by the new version of bigquery library - that's why when you use "table_resource" and "destination_project_dataset_table" and "table_resource" together - you get the error.
I agree that this is in super clear from the error mesage, I found it out myself couple of days ago when I corrected some warning messages, I think about improving this a bit because it also jumped for me as rather cryptic communication.
--
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 a change in pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#discussion_r706898814
##########
File path: airflow/providers/google/cloud/operators/bigquery.py
##########
@@ -1195,36 +1200,24 @@ def execute(self, context) -> None:
else:
schema_fields = self.schema_fields
- if schema_fields and self.table_resource:
- self.table_resource["externalDataConfiguration"]["schema"] = schema_fields
Review comment:
Agree.
--
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] bmbejcek commented on pull request #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
bmbejcek commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-913854290
I am also currently facing this issue. If we don't have a `destination_project_dataset_table` variable, it says that it is missing even when passing in this information through the table_resource schema you mentioned @potiuk. If we include both versions, then it says we can't have duplicate information. Seems like a catch-22.
--
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 a change in pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#discussion_r704190601
##########
File path: airflow/providers/google/cloud/operators/bigquery.py
##########
@@ -1119,7 +1119,6 @@ def __init__(
# BQ config
kwargs_passed = any(
[
- destination_project_dataset_table,
Review comment:
I believe this should be restored now
--
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] tnyz commented on pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-914653474
I have not closely examine other operators but they might suffer from the same issue too
--
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 #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-917923808
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] tnyz commented on a change in pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on a change in pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#discussion_r706640208
##########
File path: airflow/providers/google/cloud/operators/bigquery.py
##########
@@ -1195,36 +1200,24 @@ def execute(self, context) -> None:
else:
schema_fields = self.schema_fields
- if schema_fields and self.table_resource:
- self.table_resource["externalDataConfiguration"]["schema"] = schema_fields
Review comment:
Considering it was incorrectly introduced and table creation via table resource is not properly supported, I don't think this will break backwards compatibility
--
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] tnyz commented on pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-914596659
@potiuk to be more specific, this is the block when executing the command from table resource, using None will result in `TableReference.from_string(None)` right?
```
if self.table_resource:
tab_ref = TableReference.from_string(self.destination_project_dataset_table)
bq_hook.create_empty_table(
table_resource=self.table_resource,
project_id=tab_ref.project,
table_id=tab_ref.table_id,
dataset_id=tab_ref.dataset_id,
)
```
--
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] uranusjr merged pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #17998:
URL: https://github.com/apache/airflow/pull/17998
--
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] tnyz commented on pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-914653135
@potiuk I agree, please review the updated pr
--
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] tnyz commented on pull request #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
tnyz commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-913051061
@potiuk thanks for the feedback, however I am still seeing this issue as line 1157 only supports initializing self.destimation_dataset_table from parameter not from table resource. Do you mind elaborate more and check if the configuration is unreachable from table resource, thanks
--
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 #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-914642118
Ah I see now. I have not looked that far.I am not sure though if that is the **right** fix, I think - looking at the code deeper, is that this is really a mixture of old and new way of passing table resource parameters - there is ambiguity here - looks like the idea was to put everything in table resource and here we have a weird expactation that part of it will be passed as `destination_project_dataset_table`.
So IMHO the right approach is to change the lines you marked to:
```
if self.table_resource:
bq_hook.create_empty_table(
table_resource=self.table_resource,
project_id=None,
table_id=None,
dataset_id=None,
)
```
--
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 #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-917705721
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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 #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-914586819
There were some `default` values in the past google provider that was fixed in 5.1.0. The default values were set for few parameters (where they should not be if "table_resource" was specified).
You can set those parameters to None (for example `destination_project_dataset_table`) when you use table_resource if you use earlier version of the provider.
The new version of the provider is free of that problem I 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 #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-913113019
Please read the docs. The new "table_resource" when provided, replaces the usage of many parameters and they are ignored if specified. Whenever you use "table_resource" all provided parameters should be passed through "table_resource" rather than through individual parameters. In this particular case the right way of passing destination_dataset_table is via "table_resource"'s "tableReference" object (https://cloud.google.com/bigquery/docs/reference/rest/v2/TableReference) - where you have to specify project_id, datasetId, table_id (previously this was all passed by single "destination_project_dataset_table" string which was
"(<project>.)<dataset>.<table>'".
If you pass both "table_resource" and "destination_project_dataset_table" and "table_resource", the one passed by "destination_project_dataset_table" will be ignored by the new version of bigquery library - that's why when you use "table_resource" and "destination_project_dataset_table" and "table_resource" together - you get the error.
I agree that this is in super clear from the error mesage, I found it out myself couple of days ago when I corrected some warning messages, I think about improving this a bit because it also jumped for me as rather cryptic communication.
--
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 closed pull request #17998: Fix error when create external table
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #17998:
URL: https://github.com/apache/airflow/pull/17998
--
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] turbaszek commented on a change in pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#discussion_r706638081
##########
File path: airflow/providers/google/cloud/operators/bigquery.py
##########
@@ -1195,36 +1200,24 @@ def execute(self, context) -> None:
else:
schema_fields = self.schema_fields
- if schema_fields and self.table_resource:
- self.table_resource["externalDataConfiguration"]["schema"] = schema_fields
Review comment:
Does removing this logic break backward compatibility?
--
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] tnyz commented on pull request #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
tnyz commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-917772057
> Static checks are failing
test has been 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 #17998: Fix error when create external table using table resource
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17998:
URL: https://github.com/apache/airflow/pull/17998#issuecomment-917709060
Static checks are failing
--
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