fix(isthmus): preserve declared aggregate output types#1017
Draft
rkondakov wants to merge 1 commit into
Draft
Conversation
rkondakov
marked this pull request as draft
July 18, 2026 12:53
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.
Problem
SubstraitToCalcitereads each measure's declaredoutput_type, resolves the aggregate function, and then rebuilds theAggregateCalls through Calcite, which re-infers each call's type from the operator's own inference rule. Any declared output type that differs from that inference — in width or in nullability — is silently replaced during transport. A conversion should preserve the plan's declaration exactly rather than substitute another inference policy; #1016 has the end-to-end description.Change
AggregateFunctions.withOutputType(aggFunction, outputType)wraps the resolved aggregate in a per-invocation variant whose return-type inference isReturnTypes.explicit(declaredType). All non-type behavior delegates to the wrapped function.SubstraitRelNodeConverter.fromMeasurefirst creates the call with the resolved function and applies the wrapper only when the declared type differs from what Calcite infers, so plans whose declarations already match convert exactly as before.AggregateFunctions.withoutDeclaredOutputTypeunwraps the variant, andAggregateFunctionConverteruses it when matching Calcite calls back to Substrait functions, so the Calcite→Substrait direction and full round trips are unaffected.equals/hashCodedeliberately follow the wrapped function only. Whether two invocations with different declared types should be considered equivalent is a consumer policy; it does not belong in Calcite operator identity.Testing
SubstraitRelNodeConverterTest.declaredMeasureOutputTypesbuilds an aggregate whose measures declare output types Calcite would not infer (SUM declared asi64, AVG declared asfp32), asserts the declared types survive Substrait→Calcite, and asserts the full round trip back to Substrait.🤖 Generated with Claude Code