Skip to content

feat: support ApproxDistinct with utf8view #15200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 14, 2025

Conversation

zhuqi-lucas
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

support ApproxDistinct with utf8view

What changes are included in this PR?

support ApproxDistinct with utf8view

Are these changes tested?

Yes

Are there any user-facing changes?

support ApproxDistinct with utf8view

@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Mar 13, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @zhuqi-lucas -- looks good to me

query III
SELECT c2, approx_distinct(c1), approx_distinct(c5) FROM aggregate_test_100_utf8view GROUP BY c2 ORDER BY c2;
----
1 5 22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is the same as above 💯

Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zhuqi-lucas , LGTM. I left some comments for you to consider.

where
T: OffsetSizeTrait,
{
/// new approx_distinct accumulator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you follow the previous pattern, but the comment seems unclear vague

Suggested change
/// new approx_distinct accumulator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @Weijun-H , addressed in latest PR.

c5
FROM aggregate_test_100;

# Test approx_distinct for varchar / int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Test approx_distinct for varchar / int
# Test approx_distinct for varchar(with Utf8View) / int

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Weijun-H , addressed in latest PR.

@zhuqi-lucas
Copy link
Contributor Author

Thank you @alamb and @Weijun-H for review!

@Weijun-H Weijun-H merged commit 8061485 into apache:main Mar 14, 2025
26 checks passed
@Weijun-H
Copy link
Member

Thanks @zhuqi-lucas and @alamb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions Changes to functions implementation sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants