feat(isthmus)!: preserve aggregate output types and semantics through Calcite

[rkondakov]

Fixes #1016. Relates to #379.

What

A Substrait aggregate used to lose part of itself on the way to Calcite, and could not be
recovered on the way back. This PR gives an aggregate a resolved binding that travels with it, so
the plan's declared output type, its function options, its aggregation phase and the exact
extension declaration all survive a Substrait → Calcite → Substrait round trip.

Why — what was lost

	Before	After
Declared output type	Passed to AggregateCall.create, but RelBuilder re-infers a call's type from its operator, so Calcite's inference won. sum(DECIMAL(10,2)), which sum:dec declares as DECIMAL(38,2), came back with the argument's own width.	The plan's type is preserved.
Two measures, same shape, different types	AggregateCall.equals ignores the stored type and RelBuilder deduplicates, so they collapsed into one column — the output arity changed.	Both survive; deduplication is switched off for exactly those aggregates.
Function options	No place for them in a Calcite aggregate call, so count(a) with overflow: ERROR came back without the option, and two counts differing only by an option were indistinguishable.	Carried on the binding and restored.
Aggregation phase	Converting back always produced INITIAL_TO_RESULT. A distributed plan with partial aggregates was silently turned into a plan of full aggregations.	The phase is carried and restored (AGGREGATION_PHASE_UNSPECIFIED is treated as INTERMEDIATE_TO_RESULT, as the proto specifies).
Which declaration	Re-matched against the extension catalog; among equally-shaped declarations it could only guess. A phase consuming an intermediate state could not be matched at all, because the call's operands are then accumulator state rather than the declaration's arguments — a partial zero-argument count() silently came back as a full count(i64).	The exact declaration the plan used is taken from the binding; a phase that consumes intermediate state aligns on its single state operand, whatever the declaration's arity.
Global aggregate with no groupings	Built with zero grouping sets, so measures were re-inferred as if there were no empty group.	Built as one empty grouping set — the correct global aggregation.

How

Each converted measure resolves a ResolvedAggregateBinding, which captures the semantic identity
of the invocation: anchor, kind-aware arguments (value / type / enum, including the selected enum
option), options, phase and invocation semantics. Where Calcite cannot hold what the binding
carries — the chosen output type differs from its inference; the invocation has options, a phase
other than a full INITIAL_TO_RESULT aggregation, a type argument (only value arguments become
Calcite operands), or an enum argument no operator kind encodes (only a single leading std_dev/variance
distribution flag is operator-encoded); or the declaration is not uniquely reconstructable — reverse
re-matching resolves by signature key, falling back to wildcard/coercion matching, and picks by
registration order, so a declaration another variant can shadow for these argument types (e.g.
count(any) invoked with i32 in a catalog that also declares count(i32)) would come back
decided by load order — the aggregate operator is wrapped so that both the binding and the type
travel with it. The type has to travel on the operator's
return-type inference, not just on the AggregateCall: RelBuilder re-creates every pre-built
call with no stored type, and Aggregate's constructor asserts typeMatchesInferred, so a plain
operator carrying a divergent stored type is rejected outright. AggregateFunctionConverter then
rebuilds the invocation from that binding instead of re-matching the operator.

