Uh oh!
There was an error while loading. Please reload this page.
[#21878] extensive test for multi-dictionary column group bys - #22888
[#21878] extensive test for multi-dictionary column group bys#22888Rich-T-kid wants to merge 3 commits into
Conversation
777a7d5 to
a6c003cCompareRich-T-kid
commented
Jun 10, 2026
I tried to make each test function as declarative as possible. |
Rich-T-kid
commented
Jun 18, 2026
@2010YOUY01 I know you have a lot of experience with aggregation operator in datafusion . Do you mind taking a look at this smaller PR, when you get a chance? |
2010YOUY01
commented
Jun 18, 2026
I don't have enough context on the dictionary type support right now, but this is pretty cool. I hope to find some time to look into it later. Here is a bit of context I can share: I assume we would need to specialize a TL;DR: today, |
Rich-T-kid
commented
Jun 18, 2026
Thx, I'll defiantly take a look at the issue when I get a chance! |
Rich-T-kid
commented
Jun 23, 2026
I do think these test still provide value, @kumarUjjawal what are your thoughts? Should I close this PR as well? |
In my opinion since you are going to be working on more centralised dictionaries implementation, these tests should wait till we have the structure or you can port these to the new prs. |
Which issue does this PR close?
works towards closing #21878. But also adds test coverage for existing code that wasnt there before. There were no test for dictionaries specifically for groupValueRows
Rationale for this change
There was a lack of test that focused on dictionary arrays in
GroupValueRows. I also would like to keep the test separate from the implementation (for #21878) so that its easier to review.What changes are included in this PR?
GroupValuesRowspath(Dict<Int8,Utf8>, Dict<Int32,LargeUtf8>, Dict<Int8,Utf8View>). these are the first SLT tests where the grouping keys themselves are dictionary columnsAre these changes tested?
The changes are test 😆
Are there any user-facing changes?
No