refactor: move aggregate expression support checks to getSupportLevel#4678
Merged
andygrove merged 1 commit intoJun 18, 2026
Merged
Conversation
Static data-type and strict-floating-point support decisions for the aggregate expressions are moved out of the serde convert methods and into getSupportLevel: - Min, Max: unsupported data type and strict-floating-point gate (via a shared AggSerde.minMaxSupportLevel helper) - Average, Sum, BitAndAgg, BitOrAgg, BitXorAgg: unsupported data type - BloomFilterAggregate: unsupported child data type Behavior is unchanged: each case that previously fell back to Spark now reports Unsupported. The remaining withFallbackReason calls are child-reason rollups, including the serializeDataType rollups gated on dataType.isEmpty.
comphead
reviewed
Jun 18, 2026
|
|
||
| object CometSum extends CometAggregateExpressionSerde[Sum] { | ||
|
|
||
| override def getSupportLevel(expr: Sum): SupportLevel = |
Contributor
There was a problem hiding this comment.
we prob should have datatype supported on object level rather than on AggSerde level
comphead
reviewed
Jun 18, 2026
| override def supportsMixedPartialFinal: Boolean = true | ||
|
|
||
| override def getSupportLevel(expr: BitAndAgg): SupportLevel = | ||
| if (AggSerde.bitwiseAggTypeSupported(expr.dataType)) { |
comphead
reviewed
Jun 18, 2026
| case _: ByteType | _: ShortType | _: IntegerType | _: LongType | _: StringType => | ||
| Compatible() | ||
| case other => | ||
| Unsupported(Some(s"Unsupported data type for bloom_filter_agg child: $other")) |
Contributor
There was a problem hiding this comment.
Btw this message can unified and be on the base class, unless overriden. Currently unsupported messages are not consistent
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
2 participants
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.
Which issue does this PR close?
Part of #4673.
Rationale for this change
The serde framework intends that an expression declares whether it can be accelerated through
getSupportLevel, with the central dispatcher inQueryPlanSerdetranslatingIncompatible/Unsupportedinto the appropriate fallback. Several aggregate serdes instead made these decisions insideconvertby callingwithFallbackReasondirectly, even though the decisions are statically determinable from the expression.What changes are included in this PR?
Move static support decisions out of
convertand intogetSupportLevelfor the aggregate expressions:Min,Max: unsupported data type and the strict-floating-point gate, via a sharedAggSerde.minMaxSupportLevelhelper.Average,Sum,BitAndAgg,BitOrAgg,BitXorAgg: unsupported data type.BloomFilterAggregate: unsupported child data type (the static type filter that was embedded in the convert guard).This is behavior-preserving: every case that previously fell back to Spark continues to fall back. The remaining
withFallbackReasoncalls in this file are child-reason rollups, including theserializeDataTyperollups gated ondataType.isEmpty, which are the intended use insideconvert.How are these changes tested?
Covered by existing tests in
CometAggregateSuite(80 tests covering min/max/sum/avg/bit aggregates and decimal/float paths) and thebloom_filter_aggtest inCometExecSuite, which exercise both the supported and fallback paths.