The binding records the plan as it was converted, so it is dropped again when a planner rule has
since changed the call's arguments, and DISTINCT is always read off the Calcite call, because a
rule may legitimately have removed a redundant one. The wrapper opts out of the type-rederiving
operator hooks until they are binding-aware: rollup (getRollup()), splitting
(SqlSplittableAggFunction), singleton flattening (SqlSingletonAggFunction, which
AggregateRemoveRule unwraps) and static flattening (SqlStaticAggFunction, through which that
rule and RelBuilder's already-unique optimization rewrite a call into a constant) — and it
delegates the volatility traits (isDeterministic / isDynamicFunction) so a volatile aggregate
stays volatile.

Supporting this needed TypeExpressionEvaluator to actually work: it used to throw
UnsupportedOperationException("NYI") for anything but an already concrete return type. It now
binds numbered wildcards (the any1 of min(any1) -> any1) and integer type parameters (the P
and S of DECIMAL<P,S>) from the actual argument types and substitutes them, honouring variadic
parameter consistency; a numeric token like the 0 of DECIMAL<P,0> is a literal constraint the
actual type must satisfy, not a parameter. SimpleExtension.Function.resolveType applies the
declaration's nullability policy (MIRROR over the value arguments), and the binding derivation
delegates to it, so there is one production path. Unsupported derivations stay fail-closed and
never fall back to a caller-supplied type — on the standard catalog that means the parameterized
type classes other than decimal (varchar<L1>, fixedchar<L1>, precision_*<P>,
interval_day<P>, list<anyN>, parameterized structs) and multi-line return programs; concat,
assume_timezone and strptime_* are rejected today. quantile is the one standard aggregate
whose output type cannot be derived at all: its LIST?<any> return uses a plain any, which
carries no identity to bind (substrait-io/substrait#1150 tracks the spec fix). For phases that
consume intermediate state, :core follows the argument model upstream intended
(substrait-io/substrait#1151): such an invocation carries exactly the accumulator state, not the
declaration's arguments.

Commits

1. feat(core) — the resolved-binding model (ResolvedArgument, ResolvedFunctionBinding,
   ResolvedAggregateBinding), FunctionBindingResolver (resolve vs. opt-in validate), and a
   working TypeExpressionEvaluator. Self-contained; :core:build and :isthmus:build are green
   at this commit alone.
2. feat(isthmus)! — conversion changes: the AggregateConversion configuration on
   ConverterProvider, the transport wrapper in AggregateFunctions, and the two conversion
   fixes (deduplication, global aggregate).

New configuration

AggregateConversion is configured on the provider —
ConverterProvider.builder().aggregateConversion(...) — and has two independent settings:

• OutputTypeSource — PLAN_OUTPUT (new default) preserves the plan's declared type;
  CALCITE_INFERENCE restores the previous type behavior.
• FunctionBindingValidation — NONE (default) does not check the plan against the extension
  declaration; EXTENSION_DECLARATION requires the declared output type to match the derived one
  and rejects the plan otherwise. Validation fails closed: a shape it cannot check structurally
  (e.g. a declared list<any1>) is rejected, not silently accepted.

The default is PLAN_OUTPUT + NONE: conversion never silently changes a type, but it also does
not assert that the plan is spec-compliant.

Note that CALCITE_INFERENCE does not make the wrapper fully opt-in: an invocation carrying
options, a non-full aggregation phase, a type argument or a non-operator-encoded enum argument —
or naming a declaration another variant can shadow on the reverse path — is wrapped in every mode, since no stock Calcite
operator can express those. The output-type setting governs only the wrappers caused by a type
divergence.

Default: PLAN_OUTPUT (resolved per review discussion)

A converter silently changing a plan's declared type is a correctness problem —
DECIMAL(38,2) becoming DECIMAL(10,2) changes results — and it is what #1016 asks to fix by
default; the PR is marked breaking accordingly. The compatibility costs are real and remain:
operator identity changes for wrapped calls (call.getAggregation() == SUM-style comparisons
fail; use AggregateFunctions.boundBinding / unwrapBound), and rollup / splitting / singleton
flattening are disabled for them. CALCITE_INFERENCE restores the previous type behavior for
callers that prefer it.

Breaking changes / migration

• A converted aggregate now carries the plan's declared output type instead of Calcite's inferred
  one. Configure ConverterProvider.builder().aggregateConversion(new AggregateConversion( OutputTypeSource.CALCITE_INFERENCE, FunctionBindingValidation.NONE)) to restore the previous
  behavior.
• The resulting AggregateCall may carry a wrapper operator rather than the plain SqlAggFunction.
  Consumers comparing the operator by identity should inspect AggregateFunctions.boundBinding;
  before executing a converted plan, AggregateFunctions.unwrapBound(Aggregate) replaces bound
  calls with their delegates and honestly re-infers their types (the carried type cannot survive
  unwrapping — Aggregate's constructor asserts the stored type against the operator's inference).
• Rollup, splitting, singleton flattening and static flattening are disabled for a wrapped call:
  all of them re-infer the transformed call's type from the underlying function or discard the
  call, losing the carried type or binding. Rules that match on SqlKind instead (e.g. AGGREGATE_REDUCE_FUNCTIONS) have
  no operator hook to veto and may still rewrite the call and drop the binding; this is pinned by
  a test so a Calcite upgrade that changes it is noticed.
• Converting a plan whose aggregate has no groupings back to Substrait now yields a single empty
  grouping instead of none — the same global aggregation, spelled the way Calcite spells it.
• ParameterizedType.StringLiteral.isWildcard() now follows the type grammar exactly
  (any / any0–any9, case-insensitive). A third-party declaration using an ordinary parameter
  name that merely starts with any (e.g. f(anything)) previously matched every argument type
  and computed the same signature key as f(any); it now behaves like any other named parameter.
  No shipped extension is affected — the standard catalog only uses any, any1, any2.

Spark

spark's ToAggregateFunction / ToLogicalPlan implement a third argument model for phases
(declared arguments in every phase, phase as orthogonal metadata, with a
count() → count(1) rewrite hack). This PR does not touch it; aligning Spark with the
intermediate-state model that :core now enforces is follow-up work.

Relation to #379

Aggregate function conversion is one of the four Substrait-vs-Calcite type-mismatch points listed
in #379 (scalars and windows were addressed by #1015 and #1059 via TypeObserver). This PR
resolves the consequence of the mismatch for aggregates — the plan's declared type is preserved
instead of silently replaced — but does not wire the aggregate observation point itself:
TypeObservation carries an Expression, which an AggregateFunctionInvocation is not, so
widening it is deferred to a #379 follow-up. The declared-vs-inferred comparison lives in one
place in SubstraitRelNodeConverter so that follow-up can attach the observer without recomputing
the inference (note it is skipped when opaque semantics force the wrapper regardless, and an
inference failure under PLAN_OUTPUT counts as "diverges" instead of failing the conversion).

Testing

• :core — FunctionBindingResolverTest, ResolvedAggregateBindingTest,
  TypeExpressionEvaluatorTest, ToTypeStringTest wildcard boundaries, plus a small
  binding_extensions.yaml for wildcard / literal / option-collision / nested-shape cases.
• :isthmus — cases in SubstraitRelNodeConverterTest.Aggregate covering declared decimal width,
  global aggregation and explicit empty grouping sets, operator identity, enum arguments, options,
  phases (including a parameterized intermediate state, a zero-argument count() at
  INTERMEDIATE_TO_RESULT, and UNSPECIFIED), duplicate measures, the rollup / split / remove /
  reduce rules, both validation modes and their combination, and unwrapping under assertions; a
  CustomFunctionTest case covers an operator whose return-type inference fails.

[rkondakov]

@nielspardon This PR lets aggregate output types come either from the plan or from Calcite’s inference.

I defaulted to the plan’s type because re-inference can change results, e.g. DECIMAL(38,2) to DECIMAL(10,2). The downside is that mismatches require a wrapper operator, which can break operator identity checks and disable rollup or aggregate splitting.

Keeping CALCITE_INFERENCE preserves current behavior but leaves type loss enabled by default.

Which default do you prefer?

[nielspardon]

Really nice piece of work — the problem statement is unusually clear and the design premise holds up.
I checked the two Calcite mechanics it rests on, because if either were wrong the wrapper would be
unnecessary complexity:

• RelBuilder.AggCallImpl2.aggregateCall(...) re-creates every pre-built AggregateCall with
  type = null (RelBuilder.java:4766-4773), so an explicit type genuinely cannot survive
  RelBuilder.aggregate.
• More fundamentally, Aggregate's primary constructor asserts
  typeMatchesInferred(aggCall, Litmus.THROW) (Aggregate.java:177), which Gradle test JVMs enable.
  So a plain SqlAggFunction carrying a divergent stored type fails an assertion — the type must
  travel on the operator's inference rule, and building a LogicalAggregate directly wouldn't help
  either. Worth putting that in BoundSqlAggFunction's Javadoc; it's a stronger justification than
  the RelBuilder one currently given.

I also validated the behaviour against the Substrait spec (local v0.98.0 checkout + upstream issue
history). Most of it is conformant, and two of the things that look like bugs here are actually
spec bugs — I've filed those upstream so they don't block you:

• substrait-io/substrait#1151 — the spec contradicts itself on arguments/output_type for
  non-INITIAL_TO_RESULT phases. algebra.proto:1870-1872 says "exactly the number of arguments
  specified in the function definition" while :1855-1859 says the inputs of an
  INTERMEDIATE_TO_* phase are the intermediate values. Both were added by the same commit
  (d4cfbe0, #231), and the author had described the intended [X] -> Y / [Y] -> Y / [Y] -> Z
  model on #257 eight hours earlier. Your core validateIntermediateSignature is the reading
  upstream intended; it's alignArguments that should move (see inline).
• substrait-io/substrait#1150 — quantile's return: LIST?<any> uses a plain any, which carries
  no identity, so its output type is underivable. It's the only standard-catalog aggregate your
  deriveOutputType fails on — worth listing as a known limitation and linking that issue.

Spec-conformant and verified, so no change needed: plain-any-binds-independently /
any1-identity; equalsIgnoringNullability (scalar_functions.md:128 licenses stripping only the
outermost nullability); the MIRROR override in both directions (scalar_functions.md:105 +
simple_extensions_schema.yaml:187-189 — "they will be ignored"; and tests/cases/boolean/or.test:5
asserts or(bool, bool) = bool independently); default MIRROR; DISCRETE exact matching; option
name/value case-insensitivity (algebra.proto:1876-1881 mandates it — pre-existing EnumArg.of is
the non-conformant one); unknown-option rejection; empty-preference rejection; variadic min
semantics; the INCONSISTENT branch; zero-groupings = one empty grouping set; and rejecting a
partial phase on a non-decomposable declaration.

On your open question — yes, keep PLAN_OUTPUT as the default. Silently narrowing
DECIMAL(38,2) to DECIMAL(10,2) changes results, and asking producers to know about a flag to get
their own types back is the sharper edge. Two things also make it less risky than the description
suggests: the "unrepresentable declared type throws" behaviour is unchanged from before (the old code
converted the declared type unconditionally too), and the wrapper is only added when the type
actually diverges or the invocation carries options/a phase.

About the length of this review

Up front: there are a lot of comments below, and I don't want that to read as a verdict on the
change. It isn't. This PR touches the function-binding, type-derivation and aggregate-conversion
surfaces all at once, and adds ~1000 non-test lines of new :core API — the comment count tracks
that surface area, not the quality of the work. Most items are a few lines; five come with a
ready-to-apply suggestion block and two more carry the replacement code inline.

Here's what I'd actually gate on, so the rest can be triaged or deferred without guessing:

Blocks merge — silent wrong answers (4)

#	What
2	alignArguments drops the binding for any decomposable declaration whose arity isn't 1, so a partial count(*) silently comes back as a full count(i64).
3	UNSPECIFIED phase is treated as a full aggregation, contradicting algebra.proto:1834.
5	The splitter opt-out misses SqlSingletonAggFunction, so AggregateRemoveRule silently drops the binding's phase and options.
8	typeMatches's return true fall-through means EXTENSION_DECLARATION accepts plans the spec forbids. At minimum make the fallback throw.

Worth doing in this PR (6) — 1 (wildcard predicates, spec conformance on new API), 4 (terminal
else), 6 (unwrapBound vs Calcite's assertion), 7 (unconditional inference), 10 (derive vs
resolveType, plus retargeting the five evaluator tests at the production path), 11 (the two
coverage Javadocs, which currently mis-describe what fails).

Fine as follow-ups (3 + the batched lists) — 9 (the intra-PR duplication), 12 (dead
required() guard), 13 (Immutables nit), the small-things list, and the ConverterProvider
architectural point, which is really a conversation about how it lands alongside #1035/#1036 rather
than a change to make here.

Of the missing tests, only the zero-arg count round trip (comment 2) is load-bearing — it's the
case that proves the fix. The rest can follow.

The recurring theme across the blocking four is the same: a few predicates and matchers are
permissive in ways that silently accept a plan rather than fail loudly. That matters more here than
it normally would, because EXTENSION_DECLARATION is sold as a validation mode — a validator that
returns true on a shape it doesn't understand is worse than one that refuses.

Happy to split this into two reviews (correctness now, polish later) if that's easier to work
through, or to pair on any of them.

---

---

Small things

• alignArguments: hoist the loop-invariant if (!declaration.variadic().isPresent()) out of the
  while.
• validateOptions lowercases declared option names into a map per call; two declared names
  differing only in case collide silently.
• Strict validation derives the intermediate type twice per measure
  (validateIntermediateSignature plus outputType()).
• ParameterBindings.isInteger and ReturnTypeEvaluator.resolveInteger both do
  try { Integer.parseInt } catch on the same token — one helper returning OptionalInt would do.
  The .trim() is dead: SubstraitLexer.g4:10 sends whitespace to channel(HIDDEN).
• bindInteger silently skips a numeric token rather than checking it against the actual value, so
  a declared DECIMAL<P,0> (factorial, functions_arithmetic_decimal.yaml:166-169) never
  verifies that the actual scale is 0.
• Tests: assertEquals(false, ...) / assertEquals(true, ...) in
  appliesMirrorNullabilityForScalars -> assertFalse/assertTrue; the three "must not throw"
  tests read better as assertDoesNotThrow; helper methods are interleaved with @Test methods in
  both new test classes.
• AggregateFunctions imports org.checkerframework...Nullable, which nothing else in
  isthmus/src/main uses (core uses jspecify, and this PR uses jspecify in ResolvedArgument).
  Overriding equals(Object)/unwrap/getRollup doesn't need it.
• Architectural, worth a conversation rather than a change here: AggregateConversion is threaded as
  a SubstraitToCalcite constructor parameter plus a second ConverterProvider factory overload
  plus an isDefault() dispatch branch. ConverterProvider is already the config carrier and is
  gaining a Builder in the stacked #1035/#1036 series — putting it there removes the extra
  overload, the isDefault() branch, the "override both" warning, and the
  new SubstraitToCalcite(provider, null, conversion) shape the tests are forced into.

---

Missing tests (batched comment)

• zero-arg count @ INTERMEDIATE_TO_RESULT round trip (comment 2) — the interesting phase case.
• a declared list<any1> against a non-list actual, and h(list<i32>, list<i32?>) (comment 8).
• UNSPECIFIED phase (comment 3).
• CALCITE_INFERENCE + EXTENSION_DECLARATION together.
• a grouping-sets plan containing an explicit empty grouping — exercises the hasEmptyGroup mirror
  on the non-global path, currently only covered via globalAggregation.
• the documented "AGGREGATE_REDUCE_FUNCTIONS may still drop the type" caveat isn't pinned, so a
  future Calcite bump won't reveal if it degrades.
• boundary test for the wildcard predicates (comment 1).

[nielspardon]

Hand-written value class where its two same-PR siblings (ResolvedFunctionBinding,
ResolvedAggregateBinding) are @Value.Immutable with @Value.Check. 28 of ~69 lines are
equals/hashCode/toString/factories that Immutables would generate; FileOrFiles is the
precedent for the nested-discriminator-plus-Optional shape. Not blocking.

To be clear, this is not duplicating FunctionArg: Type extends FunctionArg, so a Type inside
a List<FunctionArg> already means "type argument" and you couldn't express "value argument
identified only by its type" there. That erasure is load-bearing for the staleness check, which
compares rebuilt ResolvedArguments rather than whole Expressions.

[rkondakov]

Deferring as triaged (follow-up).

[nielspardon]

AggregateConversion has the same shape — hand-written equals/hashCode/toString over two fields (:85, :98, :103) — so worth converting both in the same follow-up.

[rkondakov]

Agreed — AggregateConversion goes into the same Immutables follow-up.

[alexandrefimov]

Two notes from the #379 side:

convertMeasure already computes half of #379's aggregate observation. It builds inferredCall to get Calcite's inferred type, then compares it against the plan's declared type to decide whether to wrap. That pair — declared against independently inferred — is what the TypeObserver from #1015 records at the scalar point and #1059 extends to windows. Aggregate conversion is the fourth point listed in #379 and the only one still unimplemented, so if both land independently the inference ends up running twice per measure — which matters given the :525 note that running it unconditionally can now abort a conversion that previously succeeded. Wiring the observer here would pay for it once. Not free: AggregateFunctionInvocation isn't an Expression, so TypeObservation needs widening for it. Happy to take that as the #379 follow-up once this settles — flagging it now so the wrap decision and the observation don't get built twice.

@rkondakov, on the open default question: I'd ship CALCITE_INFERENCE and flip in a follow-up. Three of the findings above — AggregateRemoveRule slipping past the splitter opt-out, unwrapBound producing the combination Aggregate's constructor asserts against, and inference running on every measure — are all on the wrapper path. Making it the default before those are closed means every consumer meets them at once, whereas opt-in lets the mechanism land and get exercised first. The type-loss bug then stays on by default, which is the real cost — but that's the status quo, not a new failure mode.

[rkondakov]

@nielspardon Thanks — the Calcite mechanics verification and the two upstream spec issues are hugely
appreciated. Status, following your gating:

Blockers — all fixed, each with the test that proves it: #2 (intermediate-state phases align
on the single state operand; zero-arg count() @ INTERMEDIATE_TO_RESULT round-trips), #3
(UNSPECIFIED implies INTERMEDIATE_TO_RESULT), #5 (opt-out now covers
SqlSingletonAggFunction; AGGREGATE_REMOVE test), #8 (typeMatches fails closed; the
list<any1> and h(list<i32>, list<i32?>) cases are rejected).

Worth-doing six — done: #1 (both predicates transcribe the lexer; ToTypeString fixed too),
#4, #6 (redesigned rather than documented — see thread), #7 (with one correction: the inference
runs inside AggregateCall.create, so the guard wraps the factory call), #10 (resolveType
applies the policy, deriveOutputType delegates), #11 (both Javadocs list the real failing
shapes; the BoundSqlAggFunction Javadoc now carries the typeMatchesInferred justification).

Architectural point — done as well: #1035/#1036 are on main now, so AggregateConversion
moved into ConverterProvider.Builder; the second factory overload, the three-argument
SubstraitToCalcite constructor, isDefault() and the "override both" warning are gone.

Deferred as follow-ups, as triaged: #9 (the binder lives in io.substrait.type and the
resolver in io.substrait.extension, so sharing needs a visibility decision — will resolve it
together with the FunctionConverter TODO you pointed at), #13, and one small thing (the double
intermediate derivation under strict validation — memoizing it on the immutable binding is not
worth the contortion until #9's refactor).

Small things otherwise applied, including promoting two of them to real fixes with tests: the
bindInteger numeric-literal check (DECIMAL<P,0> now rejects a non-zero actual scale) and the
case-colliding declared option names (now rejected as ambiguous).

Default stays PLAN_OUTPUT, per your note and the #1016 acceptance criteria; the PR body now also
clarifies that CALCITE_INFERENCE doesn't make the wrapper fully opt-in (options/phases wrap in
every mode). The spec-issue cross-references live in the PR body and here rather than in Javadoc —
repo convention keeps issue links out of source.

[rkondakov]

@alexandrefimov On #379: agreed, and thanks for flagging it before both halves got built — the aggregate observer
wiring is yours as the follow-up, and the PR body now documents the relation. One heads-up for
that follow-up: after the inference-guard fix, convertMeasure computes the declared-vs-inferred
pair in exactly one place, but skips the inference when options or a non-full phase force the
wrapper regardless, and treats an inference failure under PLAN_OUTPUT as divergence. So the
observer hook will want to gate on observer != NOOP to re-enable the computation in the skipped
case — same pattern observeType uses for scalars.

