Repository navigation
Fix Flatten schema unification ordering and type degradation - #41239
vishalmore90 wants to merge 4 commits into
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41239 +/- ##
============================================
- Coverage 57.77% 57.75% -0.03%
Complexity 2617 2617
============================================
Files 1202 1202
Lines 171710 171800 +90
Branches 2163 2163
============================================
+ Hits 99204 99221 +17
- Misses 69845 69918 +73
Partials 2661 2661
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @derrickaw 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). |
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR fixes a bug in YAML Flatten schema unification where fields already present across all input schemas with identical types were unnecessarily wrapped in Optional, and where schemas with mixed Optional[T] and T for the same field collapsed to Optional[Any] (a non-deterministic coder) instead of Optional[T]. This prevented downstream transforms such as Combine / GroupByKey from using those fields as grouping keys.
🔍 General Feedback
- The schema merging logic in
_merge_schemascleanly usesnative_type_compatibility.extract_optional_typeto normalize base types and track field presence/nullability across inputPCollections. - The added unit and E2E tests thoroughly cover non-nullable preservation, mixed optionality, missing fields, conflicting types, and the
Flatten->Combinegrouping scenario.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
PTAL @derrickaw |
Fixes #41238
Description
In Beam YAML,
YamlProviders.Flatten._merge_schemas()handles schema unification when flattening multiplePCollections. Previously:set().union(*(s.keys() for s in schemas)), causing non-deterministic schema field ordering across Python runs.intandOptional[int]) was treated as a type collision and degraded the unified field type toOptional[Any].Optional[...], even when present and required across all input schemas.PCollections with identical schemas were still subjected to synthetic schema creation and per-element row conversion via_unify_element_with_schema.This PR fixes these issues by:
Noneearly from_merge_schemasif allPCollections share identical schemas, allowingbeam.Flattento execute natively without row conversion overhead.dict.fromkeys(...)to preserve deterministic field ordering._match_is_optionalandextract_optional_typeto unify types with compatible bases (e.g.,int+Optional[int]->Optional[int]).Checklist
Fixes #41238).CHANGES.mdunder## Bugfixes.yapfandpycodestyle.