Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(bigquery) Add location for Big Query job #658

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

manhld0206
Copy link
Contributor

@manhld0206 manhld0206 commented Nov 29, 2023

Summary

What changed

  • Include params for _post_json() and _delete() function of BigqueryBase class
  • Include location params for Job class
    • Add utils function _config_params to add location parameter if needed
    • Include location parameter for BigQuery API if needed

Why

@manhld0206 manhld0206 marked this pull request as ready for review November 29, 2023 04:01
@manhld0206 manhld0206 requested review from TheKevJames and a team as code owners November 29, 2023 04:01
@manhld0206 manhld0206 requested review from cphoward and aherrada-dialpad and removed request for a team November 29, 2023 04:01
@manhld0206 manhld0206 force-pushed the fix/bq_job_include_location branch 2 times, most recently from c5f7132 to 7243815 Compare December 2, 2023 10:46
@spencertollefson
Copy link

Oh nice, I totally missed your PR here @manhld0206! I made one as well, but yours looks more comprehensive than the one I made: #666.

Looks like you haven't gotten any response from reviewers yet. @TheKevJames @cphoward @aherrada-dialpad @leanaha @eddiedialpad @juanamari94

@manhld0206 manhld0206 changed the title Fix/bq job include location fix(bigquery) job include location Jan 29, 2024
@manhld0206
Copy link
Contributor Author

manhld0206 commented Jan 29, 2024

@TheKevJames May I ask for your help to run the test cases?

@manhld0206 manhld0206 changed the title fix(bigquery) job include location fix(bigquery) Add location for Big Query job Jan 29, 2024
@moiseenkov
Copy link

@overset , @kmorey , @cphoward, @aherrada-dialpad, hi everyone!
Could you please take a look at this PR and merge it if possible. Any response and releasing this feature is highly appreciated.

@TheKevJames
Copy link
Member

Sorry for the big delay on this, really appreciate the contribution(s) here. This approach looks great! I'll toss a few tests at it and get it merged asap.

@spencertollefson to answer your question on the other PR, we'd definitely like to head in the direction of supporting all available API params, if a solution could be built which is reasonable maintainable (eg. perhaps we can auto-build some code based off of their API specs, or such!). Definitely would want that to be a separate PR, as it'd likely be a pretty large change, but we'd definitely be interested in looking more into it! It's been a moonshot backlog item for us for a while, but we haven't had the bandwidth to build it in-house :)

@TheKevJames TheKevJames merged commit 4bcd503 into talkiq:master Feb 16, 2024
61 of 71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants