[YAML] add time unit suffix to windowing #30035
Conversation
Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>
|
R: @robertwb |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
| return pcoll | self._window_transform | ||
|
|
||
| @staticmethod | ||
| def _parse_time_unit(value, name): |
There was a problem hiding this comment.
While for a lot of the yaml construction, integration tests are more natural than unit tests, this function is of the right form to add some good unit tests too.
There was a problem hiding this comment.
Added some tests - required moving the classes around so they are visible outside the file.
Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>
robertwb
left a comment
robertwb
left a comment
There was a problem hiding this comment.
Just missing some unit tests.
| "Valid time units are {}.").format( | ||
| name, | ||
| suffix, | ||
| ', '.join("'{}'".format(k) for k in time_units.keys()))) |
There was a problem hiding this comment.
You could probably just do ', '.join(time_units.keys()).
There was a problem hiding this comment.
I thought this looked prettier...
Invalid windowing size time unit 't'. Valid time units are 'ms', 's', 'm', 'h', 'd'.
vs.
Invalid windowing size time unit 't'. Valid time units are ms, s, m, h, d
Polber
force-pushed
the
jkinard/windowing-time-units
branch
4 times, most recently
from
409eaf0 to
68f5768
Compare
Polber
force-pushed
the
jkinard/windowing-time-units
branch
from
68f5768 to
14957de
Compare
| "Valid time units are {}.").format( | ||
| name, | ||
| suffix, | ||
| ', '.join("'{}'".format(k) for k in time_units.keys()))) |
|
Looks like test_expand_composite_transform_with_name_input is too strict. (What was that test trying to verify?) |
Polber
force-pushed
the
jkinard/windowing-time-units
branch
from
14957de to
efac0b5
Compare
I'm not exactly sure what it is trying to test exactly. Looks like it is the same test as the ones around it except has a defined input, but the input isn't what is checked in the test (the output is). I think many of those tests need to be rewritten |
Polber
force-pushed
the
jkinard/windowing-time-units
branch
from
efac0b5 to
f8dc3c4
Compare
Signed-off-by: Jeffrey Kinard <jeff@thekinards.com>
Polber
force-pushed
the
jkinard/windowing-time-units
branch
from
f8dc3c4 to
b6c207d
Compare
2 participants
Adds time unit suffixes (ms, s, m, h, d) to windowing syntax.
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.