core: validateIntermediateSignature binds the intermediate expression from the state it is checking, so a bare-parameter intermediate can never mismatch

[nielspardon]

FunctionBindingResolver.validateIntermediateSignature checks that an intermediate-consuming phase's state argument matches the declaration's intermediate type. It derives the expected type by calling deriveIntermediateType(declaration, function.arguments()) — but it has just asserted that function.arguments() is exactly the single intermediate-state argument, so the type parameters of the intermediate expression are bound from the state being validated. When the expression is a bare parameter of the same shape as the state, binding reproduces the state and the equality check compares it against itself.

    ResolvedArgument state = arguments.get(0);
    …
    Type intermediate =
        deriveIntermediateType(
            (SimpleExtension.AggregateFunctionVariant) declaration, function.arguments());
    Type stateType = state.type().orElseThrow(IllegalStateException::new);
    if (!stateType.equals(intermediate)) {

min/max are exactly that case. functions_comparison.yaml declares the intermediate identical to the argument:

  - name: "min"
    impls:
      - args:
          - name: x
            value: "DECIMAL<P,S>"
        nullability: DECLARED_OUTPUT
        decomposable: MANY
        intermediate: "DECIMAL?<P,S>"
        return: "DECIMAL?<P,S>"

So P and S bind from whatever state the plan supplies, and every well-formed state validates.

Measured

Sweeping the catalog's decomposable aggregates whose intermediate is a non-concrete ParameterizedType, and asking whether two different well-formed nullable states are both accepted:

variant	main (944b921)	#1141 merge ref (74f0715)
min:dec, max:dec	2/2 accepted — tautological	2/2 accepted — tautological
min:pt, min:pts, min:ptstz, min:iday	0/2 — derivation throws	2/2 accepted — tautological
max:pt, max:pts, max:ptstz, max:iday	0/2 — derivation throws	2/2 accepted — tautological
sum:dec	precision pinned by the literal 38, scale unchecked	same
sum0:dec	non-nullable intermediate, so nullability discriminates	same
avg:dec	throws — parameterized struct (#1239)	same

Concretely on main, min:dec in INTERMEDIATE_TO_RESULT accepts decimal?(10,2), decimal?(38,7), decimal?(1,0) and decimal?(38,2) alike.

The decimal half is pre-existing and not a regression. #1141 does not touch this code; it makes the precision_time / precision_timestamp / precision_timestamp_tz / interval_day shapes derivable at all, which turns eight variants that previously failed closed into eight that accept anything. That is a consequence of the derivation gap closing, not a defect the PR introduces — but it takes the count from 2 to 10, so the check is worth fixing rather than leaving.

What still discriminates

Two things, both incidental: the intermediate's declared nullability (a DECIMAL?<P,S> intermediate rejects a non-nullable state), and any literal in the expression (sum:dec's DECIMAL?<38,S> does pin the precision, though its S is still bound from the state and so unchecked). Nothing checks the parameter values the phase is actually supposed to agree on.

The Javadoc claims the opposite

deriveIntermediateType documents fail-closed behavior that the call site above prevents:

The type parameters are bound from the given arguments, so an intermediate expression that refers to a parameter of the initial arguments cannot be derived from a phase that only sees the intermediate value; that fails closed rather than guessing.

For min/max the intermediate refers only to parameters of the initial arguments (P, S are declared in DECIMAL<P,S> and appear nowhere else), and it does not fail closed — it silently rebinds them from the state. The doc needs correcting whichever way the check goes.

Open question

How a parameter of an intermediate expression binds is not spec-defined; substrait-io/substrait#1151 records the surrounding contradiction, and #1239 covers the multi-field case. Worth deciding rather than picking silently, since the plausible readings differ:

• Bind from the initial arguments — which an intermediate-consuming phase does not have, so the check would have to become "the state's shape matches the intermediate's shape", dropping the parameter comparison explicitly instead of accidentally.
• Treat the state as the binding source deliberately — then the check is only ever a shape and nullability check, and should say so rather than round-tripping an equality that cannot fail.
• Carry the initial argument types on the binding so the parameters can be compared for real, which is the only reading under which this validates anything.

Nothing in the build pins the current behavior either way: no test constructs an intermediate-consuming binding with a mismatched state.