feat: topk functionality for aggregates should support utf8view and largeutf8#15152
feat: topk functionality for aggregates should support utf8view and largeutf8#15152alamb merged 6 commits intoapache:mainfrom
Conversation
|
I will add sqllogictesting after the config PR merged: |
alamb
left a comment
There was a problem hiding this comment.
Thanks @zhuqi-lucas -- looks good to me
I'll wait for SQL test coverage before hitting approve
Any chance you are willing to add LargeUtf8` support while you are at it?
Thank you @alamb for review, i am willing to add |
Thank you @alamb, enabled the slt testing for utf8view and also add LargeUtf8 support in latest PR. |
alamb
left a comment
There was a problem hiding this comment.
Thanks @zhuqi-lucas -- I just have one more test request
| ####### | ||
|
|
||
| # Setting to map varchar to utf8view, to test PR https://github.com/apache/datafusion/pull/15152 | ||
| # Before the PR, the test case would not work because the Utf8View will not be supported by the TopK aggregation |
There was a problem hiding this comment.
this change now loses coverage for utf8
Instead perhaps we can make a table with stringview?
Something like
CREATE TABLE traces_utf8view
AS SELECT
arrow_cast(trace_id, 'Utf8View') as trace_id,
timestamp,
other
)And then do an explain
explain select trace_id, MAX(timestamp) from traces group by trace_id order by MAX(timestamp) desc limit 4;There was a problem hiding this comment.
Also it would be great to do the same fir LargeUtf8
Thank you @alamb for review, addressed it in latest PR! |
alamb
left a comment
There was a problem hiding this comment.
Thank you for your patience and perseverence @zhuqi-lucas
|
FYI @avantgardnerio |
|
Thanks again @zhuqi-lucas |
Which issue does this PR close?
#15096
Rationale for this change
topk functionality for aggregates should support utf8view
What changes are included in this PR?
topk functionality for aggregates should support utf8view
Are these changes tested?
Yes
Are there any user-facing changes?
topk functionality for aggregates should support utf8view