You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "Polber (via GitHub)" <gi...@apache.org> on 2024/01/18 17:06:32 UTC
[PR] [YAML] add time unit suffix to windowing [beam]
Polber opened a new pull request, #30035:
URL: https://github.com/apache/beam/pull/30035
Adds time unit suffixes (s, m, h, d) to windowing syntax.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `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, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on PR #30035:
URL: https://github.com/apache/beam/pull/30035#issuecomment-1899671241
> Looks like test_expand_composite_transform_with_name_input is too strict. (What was that test trying to verify?)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1458173836
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
Review Comment:
OK, that's fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457955487
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +675,27 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
Review Comment:
Added some tests - required moving the classes around so they are visible outside the file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457959382
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
Review 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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on PR #30035:
URL: https://github.com/apache/beam/pull/30035#issuecomment-1898880561
R: @robertwb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457956877
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
+ print(f'name: {name}\nvalue: {value}\nsuffix: {suffix}\n')
Review Comment:
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457887464
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +675,27 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {'s': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12}
+ value, suffix = str(value)[:-1], str(value)[-1]
+ # Default to seconds if time unit suffix is not defined
+ if suffix.isnumeric():
+ value, suffix = value + suffix, 's'
+ elif not value or not value.isnumeric():
Review Comment:
Good idea
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -651,6 +651,16 @@ class WindowInto(beam.PTransform):
See [the Beam documentation on windowing](https://beam.apache.org/documentation/programming-guide/#windowing)
for more details.
+ Sizes, offsets, periods and gaps (where applicable) must be defined using
+ a time unit suffix 's', 'm', 'h' or 'd' for seconds, minutes, hours
+ or days, respectively.
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "Polber (via GitHub)" <gi...@apache.org>.
Polber commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457955975
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -48,7 +48,8 @@
from apache_beam.portability.api import schema_pb2
from apache_beam.transforms import external
from apache_beam.transforms import window
-from apache_beam.transforms.fully_qualified_named_transform import FullyQualifiedNamedTransform
+from apache_beam.transforms.fully_qualified_named_transform import \
+ FullyQualifiedNamedTransform
Review Comment:
The IDE keeps auto-formatting that. I'll figure out how to disable that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30035:
URL: https://github.com/apache/beam/pull/30035#issuecomment-1898882666
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457914435
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
+ print(f'name: {name}\nvalue: {value}\nsuffix: {suffix}\n')
+ return float(value) * float(time_units[suffix])
Review Comment:
No need for the redundant float casting on `time_units[suffix]`.
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
+ print(f'name: {name}\nvalue: {value}\nsuffix: {suffix}\n')
Review Comment:
Debugging?
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +677,30 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {
+ 'ms': 0.001, 's': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12
+ }
+ value, suffix = re.match(r'^(.*?)([^\d]*)$', str(value)).groups()
+ # Default to seconds if time unit suffix is not defined
+ if not suffix:
+ suffix = 's'
+ if not value:
+ raise ValueError(
+ f"Invalid windowing {name} value "
+ f"'{suffix if not value else value}'. "
+ f"Must provide numeric value.")
+ if suffix not in time_units:
+ raise ValueError((
+ "Invalid windowing {} time unit '{}'. " +
+ "Valid time units are {}.").format(
+ name,
+ suffix,
+ ', '.join("'{}'".format(k) for k in time_units.keys())))
Review Comment:
You could probably just do `', '.join(time_units.keys())`.
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -48,7 +48,8 @@
from apache_beam.portability.api import schema_pb2
from apache_beam.transforms import external
from apache_beam.transforms import window
-from apache_beam.transforms.fully_qualified_named_transform import FullyQualifiedNamedTransform
+from apache_beam.transforms.fully_qualified_named_transform import \
+ FullyQualifiedNamedTransform
Review Comment:
Imports are an exception to line length limits; keep it on the same line.
Also, don't use backslashes for line continuations: https://google.github.io/styleguide/pyguide.html#32-line-length
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on PR #30035:
URL: https://github.com/apache/beam/pull/30035#issuecomment-1899491438
Looks like test_expand_composite_transform_with_name_input is too strict.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #30035:
URL: https://github.com/apache/beam/pull/30035#discussion_r1457788271
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +675,27 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
Review 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.
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -651,6 +651,16 @@ class WindowInto(beam.PTransform):
See [the Beam documentation on windowing](https://beam.apache.org/documentation/programming-guide/#windowing)
for more details.
+ Sizes, offsets, periods and gaps (where applicable) must be defined using
+ a time unit suffix 's', 'm', 'h' or 'd' for seconds, minutes, hours
+ or days, respectively.
Review Comment:
Mention that if a suffix is omitted, we default to seconds.
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +675,27 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
+ time_units = {'s': 1, 'm': 60, 'h': 60 * 60, 'd': 60 * 60 * 12}
+ value, suffix = str(value)[:-1], str(value)[-1]
+ # Default to seconds if time unit suffix is not defined
+ if suffix.isnumeric():
+ value, suffix = value + suffix, 's'
+ elif not value or not value.isnumeric():
Review Comment:
This won't accept things like 1.5s.
What if instead you broke the suffix off with a regex (e.g. `^(.*?)([^\d]*)$`) and then detected whether the suffix was in our time units dict. (This would allow multi-letter suffixes, like ms, or reject them (rather than giving errors like "s is a valid suffix, but 10hour is not a valid floating point number" even if we didn't support these).
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -665,6 +675,27 @@ def __init__(self, windowing):
def expand(self, pcoll):
return pcoll | self._window_transform
+ @staticmethod
+ def _parse_time_unit(value, name):
Review Comment:
_parse_duration?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [YAML] add time unit suffix to windowing [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb merged PR #30035:
URL: https://github.com/apache/beam/pull/30035
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org