-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Fix generate sequence #35043
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 generate sequence #35043
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #35043 +/- ##
============================================
- Coverage 56.47% 56.47% -0.01%
+ Complexity 3300 3298 -2
============================================
Files 1184 1184
Lines 181773 181778 +5
Branches 3409 3409
============================================
- Hits 102655 102653 -2
- Misses 75850 75855 +5
- Partials 3268 3270 +2
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:
|
|
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). |
| "Specifies the rate to generate a given number of elements per a given number of seconds. " | ||
| + "Applicable only to unbounded sequences.") | ||
| @Nullable | ||
| public abstract Rate getRate(); |
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.
This Rate object already has the fields necessary (elements and seconds) to control elements per period. Can you check if this works?
If it doesn't work, I think we can just remove Rate and introduce them as top-level fields
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.
i.e. something like provider.GenerateSequence(start=0, rate={elements: 3, seconds: 1})
I don't remember if "rate" is provided as a dict or a beam.Row()
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.
Tested. The dict works well. I also added the test test_run_generate_sequence_with_rate.
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.
If the current Rate dict works then is there a need for this PR?
I don't think it makes sense to duplicate the API. Maybe we should keep the new rate test though
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.
The original idea is to create the similar syntax to
| public GenerateSequence buildExternal(External.ExternalConfiguration config) { |
rate, I am also fine with that.
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.
Fixes #35034
Test
Output:
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.