Jackie-Jiang opened a new pull request, #19357: URL: https://github.com/apache/pinot/pull/19357
## Summary Closes the function list in #19218. `EXPR_MIN` / `EXPR_MAX` and `TIMESERIESAGGREGATE` were the last two aggregations that never received the query's `enableNullHandling` option — neither had a single `getNullBitmap` call anywhere — so both read their whole block and let a null arrive as the column default. ### EXPR_MIN / EXPR_MAX The measuring columns form one composite key, so a null in any of them leaves the key undefined and the row cannot take part in the comparison at all. Before this, a null measuring value was compared as the column default and could win outright: `EXPR_MIN(x, y)` returned the row where `x` is NULL whenever the default sorted lowest. The projection columns are only payload carried out of the winning row, so a null there does not disqualify it. That is the same split the window funnels make between their timestamp and their extra fields. ### TIMESERIESAGGREGATE Both the value and the timestamp gate the row: a null value has nothing to add to its bucket, and a null timestamp leaves the point with no bucket to land in. The series builder is now created inside the non-null range rather than before the loop, so a block whose rows are all null leaves the holder untouched — which `extractFinalResult` already documented as meaning "nothing was aggregated", it just could not happen before. This function is not reachable from SQL, but the time-series plan node passes its query options straight through to `QueryContext`, so a time-series query that sets `enableNullHandling` does arrive here with the option on. The factory had `nullHandlingEnabled` in hand and was discarding it. ### An unrelated bug in the same method `ParentExprMinMaxAggregationFunction.aggregate` called `initializeWithNewDataBlocks` only while creating its accumulator. `initializeWithNewDataBlocks` is what rebinds the value-set wrappers to the current block, so from the second block onwards the wrappers still pointed at the *first* block's value sets and were read at the new block's row offsets. Any `EXPR_MIN` / `EXPR_MAX` without a `GROUP BY` over more than one `DocIdSetBlock` — more than 10k matching docs — could return a wrong row. `aggregateGroupBySV` and `aggregateGroupByMV` both already called it unconditionally; `aggregate` now matches them. `secondBlockIsReadWithItsOwnValues` covers it. ### Tests Neither function can be driven by `AggregationFunctionNullContractTest` — the factory rejects `EXPRMIN` / `EXPRMAX` outright, and `TIMESERIESAGGREGATE` needs time-series plan context — so each gets a targeted test in the `FunnelNullHandlingTest` mould. The harness's `EXPECTED_UNCONSTRUCTIBLE` entry for them now names the test that covers each, so the list reads as "checked elsewhere" rather than "cannot be checked", matching how the neighbouring exclusion set is already documented. One thing reviewers should know: the time-series test reads bucket values off the series builder rather than through `extractFinalResult`. A bucket no row landed in stays `null` in the builder's `Double[]`, and `extractFinalResult` feeds that array straight into a `DoubleArrayList`, which unboxes. That is pre-existing and on the SQL path the class documents as unsupported, but skipping null rows makes empty buckets more likely, so it is worth a separate look. ### Compatibility Query answers change where null handling is enabled and a gating column contains nulls, which is the point of the change. The `initializeWithNewDataBlocks` fix changes `EXPR_MIN` / `EXPR_MAX` answers regardless of the option, since it replaces wrong rows with right ones. -- 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]
