Jackie-Jiang opened a new pull request, #19332: URL: https://github.com/apache/pinot/pull/19332
## 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
