Skip to content

Fix SmartHLL ClassCastException in GROUP BY ORDER BY#18841

Merged
Jackie-Jiang merged 3 commits into
apache:masterfrom
ilamhs:fix/smart-hll-group-by-classcast
Jul 9, 2026
Merged

Fix SmartHLL ClassCastException in GROUP BY ORDER BY#18841
Jackie-Jiang merged 3 commits into
apache:masterfrom
ilamhs:fix/smart-hll-group-by-classcast

Conversation

@ilamhs

@ilamhs ilamhs commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

extractGroupByResult() blindly cast to Set but the holder can contain a HyperLogLog once the dict-ID cardinality threshold is exceeded via checkAndConvertToSketchForGroups. Mirror the threshold check already present in extractAggregationResult().

Labels: bugfix

extractGroupByResult() blindly cast to Set but the holder can contain a
HyperLogLog once the dict-ID cardinality threshold is exceeded via
checkAndConvertToSketchForGroups. Mirror the threshold check already present
in extractAggregationResult().
@Jackie-Jiang Jackie-Jiang added bug Something is not working as expected query Related to query processing labels Jun 23, 2026
@codecov-commenter

codecov-commenter commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.92%. Comparing base (63bf8dc) to head (63e033e).
⚠️ Report is 81 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18841      +/-   ##
============================================
+ Coverage     64.76%   64.92%   +0.15%     
- Complexity     1322     1347      +25     
============================================
  Files          3393     3395       +2     
  Lines        211022   212533    +1511     
  Branches      33135    33515     +380     
============================================
+ Hits         136676   137992    +1316     
- Misses        63330    63389      +59     
- Partials      11016    11152     +136     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 64.92% <100.00%> (+0.15%) ⬆️
temurin 64.92% <100.00%> (+0.15%) ⬆️
unittests 64.92% <100.00%> (+0.15%) ⬆️
unittests1 56.95% <100.00%> (-0.03%) ⬇️
unittests2 37.24% <0.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

checkAndConvertToSketchForGroups already converts DictIdsWrapper to sketch
eagerly when the threshold is exceeded, so any DictIdsWrapper remaining in
the holder at extract time is guaranteed to be below threshold. The cardinality
guard added in the prior commit is dead code; remove it per reviewer feedback.
@Vamsi-klu

Copy link
Copy Markdown
Contributor

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.

@Jackie-Jiang
Jackie-Jiang merged commit f29b359 into apache:master Jul 9, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected query Related to query processing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants