feat(isthmus)!: preserve aggregate output types and semantics through Calcite#1017
Open
rkondakov wants to merge 2 commits into
Open
Conversation
rkondakov
marked this pull request as draft
July 18, 2026 12:53
…ut types
Adds a resolved-binding model for extension function invocations, and makes
TypeExpressionEvaluator actually evaluate parameterized return types instead of
throwing.
ResolvedArgument keeps an argument's kind (value, type or enum) and its selected
enum option, so two invocations differing only by an enum argument — e.g.
std_dev(POPULATION, fp32) vs std_dev(SAMPLE, fp32) — are not conflated, and an
enum the plan left unspecified stays distinct from any specified option.
ResolvedFunctionBinding captures the semantic identity of an invocation: anchor,
ordered arguments and options. ResolvedAggregateBinding adds the aggregate phase
and invocation semantics, and selects the declaration's intermediate type for a
phase that stops at the intermediate state rather than its return type.
FunctionBindingResolver keeps the two concerns apart. resolve() only captures
identity and performs no validation, so an invocation that merely differs from
its declaration still resolves. validate() opts into checking arity, argument
kinds and types, enum options, function options and the plan-declared output
type against the declaration.
TypeExpressionEvaluator previously threw UnsupportedOperationException("NYI") for
anything but an already concrete return type, so SimpleExtension.Function's
resolveType could not evaluate a parameterized declaration at all. 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, taking variadic parameter consistency and the declaration's MIRROR
nullability policy into account. Occurrences of one numbered wildcard must agree
on a single type, while each plain any binds independently.
Derivations that are not supported — arithmetic derivations, if/then, return
programs — stay fail-closed: they raise rather than fall back to a
caller-supplied type, which is what makes a derived type trustworthy enough to
validate a plan against.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… Calcite A Substrait aggregate used to lose part of itself on the way to Calcite. The output type the plan declared was handed to AggregateCall.create, but RelBuilder re-infers a call's type from its operator, so the plan's type was replaced by Calcite's inference — sum(DECIMAL(10,2)), which the standard sum:dec declaration types as DECIMAL(38,2), came back as the argument's own width. Function options and the aggregation phase have no place in a Calcite aggregate call at all, so they were dropped, and converting back re-matched the operator against the extension catalog: among equally-shaped declarations it could only guess, it always produced a full INITIAL_TO_RESULT aggregation without options, and for a phase consuming an intermediate state it could not match at all, because the call's operands are then accumulator state rather than the declaration's arguments. Each converted measure now resolves a ResolvedAggregateBinding. Where Calcite cannot hold what the binding carries — the chosen output type differs from its inference, or the invocation has options or a phase other than INITIAL_TO_RESULT — the operator is wrapped so that both the binding and the type travel with it, and AggregateFunctionConverter rebuilds the invocation from that binding instead of re-matching. 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. Two further conversion fixes come with it. Measures that are equal to Calcite but carry different types no longer collapse into a single column: AggregateCall equality ignores the stored type and RelBuilder deduplicates, so deduplication is now switched off for exactly those aggregates. And an aggregate with no groupings is built as one empty grouping set rather than none at all, so its measures are no longer re-inferred as if there were no empty group; converting such a plan back therefore spells the same global aggregation the way Calcite spells it, as a single empty grouping. BREAKING CHANGE: converting a Substrait aggregate to Calcite now preserves the plan's declared output type instead of Calcite's inferred one, and the resulting AggregateCall may carry a wrapper operator rather than the plain SqlAggFunction. Consumers that compare the operator by identity should call AggregateFunctions.unwrapBound first; rollup and splitting are disabled for a wrapped call, since both re-infer the type from the underlying function and would discard the preserved one. Pass AggregateConversion with OutputTypeSource.CALCITE_INFERENCE to SubstraitToCalcite to restore the previous behavior. A ConverterProvider subclass that overrides getSubstraitRelNodeConverter(RelBuilder) should override the new getSubstraitRelNodeConverter(RelBuilder, AggregateConversion) as well, otherwise its customization is skipped for a non-default configuration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rkondakov
force-pushed
the
pr-1016-preserve-declared-aggregate-output-types
branch
from
July 26, 2026 10:08
db81ad0 to
4d7aa8f
Compare
rkondakov
marked this pull request as ready for review
July 26, 2026 10:12
Contributor
Author
|
@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. Keeping Which default do you prefer? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1016.
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 → Substraitround trip.Why — what was lost
AggregateCall.create, butRelBuilderre-infers a call's type from its operator, so Calcite's inference won.sum(DECIMAL(10,2)), whichsum:decdeclares asDECIMAL(38,2), came back with the argument's own width.AggregateCall.equalsignores the stored type andRelBuilderdeduplicates, so they collapsed into one column — the output arity changed.count(a)withoverflow: ERRORcame back without the option, and two counts differing only by an option were indistinguishable.INITIAL_TO_RESULT. A distributed plan with partial aggregates was silently turned into a plan of full aggregations.How
Each converted measure resolves a
ResolvedAggregateBinding, which captures the semantic identityof 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, or the invocation has options or a
phase other than
INITIAL_TO_RESULT— the aggregate operator is wrapped so that both the bindingand the type travel with it.
AggregateFunctionConverterthen rebuilds the invocation from thatbinding 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
DISTINCTis always read off the Calcite call, because arule may legitimately have removed a redundant one.
Supporting this needed
TypeExpressionEvaluatorto actually work: it used to throwUnsupportedOperationException("NYI")for anything but an already concrete return type. It nowbinds numbered wildcards (the
any1ofmin(any1) -> any1) and integer type parameters (thePand
SofDECIMAL<P,S>) from the actual argument types and substitutes them, honouring variadicparameter consistency and the
MIRRORnullability policy. Unsupported derivations — arithmeticderivations,
if/then, return programs — stay fail-closed and never fall back to a caller-suppliedtype, which is what makes a derived type trustworthy enough to validate a plan against.
Commits
feat(core)— the resolved-binding model (ResolvedArgument,ResolvedFunctionBinding,ResolvedAggregateBinding),FunctionBindingResolver(resolve vs. opt-in validate), and aworking
TypeExpressionEvaluator. Self-contained;:core:buildand:isthmus:buildare greenat this commit alone.
feat(isthmus)!— conversion changes:AggregateConversionconfiguration, the transportwrapper in
AggregateFunctions, and the two conversion fixes (deduplication, global aggregate).New configuration
AggregateConversionhas two independent settings:OutputTypeSource—PLAN_OUTPUT(new default) preserves the plan's declared type;CALCITE_INFERENCEreproduces the previous behavior.FunctionBindingValidation—NONE(default) does not check the plan against the extensiondeclaration;
EXTENSION_DECLARATIONrequires the declared output type to match the derived oneand rejects the plan otherwise.
The default is
PLAN_OUTPUT+NONE: conversion never silently changes a type, but it also doesnot assert that the plan is spec-compliant. Strict validation is deliberately opt-in, because
type derivation is fail-closed and would reject any function whose return expression it does not
yet support.
Breaking changes / migration
one. Pass
AggregateConversionwithOutputTypeSource.CALCITE_INFERENCEtoSubstraitToCalciteto restore the previous behavior.
AggregateCallmay carry a wrapper operator rather than the plainSqlAggFunction.Consumers comparing the operator by identity should call
AggregateFunctions.unwrapBoundfirst;AggregateFunctions.boundBindingexposes the binding.from the underlying function and would discard the preserved one. Rules that match on
SqlKindinstead (e.g.
AGGREGATE_REDUCE_FUNCTIONS) have no such hook and may still rewrite the call anddrop the type.
ConverterProvidersubclass overridinggetSubstraitRelNodeConverter(RelBuilder)shouldoverride the new
getSubstraitRelNodeConverter(RelBuilder, AggregateConversion)as well;otherwise its customization is skipped for a non-default configuration.
grouping instead of none — the same global aggregation, spelled the way Calcite spells it.
Open question for maintainers
Should preserving the plan's declared output type be the default, or opt-in?
This PR makes
PLAN_OUTPUTthe default, so existing callers get the new behavior without changinga line. The reasoning: a converter silently changing a plan's declared type is a correctness
problem —
DECIMAL(38,2)becomingDECIMAL(10,2)changes results, not just metadata — and aproducer that has to know about a configuration flag to get its own types back is a sharp edge.
The cost is real, though: the new default is a behavior change for every existing consumer, and
where the declared type diverges from Calcite's inference the operator is wrapped, which disables
rollup and aggregate splitting for that call and makes
call.getAggregation() == SUM-stylecomparisons fail.
The alternative is to default to
CALCITE_INFERENCE— byte-for-byte the previous behavior — andlet callers opt into
PLAN_OUTPUT. That keeps every existing consumer untouched and makes thewrapper strictly opt-in, at the price of leaving the type-loss bug on by default.
Happy to flip the default either way; the configuration supports both.
Testing
:core—FunctionBindingResolverTest,ResolvedAggregateBindingTest,TypeExpressionEvaluatorTest, plus a smallbinding_extensions.yamlfor wildcard cases.:isthmus— new cases inSubstraitRelNodeConverterTest.Aggregatecovering declared decimalwidth, global aggregation, operator identity, enum arguments, options, phases (including a
parameterized intermediate state), duplicate measures, rollup and split rules, both validation
modes, unwrapping, and converter-factory dispatch.
./gradlew :core:build :isthmus:buildis green at each of the two commits.🤖 Generated with Claude Code