robertwb commented on code in PR #33443:
URL: https://github.com/apache/beam/pull/33443#discussion_r1896108315
##########
sdks/python/apache_beam/yaml/yaml_transform.py:
##########
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
'transforms': [modified_spec] + windowing_transforms,
'input': spec['input'],
'output': modified_spec['__uuid__'],
+ 'config': {
+ 'error_handling': spec.get('config').get('error_handling', None)
+ } or {},
Review Comment:
I don't think the `or {}` will ever trigger, as this will always result in a
key with a single dictionary entry. However, setting letting
error_handling=None should be fine.
##########
sdks/python/apache_beam/yaml/yaml_transform.py:
##########
@@ -805,6 +808,9 @@ def preprocess_windowing(spec):
'name': spec.get('name', None) or spec['type'],
'transforms': [modified_spec] + windowing_transforms,
'output': windowed_outputs,
+ 'config': {
Review Comment:
Same as above.
##########
sdks/python/apache_beam/yaml/yaml_transform.py:
##########
@@ -771,6 +771,9 @@ def preprocess_windowing(spec):
'transforms': [modified_spec] + windowing_transforms,
'input': spec['input'],
'output': modified_spec['__uuid__'],
+ 'config': {
+ 'error_handling': spec.get('config').get('error_handling', None)
Review Comment:
Nit, let's use `spec['config'].get('error_handling', None)` for consistency
(unless we're worried the spec might not have a config, in which case we'd want
`spec.get('config', {})` (which I think we'd need as it happens before the
`ensure_config` phase).
Also, should it be `get('error_handling', {})` rather than
`get('error_handling', None)`
--
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-unsubscr...@beam.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
