fix(core): re-derive field reference types when copy-on-write replaces a relation

[nielspardon]

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 are 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, not against any enclosing scope.

References that resolve against something outside the tracked scopes are left as they are: a lambda parameter, and an outer reference identified by rel anchor rather than by stepping out (which is how a lateral join's right input references the current left row — resolving an anchor 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.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. A parity test asserts resolveType selects something exactly when ofRoot/ofExpression do not throw, which is what keeps the two in step.

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.

Two neighbouring bugs are deliberately left alone rather than folded in, and filed separately: the bound check in StructFieldFinder is off by one (#1068 — the exception it produces is part of what ProtoExpressionConverter reports for a malformed plan, so changing it is not a free fix), and MergeJoin.deriveRecordType reads its right input for both sides (#1067).

Behaviour worth calling out

• Where a narrowing rewrite previously threw from inside the visitor, it now yields a plan in which the unresolvable reference keeps its stale type. Anyone relying on that exception as a validation signal loses it — validation belongs in a validator, not in a copy-on-write rewrite.
• Re-derivation is unconditional, so a reference whose cached type disagreed with its input is corrected even when nothing in the subtree was replaced.
• Tracking the scope makes a visitor instance stateful for the duration of a traversal, so an instance can no longer be used to visit several relation trees concurrently. Sequential reuse is unaffected.
• visitComparisonJoinKey takes 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.

Three further bugs surfaced while wiring this up and are fixed as well:

• 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.
• The input of a ConsistentPartitionWindow was never visited, so no subtree beneath a window relation could be replaced.
• MultiBucketExchange reported no change when only its expression had been rewritten, discarding that rewrite.

Closes #185

🤖 Generated with AI

[alexandrefimov]

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.

[alexandrefimov]

Would registering the lateral-join scope here be enough for the case that matters? visit(LateralJoin) rewrites left before right, so an anchor-to-record-type map filled at that point would cover the same references ProtoRelConverter.newLateralJoin covers, without needing anything plan-wide.

If the answer is "yes but not in this PR", a sentence in "Not fixed here" naming the lateral join would keep the next reader from concluding anchors are simply out of reach.

[alexandrefimov]

This returns before resolveType when the root expression is unchanged, so an expression-rooted reference never gets the unconditional correction that retypeRootReference applies.

Defensible — an unchanged root has an unchanged type — but worth a word here or in the description, since the two branches now differ in a way the summary does not distinguish.