On the default: staying with PLAN_OUTPUT. Your three wrapper-path findings are fixed in this PR
rather than deferred (the splitter opt-out now covers SqlSingletonAggFunction, unwrapBound is
an Aggregate-level helper that honestly re-infers, and the inference is guarded and lazier than
before), which removes the "meet the defects at once" half of the argument. The compatibility
costs that remain — operator identity for wrapped calls, and rollup/split/flatten disabled for
them — are real and stay documented in the PR body; they are the price of the #1016 acceptance
criteria, and the PR is already marked breaking. CALCITE_INFERENCE stays one builder call away
for consumers that prefer the old behavior.

[rkondakov]

Pushed a hardening round on top of the review fixes, from a deeper pass over the new surfaces
(each item has a test; the full build is green):

• Ambiguous declarations now carry their binding. Reverse re-matching resolves by signature
  key and picks among equally-keyed variants by registration order (attemptMatch takes the last
  one), so a count(any) declared by two loaded extensions used to come back with whichever URN
  registered last. A declaration whose key another extension shares — or that has no reverse
  mapping at all — is now treated as opaque and wrapped. (Relatedly,
  substraitFuncKeyToSqlOperatorMap became set-valued: two same-key declarations mapping to one
  operator used to register a duplicate entry and make the forward lookup treat one operator as an
  ambiguity.)
