Jackie-Jiang opened a new issue, #19218: URL: https://github.com/apache/pinot/issues/19218
## Goal Every aggregation function should honour the query's `enableNullHandling` option, and should let `extractFinalResult` decide what an empty input means rather than substituting an empty accumulator upstream. The contract is documented on `AggregationFunction` as of #19158. This issue tracks closing the two deviations recorded there as a `TODO` on that interface, one family at a time. ## Why it is one scope but many PRs The two deviations are not separable per function. A function that never receives the option cannot conform its substitution either: without the option, `extractFinalResult` has nothing to decide with, so the substitution is the only thing holding the answer. Each PR therefore does both for one family — threads the option, skips null rows, and moves the disabled-mode identity to the render point. They are split by family rather than landed together because the identity to preserve differs per function and has to be derived from what that function's empty accumulator rendered. Getting that wrong silently changes a backward-compatible answer, so the changes want reviewing in small, verifiable batches. Two recurring hazards, both already hit: - **The function that substitutes is often not the function that renders.** The raw and smart variants delegate extraction to the plain function and serialize what it returns, so conforming the plain function alone changes the raw variant's answer without touching it. They have to move together. - **On the multi-stage engine these changes are unconditional.** `AggregateOperator` builds every aggregation function with null handling enabled and never consults the query option, so a function that discarded the flag starts honouring it there with default options. Each PR calls this out for release notes. ## Landed - [x] #19158 — document and enforce the contract; fix the raw percentile NPE; thread the option into ten multi-value functions; conform every function that already had it - [x] #19201 — always support nulls in `DataTable` serialization, which removed the third deviation entirely and is what makes returning `null` free of fidelity cost ## In flight - [ ] #19211 — the covariance functions - [ ] #19217 — the integer tuple sketch family ## Remaining Grouped the way they want to be reviewed. Counts are classes, including raw, smart and multi-value variants. - [ ] **Sketch-backed distinct counts** (~22) — `DISTINCTCOUNTBITMAP`, `DISTINCTCOUNTHLL`, `DISTINCTCOUNTHLLPLUS`, `DISTINCTCOUNTTHETASKETCH`, `DISTINCTCOUNTCPCSKETCH`, `DISTINCTCOUNTULL`, `FASTHLL`, `SEGMENTPARTITIONEDDISTINCTCOUNT` and their raw and smart variants. The largest group and the one with the most raw variants, so it should follow the pattern rather than set it. - [ ] **Funnel family** (~8) — multi-column, so it can reuse the two-block null merge exposed in #19211. - [ ] **Frequency sketches** (2) — `FREQUENTSTRINGSSKETCH`, `FREQUENTLONGSSKETCH`. - [ ] **One-offs** (~7) — `HISTOGRAM`, `IDSET`, `STUNION`, the array sums, `TIMESERIESAGGREGATE`, `FOURTHMOMENT` and `ParentExprMinMax`. Each has its own identity value and no shared base. ### Not a conformance change - [ ] **Deviation 2**: the multi-stage engine hardcodes null handling on and never reads the query option, so the two engines can answer the same query differently. This may be intended, the multi-stage engine being the SQL-conformant one. It is a decision to make rather than a refactor, and it is what makes every PR above an unconditional change on that engine. ## Test coverage `AggregationFunctionNullContractTest` checks the contract across every function it can construct and drive, and pins both the set that honours the option and the set it cannot reach, in both directions. A function that starts or stops honouring the option fails the build. It cannot drive functions whose input it cannot synthesise — the sketch families need genuine serialized sketches, not the empty byte arrays the harness supplies. Those are pinned in its skip list, so **the suite passing is not evidence for those families** and each needs its own test. #19217 is the first example. -- 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]
