Jackie-Jiang opened a new pull request, #19227: URL: https://github.com/apache/pinot/pull/19227
Part of #19218. ## Summary `FREQUENTSTRINGSSKETCH` and `FREQUENTLONGSSKETCH` never received the query's null handling option, so a null row's column default was folded into the sketch whatever the query asked for. Third of the follow-ups recorded as known deviation 1 on `AggregationFunction`, after #19211 and #19217. ## Behavior change With `enableNullHandling=true`, both functions now skip rows that carry no value, so a null no longer registers as an occurrence of the column's default in the frequency estimate. **On the multi-stage engine the change is unconditional, and no query option controls it.** `AggregateOperator` builds every aggregation function with null handling enabled and never consults the query's option, so these functions previously discarded a flag the engine had already set to `true`. Worth a release note on the same footing as the earlier PRs in this scope. ## The sketch is created inside the non-null range `getOrCreateSketch` created *and stored* a sketch in the result holder before the aggregation loop, in every path. Left that way, a block whose rows are all null would store an empty sketch and the "nothing was aggregated" signal would never reach `extractFinalResult`. It is now created inside the range, with a guard for an empty range so a zero-length block does not trip it either. This is the same defect Copilot caught in #19217, carried forward rather than repeated. ## Deserialization is per row Both functions accept a `BYTES` column of serialized sketches. The old code deserialized the whole block up front, so a row skipped as null was still deserialized. It is now per row, inside the range, and on the multi-value path it happens once per row rather than once per group key the row belongs to. ## The disabled-mode answer With the option off, `extractFinalResult(null)` renders what an empty sketch has always serialized to, built at the render point. This resolves an existing split. A zero-length block previously got a serialized empty sketch, through the eagerly created accumulator, while a query whose segments were all pruned got `NULL` — two answers for the same state. Both now give the serialized empty sketch, which is the identity the contract calls for. ## Tests `AggregationFunctionNullContractTest` now measures both functions as honouring the option, and they are pinned accordingly. That pinning has a history worth stating: these are the same two functions #19220 **removed** from that set as false positives, when the check compared serializer-valued answers by identity and so reported every one of them as honouring the option. They are back because they genuinely honour it now, and the comment in the test says so, to keep the round trip from reading as a mistake. -- 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]
