-
Notifications
You must be signed in to change notification settings - Fork 4.4k
[Python] Fix WriteToBigQuery transform using CopyJob does not work with WRITE_TRUNCATE write disposition (#34247) #35558
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
Conversation
|
retest this please |
5d8b286 to
47cb10b
Compare
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
I opened this to master instead, here is why: |
|
When u're done with the review, I'll proceed to update the |
|
Hi, |
|
Hi @liferoad, do you see a chance this getting into the current week's release, please? |
|
@ahmedabu98 can you review the PR? @portikCoder thanks for fixing this. Let the team review the PR first. It looks good to me. |
ahmedabu98
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit, but this looks great! Thanks for the thorough testing. LGTM
c55ca25 to
8c35592
Compare
…th WRITE_TRUNCATE write disposition (apache#34247) * It only truncates the first table, but originally didn't take care of identical table-ids but from different dataset-id, or project-id.
8c35592 to
0fc3c80
Compare
Thanks! Updated the |
|
I can merge when tests go green. FYI merging permissions are for committers, see https://beam.apache.org/contribute/become-a-committer/ |
I was thinking first to use only datasetId+tableId, but i changed my mind, since it makes better sense to use the full reference instead. All the other details can be found in the mentioned bug filing #34247.
Tests
I added integration tests.
Alongside it, I modified the package and used local executor to test the endresult manually (devtest), and worker fine. Didn't test it on master tho, just on the release branch.
NOTE
Originally I aimed for the 2.63.0 branch so it gets fixed at least there and OFC in the next release candidate. If u tell me don't bother this release I'm also fine, but I need to have a working solution ASAP, I don't really care if it's the new version (if it comes alive soon-ish) or the currently latest stable.
I had to figure out the hard way, since nobody was telling me it's not the way you release hotfixes. Anyways, if it'd be still possible to release it as a hotfix, I'd appreciate.
Also, is good to mention probably that I've checked couple releases back and this code didn't change, so it existed for a long time now, exactly since its creation time (13.02.2020 #10668).
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.