You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/08/06 10:50:16 UTC
[GitHub] [beam] galuszkak opened a new pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
galuszkak opened a new pull request #12480:
URL: https://github.com/apache/beam/pull/12480
This enables to look for location in BigQueryWrapper for first available referenced table for the user. If you are querying authorised view, you may end up with permission error of accessing referenced table that is in authorised view and be unable to retrieve location, so you should look on next available table for You that you should have permission.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-670206489
Jenkins is having issues right now (see https://issues.apache.org/jira/browse/INFRA-20649) but will merge this when it is fixed. Please remind me if I forget!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] galuszkak commented on a change in pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
galuszkak commented on a change in pull request #12480:
URL: https://github.com/apache/beam/pull/12480#discussion_r466696088
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -318,15 +319,20 @@ def get_query_location(self, project_id, query, use_legacy_sql):
referenced_tables = response.statistics.query.referencedTables
if referenced_tables: # Guards against both non-empty and non-None
Review comment:
@apilloud comment suggest that is also guarding against None value. If that's true, dropping this if, would break code `for loop` in case referenced_tables is actually None.
Therefore my question is, are you sure we want to remove that if?
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -318,15 +319,20 @@ def get_query_location(self, project_id, query, use_legacy_sql):
referenced_tables = response.statistics.query.referencedTables
if referenced_tables: # Guards against both non-empty and non-None
- table = referenced_tables[0]
- location = self.get_table_location(
- table.projectId, table.datasetId, table.tableId)
- _LOGGER.info(
- "Using location %r from table %r referenced by query %s",
- location,
- table,
- query)
- return location
+ for table in referenced_tables:
+ try:
+ location = self.get_table_location(
+ table.projectId, table.datasetId, table.tableId)
+ except HttpForbiddenError:
+ # Permission access for table (i.e. from authorized_view),
+ # try next one
+ continue
+ _LOGGER.info(
+ "Using location %r from table %r referenced by query %s",
+ location,
+ table,
+ query)
+ return location
_LOGGER.debug("Query %s does not reference any tables.", query)
Review comment:
@apilloud will change it. Thanks for pointing this out.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools_test.py
##########
@@ -50,6 +49,15 @@
from apache_beam.io.gcp.internal.clients import bigquery
from apache_beam.options.pipeline_options import PipelineOptions
+# Protect against environments where bigquery library is not available.
Review comment:
@apilloud this is copy from bigquery_test.py I assumed we want this consistent between files.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud merged pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud merged pull request #12480:
URL: https://github.com/apache/beam/pull/12480
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on a change in pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #12480:
URL: https://github.com/apache/beam/pull/12480#discussion_r466563727
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -318,15 +319,20 @@ def get_query_location(self, project_id, query, use_legacy_sql):
referenced_tables = response.statistics.query.referencedTables
if referenced_tables: # Guards against both non-empty and non-None
- table = referenced_tables[0]
- location = self.get_table_location(
- table.projectId, table.datasetId, table.tableId)
- _LOGGER.info(
- "Using location %r from table %r referenced by query %s",
- location,
- table,
- query)
- return location
+ for table in referenced_tables:
+ try:
+ location = self.get_table_location(
+ table.projectId, table.datasetId, table.tableId)
+ except HttpForbiddenError:
+ # Permission access for table (i.e. from authorized_view),
+ # try next one
+ continue
+ _LOGGER.info(
+ "Using location %r from table %r referenced by query %s",
+ location,
+ table,
+ query)
+ return location
_LOGGER.debug("Query %s does not reference any tables.", query)
Review comment:
This error message is slightly wrong now. How about: `Query %s does not reference any tables you have permission to inspect.`
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -318,15 +319,20 @@ def get_query_location(self, project_id, query, use_legacy_sql):
referenced_tables = response.statistics.query.referencedTables
if referenced_tables: # Guards against both non-empty and non-None
Review comment:
Is implied by the `for` loop and can be dropped.
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools_test.py
##########
@@ -50,6 +49,15 @@
from apache_beam.io.gcp.internal.clients import bigquery
from apache_beam.options.pipeline_options import PipelineOptions
+# Protect against environments where bigquery library is not available.
Review comment:
(@pabloem is this something we want to do?)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] galuszkak commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
galuszkak commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-670557878
I've looked on Linter error but it looks that is unrelated to my changes, and error was introduced in https://github.com/apache/beam/pull/12476 by @robertwb
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on a change in pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #12480:
URL: https://github.com/apache/beam/pull/12480#discussion_r466701216
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools_test.py
##########
@@ -50,6 +49,15 @@
from apache_beam.io.gcp.internal.clients import bigquery
from apache_beam.options.pipeline_options import PipelineOptions
+# Protect against environments where bigquery library is not available.
Review comment:
Thanks for pointing that out. (I assumed we wouldn't want to do this in tests, but if we are doing it in others then follow that pattern.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-670582949
Thanks. The other failure is BEAM-8912.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] galuszkak commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
galuszkak commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-669865786
R: @robertwb @apilloud
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] galuszkak commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
galuszkak commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-670522143
@apilloud I think Jenkins is fixed.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] apilloud commented on a change in pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #12480:
URL: https://github.com/apache/beam/pull/12480#discussion_r466699181
##########
File path: sdks/python/apache_beam/io/gcp/bigquery_tools.py
##########
@@ -318,15 +319,20 @@ def get_query_location(self, project_id, query, use_legacy_sql):
referenced_tables = response.statistics.query.referencedTables
if referenced_tables: # Guards against both non-empty and non-None
Review comment:
You are right, this needs to stay. (I'm getting go and python mixed up here, I don't use either of them very often.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] pabloem commented on pull request #12480: [BEAM-10647] Fixes get_query_location bug in BigQueryWrapper
Posted by GitBox <gi...@apache.org>.
pabloem commented on pull request #12480:
URL: https://github.com/apache/beam/pull/12480#issuecomment-670763547
thanks - this looks great fwiw : )
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org