Give the funnel functions the query's null handling option - #19332
Merged
Jackie-Jiang merged 1 commit intoAug 21, 2026
Conversation
The six funnel types never received the query's null handling option, so a null row was read as the column default whatever the query asked for. Both funnel architectures are covered: FunnelBaseAggregationFunction and its four subclasses, the standalone FunnelEventsFunctionEval, and FUNNELCOUNT, whose aggregation lives in AggregationStrategy and its three strategy subclasses. A funnel reads more than one column, so it cannot extend NullableSingleInputAggregationFunction. It follows the multi-input precedent set by CovarianceAggregationFunction, holding the flag itself and skipping rows through a local helper, and which column a null disqualifies the row on is decided per column rather than by one rule. The window funnels skip a row whose timestamp is null. FUNNELCOUNT skips a row whose correlateBy key is null, and with a composite key a null in any component leaves the whole key undefined. Neither skips a row for a null step: a step expression is a predicate, and a predicate over a null operand is UNKNOWN, which SQL treats as not satisfied wherever a boolean is consumed, so a null step already means that step did not match. Extra fields are not gated either, since they are payload on a matched event rather than an input to the aggregate. Both were real defects rather than accounting. BaseTransformFunction#getNullBitmap ORs its arguments' bitmaps, so null timestamps genuinely arrive, and one was being read as the column default and planted a step event at a fabricated time that corrupts window ordering. A null correlateBy key was read as the default's dictionary id, which is a real user's id, so the funnel credited that user with a step they never took. FunnelNullHandlingTest is new and covers both architectures, including that KEEP_ALL must not fabricate a dummy event for a null timestamp. Every case was verified by mutation rather than by inspection. SyntheticBlockValSets gains a single-value dictionary-ids fixture, and its class comment now records that the null bitmap is independent of the values, which is why a test wanting "nothing aggregated" in the disabled mode needs an untouched holder rather than an all-null block. With this the option reaches every user-facing aggregation, so the first known deviation on AggregationFunction is removed and replaced by what the series settled: how a multi-input function decides, and the three internal functions that stay outside the contract. AggregationFunctionNullContractTest gains multi-value block shapes, which make the array sums drivable by the generic harness for the first time, and a pin on the funnels' empty answer, which is zero rather than NULL and was previously unpinned.
Jackie-Jiang
force-pushed
the
null_contract_funnel_window
branch
from
August 21, 2026 22:13
8e161d4 to
e56d88c
Compare
yashmayya
approved these changes
Aug 21, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19332 +/- ##
============================================
+ Coverage 66.97% 67.03% +0.05%
Complexity 1424 1424
============================================
Files 3463 3463
Lines 222078 222149 +71
Branches 34957 34954 -3
============================================
+ Hits 148743 148914 +171
+ Misses 61467 61352 -115
- Partials 11868 11883 +15
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:
|
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 (sketch-backed distinct counts) and #19326 (value aggregations).
The six funnel types —
FUNNELMAXSTEP,FUNNELCOMPLETECOUNT,FUNNELSTEPDURATIONSTATS,FUNNELMATCHSTEP,FUNNELEVENTSFUNCTIONEVALandFUNNELCOUNT— never received the query's null handling option, so a null row wasread as the column default whatever the query asked for. Both funnel architectures are covered:
FunnelBaseAggregationFunctionand its four subclasses, the standaloneFunnelEventsFunctionEval, andFUNNELCOUNT, whose aggregation lives inAggregationStrategyand its three strategy subclasses.Which null disqualifies a row is decided per column
A funnel reads more than one column, so it cannot extend
NullableSingleInputAggregationFunction. It follows themulti-input precedent set by
CovarianceAggregationFunction— holding the flag itself and skipping rows through alocal helper — and each column's rule follows from what that column is for:
correlateBykey (FUNNELCOUNT)There is no SQL standard for funnel functions themselves, but each column role has one, and this is where they land.
Both were real defects
BaseTransformFunction#getNullBitmapORs its arguments' bitmaps, so null timestamps genuinely arrive. One wasbeing read as the column default and planted a step event at a fabricated time, corrupting window ordering and
able to manufacture funnel completions. A null
correlateBykey was read as the default's dictionary id — a realuser's id — so the funnel credited that user with a step they never took.
Testing
FunnelNullHandlingTestis new and covers both architectures, including thatKEEP_ALLmust not fabricate a dummyevent for a null-timestamp row. Every case was verified by mutation rather than by inspection.
SyntheticBlockValSetsgains a single-value dictionary-ids fixture, and its class comment now records that thenull bitmap is independent of the values — which is why a test wanting "nothing was aggregated" in the disabled
mode needs an untouched holder rather than an all-null block.
AggregationFunctionNullContractTestgains multi-value block shapes, which makeSUMARRAYLONGandSUMARRAYDOUBLEdrivable by the generic harness for the first time — the set of undrivable functions shrinking rather than growing —
and a pin on the funnels' empty answer, which is zero rather than
NULLand was previously unpinned.Contract documentation
With this the option reaches every user-facing aggregation, so the first known deviation on
AggregationFunctionis removed. It is replaced by what the series settled: how a multi-input function decides which nulls matter, and
the three functions that stay outside the contract (
ParentExprMinMax,ChildExprMinMax,TimeSeriesAggregation— none of them an aggregate a query can name). The multi-stage engine's unconditional null handling remains as the
sole outstanding deviation.
Known, deliberately not fixed here
FunnelEventsFunctionEval#getExtraFieldsBlocksandTimeSeriesAggregationFunctionswitch on the logical valuetype rather than the stored type, so
BOOLEANandJSONcolumns are rejected where they would work. That is atype-dispatch bug rather than a null-handling one, and it widens accepted input, so it is going in its own PR.