Skip to content

Give the funnel functions the query's null handling option - #19332

Merged
Jackie-Jiang merged 1 commit into
apache:masterfrom
Jackie-Jiang:null_contract_funnel_window
Aug 21, 2026
Merged

Give the funnel functions the query's null handling option#19332
Jackie-Jiang merged 1 commit into
apache:masterfrom
Jackie-Jiang:null_contract_funnel_window

Conversation

@Jackie-Jiang

Copy link
Copy Markdown
Contributor

Summary

Part of #19218. Follows #19316 (sketch-backed distinct counts) and #19326 (value aggregations).

The six funnel types — FUNNELMAXSTEP, FUNNELCOMPLETECOUNT, FUNNELSTEPDURATIONSTATS, FUNNELMATCHSTEP,
FUNNELEVENTSFUNCTIONEVAL and FUNNELCOUNT — 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.

Which null disqualifies a row is decided per column

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 each column's rule follows from what that column is for:

Column Rule Why
Timestamp (window funnels) skip the row an aggregate ignores rows whose input is NULL, and the event has no position in the window
correlateBy key (FUNNELCOUNT) skip the row the key is what distinct counts are taken over; with a composite key a null in any component leaves the whole key undefined
Step predicates not gated a predicate over NULL 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 not gated payload on a matched event, not an input; dropping the row would lose an event that really happened

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#getNullBitmap ORs its arguments' bitmaps, so null timestamps genuinely arrive. One was
being 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 correlateBy key was read as the default's dictionary id — a real
user's id
— so the funnel credited that user with a step they never took.

Testing

FunnelNullHandlingTest is new and covers both architectures, including that KEEP_ALL must not fabricate a dummy
event for a null-timestamp row. 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 was aggregated" in the disabled
mode needs an untouched holder rather than an all-null block.

AggregationFunctionNullContractTest gains multi-value block shapes, which make SUMARRAYLONG and SUMARRAYDOUBLE
drivable 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 NULL and was previously unpinned.

Contract documentation

With this the option reaches every user-facing aggregation, so the first known deviation on AggregationFunction
is 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#getExtraFieldsBlocks and TimeSeriesAggregationFunction switch on the logical value
type rather than the stored type, so BOOLEAN and JSON columns are rejected where they would work. That is a
type-dispatch bug rather than a null-handling one, and it widens accepted input, so it is going in its own PR.

@Jackie-Jiang Jackie-Jiang added query Related to query processing functions Related to scalar or aggregation functions null support Related to NULL value handling backward-incompat Introduces a backward-incompatible API or behavior change release-notes Referenced by PRs that need attention when compiling the next release notes labels Aug 21, 2026
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.
@codecov-commenter

codecov-commenter commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.78157% with 153 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.03%. Comparing base (3c92923) to head (e56d88c).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...w/FunnelEventsFunctionEvalAggregationFunction.java 6.34% 59 Missing ⚠️
...egation/function/HistogramAggregationFunction.java 46.98% 42 Missing and 2 partials ⚠️
...gregation/function/funnel/AggregationStrategy.java 42.25% 38 Missing and 3 partials ⚠️
...n/funnel/window/FunnelBaseAggregationFunction.java 84.48% 2 Missing and 7 partials ⚠️
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     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 ?
java-25 67.03% <47.78%> (+0.05%) ⬆️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 67.03% <47.78%> (+0.05%) ⬆️
unittests 67.03% <47.78%> (+0.05%) ⬆️
unittests1 57.77% <47.78%> (+0.08%) ⬆️
unittests2 38.86% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jackie-Jiang
Jackie-Jiang merged commit 59c2883 into apache:master Aug 21, 2026
11 of 12 checks passed
@Jackie-Jiang
Jackie-Jiang deleted the null_contract_funnel_window branch August 21, 2026 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backward-incompat Introduces a backward-incompatible API or behavior change functions Related to scalar or aggregation functions null support Related to NULL value handling query Related to query processing release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants