Skip to content

[BEAM-10647] Fixes get_query_location bug in BigQueryWrapper - #12480

Merged
apilloud merged 1 commit into
apache:masterfrom
FlyrInc:feature/BEAM-10647
Aug 7, 2020
Merged

[BEAM-10647] Fixes get_query_location bug in BigQueryWrapper#12480
apilloud merged 1 commit into
apache:masterfrom
FlyrInc:feature/BEAM-10647

Conversation

@galuszkak

@galuszkakgaluszkak commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

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) 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.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

LangSDKDataflowFlinkSamzaSparkTwister2
GoBuild Status---Build Status---Build Status---
JavaBuild StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build StatusBuild Status
Build Status
Build Status
Build Status
PythonBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
---Build Status---
XLangBuild Status---Build Status---Build Status---

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsite
Non-portableBuild StatusBuild Status
Build Status
Build Status
Build StatusBuild Status
Portable---Build Status------

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels

See CI.md for more information about GitHub Actions CI.

@galuszkak
galuszkakforce-pushed the feature/BEAM-10647 branch 2 times, most recently from 5fca0c3 to 1f8f392CompareAugust 6, 2020 11:07
@galuszkak

Copy link
Copy Markdown
ContributorAuthor

R: @robertwb@apilloud

@apilloud
apilloud requested a review from pabloemAugust 6, 2020 16:55

@apilloudapilloud left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'm not familiar with Python style, particularly around optional imports, so I'll defer to @pabloem on this.

I believe we recently received a report of this problem on the Java SDK as well. It might be nice to have a integration test, as it is possible this behavior changed on BigQuery. Thanks for taking the time to track it down!

cc: @chamikaramj

Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools.py Outdated
Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools.py Outdated
Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools_test.py Outdated

@galuszkakgaluszkak left a comment

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apilloud I will address that one comment. Let me know if other things make sense.

Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools.py Outdated
Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools.py Outdated
Comment threadsdks/python/apache_beam/io/gcp/bigquery_tools_test.py Outdated
@apilloud

Copy link
Copy Markdown
Member

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!

@galuszkak

Copy link
Copy Markdown
ContributorAuthor

@apilloud I think Jenkins is fixed.

Thanks!

@galuszkak

Copy link
Copy Markdown
ContributorAuthor

I've looked on Linter error but it looks that is unrelated to my changes, and error was introduced in #12476 by @robertwb

@apilloud

Copy link
Copy Markdown
Member

Thanks. The other failure is BEAM-8912.

@apilloud
apilloud merged commit 82a7f62 into apache:masterAug 7, 2020
@pabloem

Copy link
Copy Markdown
Member

thanks - this looks great fwiw : )

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@galuszkak@apilloud@pabloem