Repository navigation
[YAML] Fix WindowInto day duration parsing and ExtractWindowingInfo duplicate field check (#40902) - #40903
[YAML] Fix WindowInto day duration parsing and ExtractWindowingInfo duplicate field check (#40902)#40903LibenHailu wants to merge 1 commit into
Conversation
…uplicate field check (apache#40902)
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly resolves two bugs in Beam YAML windowing transforms: correcting the 'd' (day) duration multiplier in WindowInto._parse_duration from 12 hours (60 * 60 * 12) to 24 hours (60 * 60 * 24), and fixing _ExtractWindowingInfo so that duplicate field validation checks against field names rather than (name, type) tuples. The accompanying unit tests in yaml_transform_test.py and yaml_mapping_test.py thoroughly cover both fixes.
🔍 General Feedback
- The changes are minimal, well-scoped, and adhere to existing codebase conventions.
- Unit tests directly verify both the duration parsing helper and end-to-end YAML transform execution.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @chamikaramj for label yaml. 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). |
Fixes #40902
Description
This PR fixes two bugs in Beam YAML windowing transforms:
YamlProviders.WindowInto._parse_duration(yaml_provider.py):'d'(day) time unit multiplier from60 * 60 * 12(43,200s / 12 hours) to60 * 60 * 24(86,400s / 24 hours)._ExtractWindowingInfo(yaml_mapping.py):named_fields_from_element_type(pcoll.element_type)returnslist[tuple[str, type]], soelif field in existing_fields:was comparing astragainst(str, type)tuples and always evaluated toFalse.existing_field_names = {name for name, _ in existing_fields}and checkedelif field in existing_field_names:so that colliding windowing field names properly raiseValueError(f'Input schema already has a field named {field}.').Tests Added
YamlWindowingTest.test_parse_durationandYamlWindowingTest.test_windowing_daysinsdks/python/apache_beam/yaml/yaml_transform_test.pyYamlMappingTest.test_extract_windowing_info_duplicate_fieldinsdks/python/apache_beam/yaml/yaml_mapping_test.pyThank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123orfixes #456).CHANGES.mdwith noteworthy changes.