fix(core): retain reference scope during dereference

[bvolpato]

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.

Nested outer and lambda field paths remain unsupported when converting protobuf back to POJOs or POJOs back to Calcite. Reject these paths explicitly instead of dropping segments or selecting another field. Reading nested lambda parameter paths remains tracked in #1322.

Summary by CodeRabbit

• Bug Fixes
  • Corrected nested field dereferencing to preserve the reference’s existing scope and correlation context, improving handling of nested fields in correlated queries.
• Tests
  • Added coverage for dereferencing struct, list, and map fields across reference scopes, including correlated nested-field queries.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Caution

Review failed

An error occurred during the review process. Please try again later.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 166d0662-2d4d-44f6-9f4b-44e0db9ee854

📥 Commits

Reviewing files that changed from the base of the PR and between 4d21734 and 527eaf7.

📒 Files selected for processing (3)

• core/src/main/java/io/substrait/expression/FieldReference.java
• core/src/test/java/io/substrait/expression/FieldReferenceDereferenceTest.java
• isthmus/src/test/java/io/substrait/isthmus/CorrelatedNestedFieldTest.java

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

FieldReference.dereference now copies the existing reference before replacing its segments. New tests check dereference behavior across reference scopes and verify correlated nested-field references in a converted plan.

Changes

Field reference dereferencing

Layer / File(s)	Summary
Dereference behavior and validation core/src/main/java/io/substrait/expression/FieldReference.java, core/src/test/java/io/substrait/expression/FieldReferenceDereferenceTest.java, isthmus/src/test/java/io/substrait/isthmus/CorrelatedNestedFieldTest.java	dereference copies the current reference, sets the new type, and prepends the new segment. Tests check segment ordering, scope preservation, serialized scope fields, and correlated nested-field references in a converted plan.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: nielspardon

Merge Risk: 🟡 Moderate · up to 527ea

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 Review

Security architecture risk: 🔵 Low · up to 527ea

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

• Low · reliability · inferred: Scope-preserving dereference outputs can reach existing reverse-conversion branches that consume only one segment instead of rejecting unsupported nested paths. This creates a producer/consumer failure-containment gap: reverse conversion may return a different reference. The reader defects predate the PR, but this dereference path now generates the scoped forms that select them. No authorization or data-exposure consequence was established.

Security review details

Security Blast Radius

• inferred — The demonstrated propagation scope is library-level reference construction, serialization, and correlated-expression conversion. The evidence does not establish an independently attackable production endpoint, tenant boundary, privileged sink, or deployment-wide exposure.

Trust Boundaries and Controls

• observed — Correlated-field conversion rejects expressions without a relation visitor or a binding relation anchor. Scope preservation does not remove these checks. They establish reference binding, not tenant authorization.

Resilience and Maintainability Implications

• observed — Rex conversion's type-observation hook is not an unconditional rejection mechanism: it returns immediately for the no-op observer. It therefore does not universally contain the scoped-path loss identified in the conversion branches.

Hardening Proposals

• proposed — Reverse-conversion boundaries could reject unsupported nested scoped paths explicitly until complete path preservation is implemented, preventing silent reference substitution.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, uses a valid Conventional Commit format, and clearly describes the main change: preserving reference scope during dereference.
Description check	✅ Passed	The description explains the defect, the behavior being fixed, the affected reference scopes, and the explicit limitation. It provides the required rationale and is relevant to the changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nielspardon]

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.

[nielspardon]

@coderabbitai resume

[coderabbitai]

✅ Action performed

Reviews resumed and review finished.

[bvolpato]

The reverse converters now reject unsupported nested scoped paths explicitly instead of silently selecting another field. Export still preserves the scope. I applied the test cleanups, moved the SQL regression into SubqueryPlanTest, and documented the remaining reader limitation.

[nielspardon]

LGTM