• Type arguments are first-class now. They are validated against their declared pattern under
  EXTENSION_DECLARATION (a declared type: i32 no longer accepts a string), their pattern
  participates in parameter binding ((<type: DECIMAL<P,S>>) -> DECIMAL<P,S> derives instead of
  throwing "Unbound type parameter"), they always force the wrapper (only value arguments become
  Calcite operands), and the reverse conversion restores them from the binding without consuming
  an operand.
• SubstraitRelNodeConverter(RelBuilder, ConverterProvider) now takes its
  aggregate-conversion policy from the provider instead of hard-coding the default.
• INCONSISTENT variadic repetitions no longer escape literal constraints: a declared
  DECIMAL<P,0>... rejects a non-zero scale in every repetition while still letting P differ
  between repetitions.

[rkondakov]

One more hardening pass over the wrapper/uniqueness surfaces (each item has a test; full build
green):

• The uniqueness check now sees wildcard/variadic overlap. It was key-set based, so a catalog
  declaring both count(any) and count(i32) slipped through: a plan invoking count(any) with
  an i32 operand saw disjoint keys, dropped the binding, and reverse direct matching
  reconstructed count(i32). The check now mirrors the reverse selection — direct signature-key
  lookup built from the actual argument types (enum operands probe both req/opt spellings),
  last-registered variant wins — and, when no direct key hits, requires the declaration to be the
  sole variant that can accept the call before letting the binding drop.
