fix!: apply ReadRel projection during schema inference - #266
alexandrefimov wants to merge 2 commits into
Conversation
`infer_rel_schema` currently returns the full `base_schema` for a masked read. A mask selecting one field can therefore report all input fields, and emit mappings are evaluated against the wrong columns. Apply the read projection before emit, following the [Read Operator in spec v0.99.0](https://github.com/substrait-io/substrait/blob/v0.99.0/site/docs/relations/logical_relations.md#read-operator). Recursively project struct fields, list elements, and map values while preserving their nullability and container types. Keep explicit `RelRoot.names` and the original input schema unchanged. For projected reads, leave correlations inside `filter` and `best_effort_filter` offset-based: [Read Filtering](https://github.com/substrait-io/substrait/blob/v0.99.0/site/docs/relations/logical_relations.md#read-filtering) explicitly places those predicates before projection. Mask unwrapping is a compatibility choice. The [unwrapping section](https://github.com/substrait-io/substrait/blob/v0.99.0/site/docs/expressions/field_references.md#unwrapping-behavior) describes singleton unwrapping by default but leaves its serialization as TBD. This change follows Java's type projector: it preserves structs and containers at every level regardless of `maintain_singular_struct`. For example, selecting only the string from a nested `struct<i64, string>` keeps a nested `struct<string>`, even with the flag false. The validator instead requires the flag for a single-field read projection. Struct fields also follow Java's mask order, including reordered and repeated selections. The same spec page still lists column reordering as an open question, so preserving mask order and duplicates remains a compatibility choice. Closes substrait-io#264 BREAKING CHANGE: ReadRel projection masks now change inferred output schemas and the indices seen by emit and parent relations. Invalid mask field indices and selector/type mismatches now raise errors instead of being ignored.
| if rel.read.HasField("projection"): | ||
| # Spec v0.99.0 describes default unwrapping but leaves its | ||
| # serialization open. Match Java by keeping structs and containers | ||
| # at every level, regardless of maintain_singular_struct. |
There was a problem hiding this comment.
this sounds like a bug in java impl, right? why should we ignore maintain_singular_struct?
There was a problem hiding this comment.
Right, MaskExpressionTypeProjector never consults it. The flag switches unwrapping on top of the field selection this PR applies: with true we already do what it asks, and false would only matter below the top level, since infer_rel_schema has to return the read's row as a Type.Struct. Reworded that comment in 95a2e9a to rest on the schema rather than on Java, and to say that nested levels keep the struct too.
The comment rested on substrait-java doing the same, which says nothing about this code a year from now, and it left the nested case unstated. A relation's schema is a struct, so a mask that selects one field at the top level has nothing to unwrap into; nested levels follow that, and maintain_singular_struct is never read.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughRead schema inference now applies projection masks before emit mappings and recursively projects nested types. Projected-read filter references remain offset-based during relation conversion. Tests cover ordering, validation, nested collections, downstream indices, correlations, and plan immutability. ChangesRead projection handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
infer_rel_schemacurrently returns the fullbase_schemafor a masked read. A mask selecting one field can therefore report all input fields, and emit mappings are evaluated against the wrong columns.Apply the read projection before emit, following the Read Operator in spec v0.99.0. Recursively project struct fields, list elements, and map values while preserving their nullability and container types. Keep explicit
RelRoot.namesand the original input schema unchanged. For projected reads, leave correlations insidefilterandbest_effort_filteroffset-based: Read Filtering explicitly places those predicates before projection.Mask unwrapping is a compatibility choice. The unwrapping section describes singleton unwrapping by default but leaves its serialization as TBD. This change follows Java's type projector: it preserves structs and containers at every level regardless of
maintain_singular_struct. For example, selecting only the string from a nestedstruct<i64, string>keeps a nestedstruct<string>, even with the flag false. The validator instead requires the flag for a single-field read projection.Struct fields also follow Java's mask order, including reordered and repeated selections. The same spec page still lists column reordering as an open question, so preserving mask order and duplicates remains a compatibility choice.
Closes #264
BREAKING CHANGE: ReadRel projection masks now change inferred output schemas and the indices seen by emit and parent relations. Invalid mask field indices and selector/type mismatches now raise errors instead of being ignored.
Summary by CodeRabbit
Bug Fixes
Tests