Conversation
07dce85 to
b335a63
Compare
Dereferencing a scoped field rebuilds FieldReference without its outer-reference or lambda metadata. SQL such as i.id = o.s.v inside a correlated subquery consequently exports o.s.v as a local root reference; with matching schemas it instead reads i.s.v without a type error. Copy the original reference metadata while extending its path and deriving the selected type. Struct, list, and map dereferences retain their outer steps, relation anchor, or lambda scope, including a zero-step reference to the current lambda. This fixes reference construction and SQL export. It does not add support for nested scoped paths in reverse converters that currently cannot handle them.
b335a63 to
527eaf7
Compare
|
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)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
ChangesField reference dereferencing
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🟡 Moderate · up to Forward conversion preserves reference scope, but reverse conversion can silently change nested correlated references or reject nested lambda references. Handle or explicitly reject unsupported paths before merging to prevent incorrect round-trip results. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change repairs reference binding without demonstrating new access privileges or an authorization bypass. Some reverse conversions can still reinterpret unsupported nested references without rejecting them. Production exposure and end-to-end behavior remain unconfirmed. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
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 |
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Thanks, keeping the scope is right: the spec allows a nested direct_reference under outer_reference and lambda_parameter_reference. The reverse converters don't reject these paths, though. They now read the wrong field without any error, so please add guards (or real handling) in this PR.
|
|
||
| private FieldReference dereference(Type newType, ReferenceSegment nextSegment) { | ||
| return ImmutableFieldReference.builder() | ||
| .from(this) |
There was a problem hiding this comment.
Reject nested scoped paths in these readers, or handle them. Before this change they threw on these references; now they return a wrong result. These lines are outside the diff, so I couldn't attach suggestions:
ExpressionRexConverter.java:834(outer) and:863(lambda) takesegments().get(0), which is the innermost step. The new isthmus query converts back to Calcite asi.id = $cor0.ID, and a lambdax.f1becomes(p0, p1) -> p1. Throwing whensegments().size() > 1covers both.ProtoExpressionConverter.java:94reads only the topstruct_fieldand drops itschild, so the exported plan reads back asi.id = o.s(INTEGER vs ROW). The lambda case at:119already throws onhasChild(); do the same here.
An assertProtoPlanRoundrip on the new isthmus query would have caught the second one.
| case OUTER_ANCHOR: | ||
| builder.outerReferenceRelReference(7); | ||
| break; | ||
| case LAMBDA_CURRENT: |
There was a problem hiding this comment.
A dereferenced lambda parameter now writes a nested lambda_parameter_reference path, which ProtoExpressionConverter:119 still rejects, so these plans no longer survive POJO → proto → POJO. That reader side is item 3 of #1322; either handle it here or note it as a known gap in the PR description.
| FieldReference reference = reference(scope, R.struct(R.BOOLEAN, N.I64)); | ||
|
|
||
| assertDereference( | ||
| reference, reference.dereferenceStruct(1), N.I64, FieldReference.StructField.of(1)); |
There was a problem hiding this comment.
Dereference a different field than the base's StructField(1), so the expected list isn't symmetric and a segment-order regression fails this case too.
| reference, reference.dereferenceStruct(1), N.I64, FieldReference.StructField.of(1)); | |
| reference, reference.dereferenceStruct(0), R.BOOLEAN, FieldReference.StructField.of(0)); |
| assertEquals(expectedType, dereferenced.getType()); | ||
| assertEquals(List.of(nextSegment, original.segments().get(0)), dereferenced.segments()); | ||
| assertEquals(original.inputExpression(), dereferenced.inputExpression()); | ||
| assertEquals(original.outerReferenceStepsOut(), dereferenced.outerReferenceStepsOut()); | ||
| assertEquals(original.outerReferenceRelReference(), dereferenced.outerReferenceRelReference()); | ||
| assertEquals( | ||
| original.lambdaParameterReferenceStepsOut(), | ||
| dereferenced.lambdaParameterReferenceStepsOut()); | ||
|
|
||
| io.substrait.proto.Expression.FieldReference originalProto = | ||
| expressionProtoConverter.toProto(original).getSelection(); | ||
| io.substrait.proto.Expression.FieldReference dereferencedProto = | ||
| expressionProtoConverter.toProto(dereferenced).getSelection(); | ||
| assertEquals(originalProto.getRootTypeCase(), dereferencedProto.getRootTypeCase()); | ||
| assertEquals(originalProto.getOuterReference(), dereferencedProto.getOuterReference()); | ||
| assertEquals( | ||
| originalProto.getLambdaParameterReference(), | ||
| dereferencedProto.getLambdaParameterReference()); |
There was a problem hiding this comment.
Compare the whole object instead of listing attributes by hand, so a newly added attribute that dereference drops is caught. Also remove the then-unused java.util.List import.
| assertEquals(expectedType, dereferenced.getType()); | |
| assertEquals(List.of(nextSegment, original.segments().get(0)), dereferenced.segments()); | |
| assertEquals(original.inputExpression(), dereferenced.inputExpression()); | |
| assertEquals(original.outerReferenceStepsOut(), dereferenced.outerReferenceStepsOut()); | |
| assertEquals(original.outerReferenceRelReference(), dereferenced.outerReferenceRelReference()); | |
| assertEquals( | |
| original.lambdaParameterReferenceStepsOut(), | |
| dereferenced.lambdaParameterReferenceStepsOut()); | |
| io.substrait.proto.Expression.FieldReference originalProto = | |
| expressionProtoConverter.toProto(original).getSelection(); | |
| io.substrait.proto.Expression.FieldReference dereferencedProto = | |
| expressionProtoConverter.toProto(dereferenced).getSelection(); | |
| assertEquals(originalProto.getRootTypeCase(), dereferencedProto.getRootTypeCase()); | |
| assertEquals(originalProto.getOuterReference(), dereferencedProto.getOuterReference()); | |
| assertEquals( | |
| originalProto.getLambdaParameterReference(), | |
| dereferencedProto.getLambdaParameterReference()); | |
| assertEquals( | |
| ImmutableFieldReference.copyOf(original) | |
| .withType(expectedType) | |
| .withSegments(nextSegment, original.segments().get(0)), | |
| dereferenced); |
| } | ||
|
|
||
| private FieldReference reference(ReferenceScope scope, Type type) { | ||
| ImmutableFieldReference.Builder builder = |
There was a problem hiding this comment.
Nit: build these with the existing factories (newRootStructReference, newStructReference, newRootStructOuterReference, newRootStructOuterReferenceByRelReference, newLambdaParameterReference) in a switch expression, which also drops the unreachable default.
| import org.apache.calcite.sql.parser.SqlParseException; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class CorrelatedNestedFieldTest { |
There was a problem hiding this comment.
Nit: move this into SubqueryPlanTest as a @Test using toProto(toSubstraitPlan(sql, catalog)), since it repeats that class's correlated-EXISTS navigation step for step.
|
@coderabbitai resume |
✅ Action performedReviews resumed and review finished. |
2 participants
Dereferencing a scoped field rebuilds FieldReference without its outer-reference or lambda metadata. SQL such as i.id = o.s.v inside a correlated subquery consequently exports o.s.v as a local root reference; with matching schemas it instead reads i.s.v without a type error.
Copy the original reference metadata while extending its path and deriving the selected type. Struct, list, and map dereferences retain their outer steps, relation anchor, or lambda scope, including a zero-step reference to the current lambda.
This fixes reference construction and SQL export. It does not add support for nested scoped paths in reverse converters that currently cannot handle them.
Summary by CodeRabbit