• Non-distribution enum arguments are carried. Only the std_dev/variance distribution enum
  is encoded in the operator's SqlKind and re-synthesized on the way back; any other enum
  argument (e.g. a median(EXACT|APPROXIMATE, x) mapped via additionalSignatures) now forces
  the wrapper instead of failing reverse arity matching.
• SqlStaticAggFunction is blocked on the wrapper, alongside SqlSingletonAggFunction: both
  AggregateRemoveRule and RelBuilder's already-unique optimization flatten through it,
  dropping the binding.
• The wrapper delegates isDeterministic() / isDynamicFunction(), so a volatile UDAF is not
  silently treated as stable or cacheable.

[alexandrefimov]

Its only caller passes what the two-arg form already derives — ConverterProvider:417 against :146. So this is a public constructor plus a second configuration channel that silently bypasses a subclass's getAggregateConversion(), for no call site that needs it. Cheaper to drop now than after release.

[rkondakov]

Done — the three-argument constructor is gone: the two-argument one reads the provider's getAggregateConversion() directly, and ConverterProvider's factory calls it, so the provider is the single configuration channel and a subclass override always takes effect. Thanks also for the two calcite-core verifications (splittable-via-singleton subtyping and the type-blind AggregateCall hash) — good to have them pinned against 1.42.

