Add support for dictionary for approx_distinct - #24646
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24646 +/- ##
==========================================
- Coverage 81.44% 81.44% -0.01%
==========================================
Files 1118 1118
Lines 399560 399560
Branches 399560 399560
==========================================
- Hits 325424 325417 -7
- Misses 55137 55141 +4
- Partials 18999 19002 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @mkleen this looks good overall. I had two comments please check.
| | DataType::Map(_, _) | ||
| | DataType::Struct(_) | ||
| | DataType::Union(_, _) | ||
| | DataType::Dictionary(_, _) |
There was a problem hiding this comment.
Would it work to match on the value type instead, so a dictionary is supported exactly when its values would be? Something like DataType::Dictionary(_, value_type)
There was a problem hiding this comment.
Good point. Floats will be unsupported see #23084, So we should exclude them in all container types.
| | DataType::Map(_, _) | ||
| | DataType::Struct(_) | ||
| | DataType::Union(_, _) | ||
| | DataType::Dictionary(_, _) |
There was a problem hiding this comment.
would it make sense to have both dictionary arms on the value type, so Dictionary(_, value_type) is accepted only when is_hll_groups_type(value_type) is true?
3 participants
Which issue does this PR close?
approx_distinctfunction #22989 but does not close it. More types are coming.Rationale for this change
Dictionaryforapprox_distinctWhat changes are included in this PR?
HLLAccumulatorandHllGroupsAccumulatorto supportDictionaryAre these changes tested?
Yes
Are there any user-facing changes?
Yes,
approx_distinctsupports nowDictionarybut no breaking changes.