fix(core)!: re-derive field reference types when copy-on-write replaces a relation - #1061
Conversation
alexandrefimov
left a comment
alexandrefimov
left a comment
There was a problem hiding this comment.
Read this against origin/main and checked the parts that could go wrong on their own; three things, only the first of which I think needs a decision.
The anchor case is the lateral-join case. retypeRootReference leaves an outer reference identified by rel_anchor alone, on the grounds that resolving an anchor needs plan-wide context. That is true of anchors in general — one can point into a ReferenceRel-shared subtree — but the common producer of them is not general: ProtoRelConverter.newLateralJoin registers anchorScopes.put(anchor, left.getRecordType()) before converting the right input, precisely because that is where the references live, and outerReferenceScope special-cases LateralJoin to the left record type for the same reason. visit(LateralJoin) here already rewrites left before right, so the same registration would fit in the same place.
That matters more than the general case, because a lateral join's right input is exactly where a rewrite of the left changes the type its references resolve against — the bug this PR is about, left unfixed for the relation where correlation is most common. I am not sure it is worth doing in this PR rather than the next one; I am fairly sure "Not fixed here" should name it, since as written the reader is left thinking anchors are unresolvable rather than that one resolvable case was deferred.
Checked while looking at this and it holds: inputTypeStepsOut indexes the enclosing stack exactly as the shipped ProtoRelConverter.outerScopeForStepsOut does, including agreeing on stepsOut == 0, and nothing pushes a steps_out scope for a lateral join in either place — so the two mechanisms have the same shape rather than two conventions.
Re-derivation is not quite unconditional. The description says a reference whose cached type disagreed with its input is corrected even when nothing was replaced, and retypeRootReference does that. The expression-rooted branch does not: if the root expression comes back unchanged, visitFieldReference returns empty before resolveType runs. There is a good argument that this is right — the root is right there, and if it did not change neither did its type — but then the claim holds for root references only, and it is the kind of asymmetry that reads as an oversight later.
Is this a breaking release? The title is fix(core) without !, and three things in the description look like they belong on the other side of that line: visitComparisonJoinKey changes a public signature, a rewrite that previously threw now returns a plan with a stale type, and a visitor instance acquires a no-concurrent-reuse contract it did not have. #1058 took the same shape — validation behaviour that consumers could be relying on — and shipped as breaking, and I made the same argument on #1074 for Fetch. Being wrong about this is cheap in one direction and not in the other.
…es a relation A FieldReference caches the type of the field it references. RelCopyOnWriteVisitor copied those references over verbatim, so replacing a subtree with one that emits a different record type left every reference above it carrying the type of the relation that is no longer there — and the record types derived from those references, such as a Project's, were wrong in turn. The visitor now tracks the record type that each relation's own expressions resolve against and re-derives the cached type of every field reference it rewrites from it. Inputs are rewritten before their relation's expressions so the scope is the type a replaced input emits, and enclosing scopes are tracked per subquery boundary so an outer reference re-derives against the relation it steps out to. Each scope is the one the reference actually resolves against, which is not always the relation's inputs concatenated. A join condition, post-join filter and residual expression resolve against the two inputs combined. A hash or merge join key resolves against the single side it selects from, its offsets being side-relative, which is why proto conversion types each side with its own converter. The filter of a read relation resolves against the schema being read. A lateral join's right input resolves the references it makes to the current left row against the left record type, under the join's rel anchor; the left input is rewritten first, so that type is the one a replaced left input emits, mirroring ProtoRelConverter.newLateralJoin, which registers the same scope before converting the right input. Re-derivation does not depend on anything having been replaced: a reference whose cached type disagrees with the root it resolves against is corrected either way, whether that root is a relation or another expression that the rewrite left alone. References that resolve against something outside the tracked scopes keep the type they have: a lambda parameter, and an outer reference naming a rel anchor no enclosing relation exposes, which needs plan-wide context this visitor does not have. Re-deriving a type must not become a new way for a rewrite to fail. A rewrite that drops a column, reshapes a nested type, or changes a container kind can leave a reference selecting something its input no longer has; the resulting tree is invalid either way, so such a reference keeps its cached type instead. Delivering that needed the resolution itself to be total, not a guard in front of a throwing derivation — a guard that checks only the outermost segment still lets a nested reference reach the derivation and throw. FieldReference.resolveType reports the type a chain of segments selects, or nothing, at any depth and for every kind of segment. It sits beside the finders whose rules it mirrors so the two cannot drift apart silently, and it mirrors them exactly, including the two asymmetries that matter: a list element offset is not bounds-checked, because the length of a list is not part of its type, and a map key type is compared exactly, nullability included. One neighbouring bug surfaced while wiring this up and is fixed as well, being load-bearing for the retyping above: visitFieldReference built its replacement without copying the original, dropping the segments and the type. Since the type is mandatory, rewriting any reference rooted at an expression threw instead of returning the rewritten reference. Not fixed here: the types cached on function invocations are not re-derived — that needs the function declarations, which the visitor does not have — so a relation whose record type comes from a measure or window function can still be stale. Expression.ScalarSubquery caches its type the same way. Whether these types should be cached on the POJOs at all is the broader question the issue raises; this is the short-term fix it asks for. The off-by-one bound check in StructFieldFinder also stays, because the exception it produces is part of what ProtoExpressionConverter reports for a malformed plan. Closes substrait-io#185 BREAKING CHANGE: RelCopyOnWriteVisitor.visitComparisonJoinKey now takes the record types of the join's two sides alongside the key, so an override must adopt the new signature. A rewrite that leaves a field reference unable to resolve against its input no longer throws from inside the visitor; it returns a plan in which that reference keeps its stale type, so callers relying on that exception as a validation signal must validate separately. A visitor instance now carries the scope of the traversal it is running, so a single instance can no longer visit several relation trees concurrently; sequential reuse is unaffected.
d86b8b1 to
fd18dcc
Compare
nielspardon
changed the title
alexandrefimov
left a comment
alexandrefimov
left a comment
There was a problem hiding this comment.
Rechecked fd18dcc4. The lateral join now exposes its rewritten left record type under the anchor while visiting the right input. References rooted at an unchanged expression also have their cached type corrected.
Both regression tests fail when the corresponding fixes are removed and pass on this head. The full core test task passes (797 passed, 32 skipped). Both of my earlier comments are addressed.
andrew-coleman
left a comment
andrew-coleman
left a comment
There was a problem hiding this comment.
Scope tracking checks out, and the visitFieldReference bug it fixes is real.
Five comments below. One is a regression — recordTypeOf runs unconditionally, so a pure traversal over a Set with mismatched field counts now throws where it returned on main. The rest are silent mis-typing or a bypassed extension point.
I applied all five and confirmed each reproduces on head and flips with the fix; :core:test 829 pass, spotless and javadoc clean. Two corrections from testing: inSubqueryScope needs public not protected, and the read-relation gap is pre-existing rather than new. Patch available if that beats five suggestions.
Tolerate a relation whose record type cannot be derived, route every field reference position through visit(FieldReference), retype read and update expressions against the schema they resolve against, and expose the subquery boundary to ExpressionCopyOnWriteVisitor subclasses.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: substrait-io/substrait-java/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe copy-on-write visitors now track relation input types and use them to refresh cached field-reference types. ChangesField Reference Retyping
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant RelCopyOnWriteVisitor
participant ExpressionCopyOnWriteVisitor
participant FieldReference
RelCopyOnWriteVisitor->>RelCopyOnWriteVisitor: Push relation input-type scope
RelCopyOnWriteVisitor->>ExpressionCopyOnWriteVisitor: Visit field reference
ExpressionCopyOnWriteVisitor->>RelCopyOnWriteVisitor: Look up root or enclosing-scope type
ExpressionCopyOnWriteVisitor->>FieldReference: Resolve type from root and segments
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Copy-on-write rewrites now refresh cached field-reference types from the correct relation scope. If a reference cannot be resolved, it keeps its cached type instead of failing. Review feedback has been incorporated or addressed, and no outstanding defect has been identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change corrects reference typing but also changes public extension and concurrent-use contracts. No introduced security vulnerability was established. Compatibility and validation in downstream applications remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 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 |
|
Thanks — applied four of the five, one of them differently.
Lateral join as a |
andrew-coleman
left a comment
andrew-coleman
left a comment
There was a problem hiding this comment.
Four of the five are in, and two came out better than suggested — routing the subquery boundary through a protected final helper on ExpressionCopyOnWriteVisitor is the right shape, and keeps RelCopyOnWriteVisitor's API from widening. My "needs public" was only true of my own version of the fix. :core:test 833 pass, spotless and javadoc clean at 4a06003c.
Two small things left, plus a reply on the open lateral-join thread.
One for the commit message: this removes protected outsideInputScope, so a downstream subclass calling it no longer compiles. Nothing in-repo does, and fd18dcc4 already carries a BREAKING CHANGE: footer for visitComparisonJoinKey — worth appending the removal there, since CHANGELOG.md is generated from those footers.
|
Applied the test in |
3 participants
A
FieldReferencecaches the type of the field it references.RelCopyOnWriteVisitorcopied those references over verbatim, so replacing a subtree with one that emits a different record type left every reference above it carrying the type of the relation that is no longer there — and the record types derived from those references, such as aProject's, were wrong in turn.The visitor now tracks the record type that each relation's own expressions resolve against and re-derives the cached type of every field reference it rewrites from it. Inputs are rewritten before their relation's expressions so the scope is the type a replaced input emits, and enclosing scopes are tracked per subquery boundary so an outer reference re-derives against the relation it steps out to.
Each scope is the one the reference actually resolves against, which is not always the relation's inputs concatenated:
ProtoRelConverter.newLateralJoin, which registers the same scope before converting the right input.Re-derivation does not depend on anything having been replaced: a reference whose cached type disagrees with the root it resolves against is corrected either way, whether that root is a relation or another expression the rewrite left alone.
References that resolve against something outside the tracked scopes keep the type they have: a lambda parameter, and an outer reference naming a rel anchor that no enclosing relation exposes, which needs plan-wide context this visitor does not have.
Resolving a reference is total
Re-deriving a type must not become a new way for a rewrite to fail. A rewrite that drops a column, reshapes a nested type, or changes a container kind can leave a reference selecting something its input no longer has; the resulting tree is invalid either way, so such a reference keeps its cached type instead.
Delivering that needed the resolution itself to be total, not a guard in front of a throwing derivation — a guard that checks only the outermost segment still lets a nested reference reach the derivation and throw.
FieldReference.resolveTypereports the type a chain of segments selects, or nothing, at any depth and for every kind of segment. It sits beside the finders whose rules it mirrors so the two cannot drift apart silently, and it mirrors them exactly, including the two asymmetries that matter: a list element offset is not bounds-checked, because the length of a list is not part of its type, and a map key type is compared exactly, nullability included.Also fixed here
One neighbouring bug surfaced while wiring this up, load-bearing for the retyping above:
visitFieldReferencebuilt its replacement without copying the original, dropping the segments and the type. Since the type is mandatory, rewriting any reference rooted at an expression threw instead of returning the rewritten reference.Not fixed here
The types cached on function invocations are not re-derived — that needs the function declarations, which the visitor does not have — so a relation whose record type comes from a measure or window function can still be stale.
Expression.ScalarSubquerycaches its type the same way. Whether these types should be cached on the POJOs at all is the broader question the issue raises; this is the short-term fix it asks for.The off-by-one bound check in
StructFieldFinderstays as it is (#1068 — the exception it produces is part of whatProtoExpressionConverterreports for a malformed plan, so changing it is not a free fix).In a lateral join's right input, only the
rel_referenceform of a reference to the left row is re-derived against the left record type — the formLateralJoinReldocuments. The deprecatedsteps_outform is not treated as stepping out to the left row: nothing in the spec makes the right input a subquery boundary, andProtoRelConverter.newLateralJoindoes not either, so such a reference resolves against the enclosing subquery scope in both, and changing that belongs in both converters together.Behaviour worth calling out
ScatterExchange's fields and aComparisonJoinKey's sides — are now rewritten throughvisit(FieldReference), so an override of that standard hook applies there too. Rewriting such a position to anything other than a field reference throwsIllegalStateException.visitComparisonJoinKeytakes the two side record types now. It could not previously return a usable reference at all, so nothing can have depended on the old signature.Closes #185
BREAKING CHANGE:
RelCopyOnWriteVisitor.visitComparisonJoinKeynow takes the record types of the join's two sides alongside the key, so an override must adopt the new signature. A rewrite that leaves a field reference unable to resolve against its input no longer throws from inside the visitor; it returns a plan in which that reference keeps its stale type, so callers relying on that exception as a validation signal must validate separately. A visitor instance now carries the scope of the traversal it is running, so a single instance can no longer visit several relation trees concurrently; sequential reuse is unaffected. An override ofExpressionCopyOnWriteVisitor.visit(FieldReference)now also applies to aScatterExchange's fields and aComparisonJoinKey's sides, and rewriting one of those to anything other than a field reference throwsIllegalStateException.