Give the value-accumulating aggregations the query's null handling option, and add their missing multi-value paths - #19326
Merged
Jackie-Jiang merged 1 commit intoAug 21, 2026
Conversation
…tion, and add their missing multi-value paths SKEWNESS, KURTOSIS, STUNION, SUMARRAYLONG, SUMARRAYDOUBLE, HISTOGRAM and IDSET never received the query's null handling option, so a null row was aggregated as the column default no matter what the query asked for. They now extend NullableSingleInputAggregationFunction and skip null rows when the option is enabled, across aggregate, aggregateGroupBySV and aggregateGroupByMV. Five of the seven were not even being passed the flag by the factory. Each of these renders "nothing aggregated" differently with the option disabled - NaN for the moments, the empty point for STUNION, an all-zero histogram, an empty id set, NULL for the array sums - and every one of those is a backward-compatibility constraint. The substitution of an empty accumulator for an untouched holder is therefore kept, guarded on the flag, so the disabled path including the intermediate that crosses the wire is unchanged. Only the enabled path yields the null that means nothing was aggregated. HISTOGRAM and STUNION gain multi-value support. HISTOGRAM carried a "TODO: Add MV support for histogram" and rejected multi-value columns in two of its three methods while the third failed with a different error; STUNION read the single-value representation everywhere. IDSET already handled both but was missing the multi-value BYTES case, although IdSets supports BYTES and the single-value case worked. HISTOGRAM also gains BIG_DECIMAL, which it previously rejected as a "non-numeric type". All three are restructured into three entry points that dispatch to private SV/MV helpers, matching the sketch-backed distinct counts. One bug is fixed along the way that the wrapping introduced and the restructure exposed: forEachNotNull invokes its consumer once per contiguous non-null range, so publishing a locally accumulated histogram from inside the lambda re-added the earlier ranges' counts on every later range. It is accumulated with foldNotNull and published once, guarded on a row count so an all-null block still leaves the holder untouched. SKEWNESS and KURTOSIS keep their single-value, non-BIG_DECIMAL restriction. Both limits live in the shared StatisticalAggregationFunctionUtils.getValSet, which VARPOP, VARSAMP, STDDEVPOP, STDDEVSAMP and the covariances also route through and which carries its own MV TODO, so widening it belongs in its own change. ValueAggregationNullHandlingTest is new and covers what AggregationFunctionNullContractTest cannot: it drives one synthetic single-value block through aggregate only, and cannot construct most of these from its shared argument shapes at all. Every case was verified by mutation rather than by inspection. One mutation survived the first pass - removing the empty-range guard from the array sums - which is what the zero-length block test now pins. SyntheticBlockValSets gains the Int, Float, Double and BigDecimal multi-value fixtures the new paths need.
Jackie-Jiang
added
query
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19326 +/- ##
==========================================
Coverage 67.00% 67.00%
Complexity 1424 1424
==========================================
Files 3463 3463
Lines 221671 222076 +405
Branches 34954 34956 +2
==========================================
+ Hits 148524 148800 +276
- Misses 61310 61393 +83
- Partials 11837 11883 +46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
yashmayya
approved these changes
Aug 21, 2026
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.
Summary
Part of #19218. Follows #19316, which did the sketch-backed distinct counts.
SKEWNESS,KURTOSIS,STUNION,SUMARRAYLONG,SUMARRAYDOUBLE,HISTOGRAMandIDSETnever receivedthe query's null handling option, so a null row was aggregated as the column default whatever the query
asked for. They now extend
NullableSingleInputAggregationFunctionand skip null rows when the option isenabled, across
aggregate,aggregateGroupBySVandaggregateGroupByMV. Five of the seven were not evenbeing passed the flag by
AggregationFunctionFactory.The empty answer is per-function, and each one is a constraint
These seven disagree on what "nothing aggregated" renders to with the option disabled:
NaNfor the moments,the empty point for
STUNION, an all-zero histogram, an empty id set, andNULLfor the array sums. Each isthe answer that mode has always given, so the substitution of an empty accumulator for an untouched holder is
kept and guarded on the flag rather than removed. The disabled path — including the intermediate result that
crosses the wire — is unchanged; only the enabled path yields the
nullthat means nothing was aggregated.SUMARRAYLONG/SUMARRAYDOUBLEneed no such guard, becauseNULLis already their answer in both modes.That is now pinned by a test rather than left to inspection.
Multi-value support
HISTOGRAMcarried a//TODO: Add MV support for histogram, rejected multi-value columns in two of itsthree methods, and failed with a different error in the third. It now supports them, and also
BIG_DECIMAL,which it previously rejected as a "non-numeric type".
STUNIONread the single-value representation in all three paths. A multi-value row now folds everygeometry into the same union.
IDSETalready handled both, but was missing the multi-valueBYTEScase even thoughIdSetssupportsBYTESand the single-value case worked. SV and MV are now symmetric across all six types it supports.All three are restructured into three entry points dispatching to private SV/MV helpers, matching the
sketch-backed distinct counts.
A bug the restructure exposed
forEachNotNullinvokes its consumer once per contiguous non-null range.HISTOGRAMaccumulates into alocal buffer and publishes it, so publishing from inside the lambda re-added the earlier ranges' counts on
every later range — for rows
[v0, null, v2],v0was counted twice. It is now accumulated withfoldNotNulland published once, guarded on a row count so an all-null block still leaves the holderuntouched. The other six accumulate directly into the holder and were never affected.
What is deliberately left out
SKEWNESSandKURTOSISkeep their single-value, non-BIG_DECIMALrestriction. Both limits live in theshared
StatisticalAggregationFunctionUtils.getValSet, whichVARPOP,VARSAMP,STDDEVPOP,STDDEVSAMPand both covariances also route through, and which carries its own MV TODO. Widening it silently changes six
other functions, so it belongs in its own change.
Testing
ValueAggregationNullHandlingTestis new. It covers whatAggregationFunctionNullContractTestcannot: thatharness drives one synthetic single-value block through
aggregateonly, and cannot construct most of thesefrom its shared argument shapes at all.
Every case was verified by mutation rather than by inspection. One mutation survived the first pass —
removing the empty-range guard from the array sums — which is what the zero-length block test now pins.
AggregationFunctionNullContractTestgainsSKEWNESS,KURTOSIS,STUNION,HISTOGRAM,IDSETand botharray sums in the set that must answer
NULLwhen nothing was aggregated, and pinsSKEWNESS/KURTOSIS/IDSET/HISTOGRAMas newly honouring the option.SyntheticBlockValSetsgains theInt,Float,Doubleand
BigDecimalmulti-value fixtures the new paths need.