Repository navigation
Conversation
JackieTien97
left a comment
JackieTien97
left a comment
There was a problem hiding this comment.
One correctness regression remains in the grouped BLOB aggregation path; please address the inline comment.
| if (spaceSaving == null || spaceSaving.isEmpty()) { | ||
| columnBuilder.appendNull(); | ||
| return; |
There was a problem hiding this comment.
[P2] Handle empty BLOB groups in the overridden final evaluator
Please also add the null/empty-state guard to BlobGroupedApproxMostFrequentAccumulator.evaluateFinal(). For a group whose BLOB values are all NULL, this inherited evaluateIntermediate() now emits NULL, and BinaryGroupedApproxMostFrequentAccumulator.addIntermediate() skips it, leaving the final group's state uninitialized. The BLOB-specific evaluateFinal() overrides the guarded base implementation and still calls state.getSpaceSavings().get(groupId).getBuckets() unconditionally, so a PARTIAL -> FINAL aggregation now throws an NPE. I reproduced this with the parent and PR accumulator sources: the parent returns {}, whereas this revision throws. Please add a grouped BLOB regression test covering an all-NULL group through partial/final aggregation, including alongside a nonempty group.
Fix NPE of approx_most_frequent and approx_count_distinct on empty input