[alexandrefimov]

Thanks, this addresses the concern.

[alexandrefimov]

Checked two of the open threads against calcite-core 1.42 itself rather than the diff; both are answered by the current code.

unwrap never names SqlSplittableAggFunction, but javap on 1.42 gives SqlSplittableAggFunction extends SqlSingletonAggFunction, so the isAssignableFrom check already refuses the splitter request.

hasTypeDistinctDuplicates needs AggregateCall to hash as type-blind as it compares. 1.42 hashes aggFunction, distinct, approximate, ignoreNulls, rexList, argList, filterArg, distinctKeys, collation — no type — so two calls differing only in type collide and get compared.

[rkondakov]

Follow-up tightening on the enum-argument surface (tests included, full build green):

• "Operator-encoded" now means the exact shape the reverse conversion can rebuild — a single
  leading distribution flag on a std_dev/variance kind. A statistical declaration carrying an
  additional enum (or its enum in a non-leading position) previously counted as encoded, dropped
  the binding, and the extra enum could not come back. Now any such invocation rides the binding.
• Variadic trailing enum arguments round-trip. The variadic tail of the reverse alignment
  restored repeated type arguments from the binding but not repeated enums, so
  f(x, flag: [A,B,C]...) lost every repetition after the first; enum repeats are now restored
  the same way (the v0.99.0 extension schema does not forbid variadic enums).
