Vamsi-klu commented on PR #18841: URL: https://github.com/apache/pinot/pull/18841#issuecomment-4912316579
Traced the root cause and the fix looks correct. checkAndConvertToSketchForGroups (called from both aggregateGroupBySV and aggregateGroupByMV) does setValueForKey(groupKey, convertToSketch(dictIdsWrapper)) once a group's dict-id cardinality exceeds getDictIdCardinalityThreshold(), so the group holder can legitimately hold a sketch rather than a DictIdsWrapper. The old `return (Set) result` cast then CCE'd on it. Widening the return to Object and returning result directly mirrors extractAggregationResult (whose else-branch already returns the raw sketch), and since the non-grouped path already emits sketches, downstream merge/serialization handles the type fine. Good that this sits in the shared base, so it also covers the sibling subclasses (SmartHLLPlus and SmartULL), not just SmartHLL. One note on the test: it plants a HyperLogLog directly via setValueForKey rather than driving aggregation past the threshold, so it pins the changed line (asserts extracted instanceof HyperLogLog and not a Set) but doesn't exercise checkAndConvertToSketchForGroups end-to-end. It's a valid regression test, but an aggregation-driven case that pushes a group over dictThreshold would guard the whole conversion->extraction path against future regressions. -- 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]
