Fix PostCommit YAML Xlang Direct Workflow #36874
Conversation
| /assign @damccorm |
| Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| assign set of reviewers @damccorm |
| Assigning reviewers: R: @tvalentyn 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). |
| table_name: "{BQ_TABLE}" | ||
| fields: ['label'] | ||
| row_restriction_template: "label = '37a'" | ||
| row_restriction_template: "label = '{{}}'" |
There was a problem hiding this comment.
This should work as written, so if this is failing then it is probably a real bug which is being covered up. We should address that in the transform itself, not in the test.
There was a problem hiding this comment.
To be clear - we should not be changing this test.
There was a problem hiding this comment.
Ah okay got it, I will revert it and will focus on fixing handler
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## master #36874 +/- ## ============================================ + Coverage 40.33% 40.37% +0.03% Complexity 3456 3456 ============================================ Files 1225 1226 +1 Lines 187817 188243 +426 Branches 3586 3586 ============================================ + Hits 75762 76000 +238 - Misses 108665 108853 +188 Partials 3390 3390
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Drive-by: the syntax for the review command is like so: |
| Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
aIbrahiim force-pushed the fix-postcommit-yaml-xlang-direct branch from 1d5649e to 6c0ed7e Compare | PTAL at lint and formatter errors. |
| self.fields = fields if fields else [] | ||
| self.condition_value_fn = condition_value_fn | ||
| self.query_fn = query_fn | ||
| self._has_placeholders = '{}' in self.row_restriction_template |
There was a problem hiding this comment.
I don't think we need to add a new feature to support {} placeholders
There was a problem hiding this comment.
Okay I will remove the placeholder logic here and will make the handler will try to match by key first, and if there are unmatched requests like hardcoded templates it will pair them with the same result.
aIbrahiim force-pushed the fix-postcommit-yaml-xlang-direct branch from abd73a3 to 6d2d881 Compare | elements: | ||
| - {label: '11a', rank: 0, name: 'S1'} | ||
| - {label: '37a', rank: 1, name: 'S2'} | ||
| - {label: '389a', rank: 2, name: 'S3'} |
There was a problem hiding this comment.
Why are we changing this?
There was a problem hiding this comment.
The test expectation was incorrect. Before the fix, all inputs got the same result. After the fix, each input correctly matches its table row, so we now expect all 3 enriched rows with correct ranks (0, 1, 2)
There was a problem hiding this comment.
When I looked at this the other day, I found it is a flaky test.
https://github.com/apache/beam/actions/runs/19349133623/job/55356733649
In the run above, one was successful, while the others were not.
The perm-red started around 11/13, but I did not see any related change there.
#35198 (comment)
Even if we have a fix, we have to run it multiple times to make sure it won't be flaky again.
There was a problem hiding this comment.
Also, according to the original docstring of row_restriction_template, the line row_restriction_template: "label = '37a'" in the yaml file is supposed to filter the rows. Right?
There was a problem hiding this comment.
Yes, row_restriction_template: "label = '37a'" should filter. The fix will use it as is so only matching rows get enriched and test expectation should align with this. and I'll run multiple times to verify stability.
4 participants
Fixes: #35198
Successful run: https://github.com/aIbrahiim/beam/actions/runs/19570901663
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.