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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
