Jackie-Jiang opened a new pull request, #19220: URL: https://github.com/apache/pinot/pull/19220
Part of #19218. ## Summary `AggregationFunctionNullContractTest` decides whether a function honours `enableNullHandling` by aggregating an all-null block under both modes and comparing the two final results. It compared them with `Objects.equals` on the objects. Several functions answer with a serializer — `SerializedKLL`, `SerializedTDigest`, `SerializedHLL`, `SerializedCPCSketch`, `SerializedFrequentStringsSketch` and friends — and **none of those implements `equals`**. So the comparison compared identities. Two runs produce two distinct objects, which are never equal, and the function is recorded as honouring the option whatever it actually does. Eight functions were pinned as honouring the option while receiving no option at all: `DISTINCTCOUNTRAWHLL`, `DISTINCTCOUNTRAWHLLPLUS`, `DISTINCTCOUNTRAWHLLMV`, `DISTINCTCOUNTRAWHLLPLUSMV`, `DISTINCTCOUNTRAWULL`, `DISTINCTCOUNTRAWCPCSKETCH`, `FREQUENTSTRINGSSKETCH`, `FREQUENTLONGSSKETCH` All eight are listed under the first known deviation on `AggregationFunction` as functions that never receive the option — none of them contains a `getNullBitmap`, `forEachNotNull` or `_nullHandlingEnabled` reference. The set and the contract were stating opposite things about the same functions. The worse half is the direction that cannot fail. For those functions the assertion could never detect one that *stopped* honouring the option, because a difference was always reported. That is the half meant to catch regressions. ## The fix Compare the value the broker renders — the final result converted through `getFinalResultColumnType` — rather than the object. The comparison is also deep. Converting can yield a primitive array, and arrays compare by identity too: with a shallow comparison, `HISTOGRAM` immediately took the place of the eight that left. The test caught that on the first run of the fix. ## Effect `HONOURS_NULL_HANDLING` goes from 59 to 51 entries. Nothing gained the option and nothing lost it — the eight removed were never honouring it, and the set now agrees with the contract. The Javadoc records both bounds on what the set proves: it covers only what the harness can drive, and it compares rendered answers for the reason above. ## Why this comes before the rest of the scope Every family still to conform under #19218 is a sketch family, and their answers are exactly the serialized wrappers this defect blinds the test to. Conforming them against a vacuous check would prove nothing. -- 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]