• FunctionBindingResolver's class Javadoc caught up with the code: signature matching is
  described as fail-closed (nested shapes rejected, not accepted unchecked), literal parameters
  constrain every variadic repetition, and an unspecified enum option is always rejected.

[nielspardon]

Three documentation-only suggestions inline — comment and Javadoc text, no behaviour change. Nothing blocking from me.

[nielspardon]

LGTM with the small comment fixes

[rkondakov]

Pushed the round addressing the five comments above, plus a set of correctness fixes on the
enum-encoding and uniqueness surfaces that a further pass over them surfaced (each with a test;
full build green):

• encodesEnumArguments now demands the exact shape the reverse can rebuild, value included.
  The kind fixes the value leadingEnumArgs synthesizes, so a *_POP operator only encodes a
  leading POPULATION — a foreign value (EXACT), the wrong distribution, a different spelling,
  or an unspecified one now rides the binding. It also checks that the declaration lists the
  synthesized value in that exact spelling: EnumArg.of rebuilds the operand with an
  exact-spelling options().contains() lookup, while validation accepts option values
  case-insensitively, so a declaration listing population is reachable and used to throw on the
  way back.
• filterByDistribution matches the distribution value case-insensitively. It used exact
  equals and threw for anything else, so a legally lowercase-spelled population failed the
  forward conversion before any wrapper could be considered. Now the operator resolves, the
  non-exact spelling forces the wrapper, and the round trip restores the plan's own spelling
  verbatim (lowercaseDistributionSpellingSurvivesRoundTrip).
• The uniqueness fallback consults the real reverse matchers instead of approximating them —
  see the thread on uniquelyProvides for details.

One adjacent finding, out of scope here: core's proto→POJO conversion (FunctionArg.ProtoFrom)
rebuilds enum arguments through the strict EnumArg.of, so a plan spelling an option value in a
different case than its declaration cannot be deserialized from proto at all — stricter than the
spec's case-insensitive option matching. Worth a separate look; the new round-trip test pins the
Calcite path only because of it.