fix(isthmus)!: apply the projection a read relation carries - #1280
Conversation
`AbstractReadRel` applies a read's projection when it derives the record type, so the relation produces the columns the mask keeps. The conversion built the scan from the initial schema and gave the Calcite node every column of it, mask or no mask.
An emit mapping selects by index, and so does every field reference a parent relation makes. Both index the columns the relation produces, so against that node they landed on the wrong columns, with no error. A `VirtualTableScan` carrying a projection was refused outright rather than converted against those columns.
```
NamedScan t(a i64, b string, c fp64), projection = mask[a, c], emit = [1]
record type Struct{[FP64]} -- column c
Calcite row type RecordType(VARCHAR b) -- column b
```
The mask now becomes a projection above the scan and below the emit mapping, for a named table and a virtual one alike. Converting back gives that projection rather than the mask -- a `Project` over the whole schema, selecting the same columns in the same order -- so a round trip keeps the record type and not the encoding.
A mask that selects inside a column -- some of a struct's fields, some of a list's elements -- throws `UnsupportedOperationException`: applying it would mean rebuilding the column's value, which this conversion does not do.
The masked columns come out in the order the mask lists them. Spec v0.102.0 does not settle whether a mask may reorder at all: `expressions/field_references.md` describes a mask as removing columns and raises reordering as an open question. `MaskExpressionTypeProjector` already derives the record type in that order, though, and refusing a mask that reorders would reject plans the model accepts.
This addresses the projection part of substrait-io#1204, which also names `filter` and `best_effort_filter`. substrait-io#1260 applies `filter`; `best_effort_filter` is still dropped.
BREAKING CHANGE: a `NamedScan` carrying a projection now converts to Calcite with the mask applied, where it used to convert with the mask dropped and give a tree whose columns were not the relation's. A `VirtualTableScan` carrying one converts as well, where it used to be refused. A `NamedScan` whose mask selects inside a column now throws `UnsupportedOperationException`, where that projection used to be dropped in silence along with the rest.
There was a problem hiding this comment.
Cite substrait-io/substrait#1225 in the description as the basis for applying the mask in the order it lists. Its survey (DuckDB writes SELECT c2, c0 FROM t_mix as one read masking [2, 0], and every consumer surveyed reads the listed order, with none rejecting it) is much stronger ground than MaskExpressionTypeProjector happening to derive the record type that way, and naming algebra.proto's MaskExpression comment as the text pulling the other way keeps the choice reviewable. Could you also send the spec change you offered in substrait-io/substrait#1225 alongside this, so the order no longer rests on consensus alone?
|
Done, and the spec change is substrait-io/substrait#1247. |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Add to the BREAKING CHANGE: footer what a consumer should do about the new throw for a mask that selects inside a column, since CONTRIBUTING asks the footer for both what breaks and what to do instead. A ProjectRel above the read isn't an escape hatch there, because isthmus refuses a nested field reference as well.
…s apply it applyProjection is protected like the other apply methods, its Javadoc says why maintainSingularStruct is not read, and applyOutputNames names it among the projections hint names land on. The shared-name test now masks the two columns that share a name.
|
Done: the footer now says to select the whole column. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (3)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughNamed and virtual table scans now apply optional read projections during conversion. The converter selects top-level columns in mask order and throws ChangesRead projection conversion
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change aligns converted scan output with whole-column read masks and explicitly rejects unsupported nested selections. No concrete merge-blocking issue remains in the supplied evidence; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change corrects column selection and reduces downstream exposure to unselected columns. Nested selections now fail explicitly rather than producing an incorrectly shaped result. No introduced authorization bypass was established, but output projection does not guarantee that excluded columns are never accessed at the source. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
AbstractReadRelapplies a read's projection when it derives the record type, so the relation produces the columns the mask keeps. The conversion built the scan from the initial schema and gave the Calcite node every column of it, mask or no mask.An emit mapping selects by index, and so does every field reference a parent relation makes. Both index the columns the relation produces, so against that node they landed on the wrong columns, with no error. A
VirtualTableScancarrying a projection was refused outright rather than converted against those columns.The mask now becomes a projection above the scan and below the emit mapping, for a named table and a virtual one alike. Converting back gives that projection rather than the mask -- a
Projectover the whole schema, selecting the same columns in the same order -- so a round trip keeps the record type and not the encoding.A mask that selects inside a column -- some of a struct's fields, some of a list's elements -- throws
UnsupportedOperationException: applying it would mean rebuilding the column's value, which this conversion does not do.The masked columns come out in the order the mask lists them, as substrait-io/substrait#1225 proposes: DuckDB writes
SELECT c2, c0 FROM t_mixas one read masking[2, 0], and every consumer surveyed there that applies the mask reads the listed order. The text pulling the other way is theMaskExpressioncomment inalgebra.proto(spec v0.102.0), which says a mask only removes elements; substrait-io/substrait#1247 changes it.This addresses the projection part of #1204, which also names
filterandbest_effort_filter. #1260 proposes support forfilter;best_effort_filteris still dropped.BREAKING CHANGE: a
NamedScancarrying a projection now converts to Calcite with the mask applied, where it used to convert with the mask dropped and give a tree whose columns were not the relation's. AVirtualTableScancarrying one converts as well, where it used to be refused. ANamedScanwhose mask selects inside a column now throwsUnsupportedOperationException, where that projection used to be dropped in silence along with the rest. To convert such a plan, have the mask select the whole column: isthmus refuses a nested field reference in aProjectRelabove the read as well, so neither place can prune inside a column.Summary by CodeRabbit