Skip to content
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

STRING_AGG missing functionality #14412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gabotechs
Copy link
Contributor

@gabotechs gabotechs commented Feb 2, 2025

Which issue does this PR close?

Rationale for this change

See #14413 first.

Complete the missing functionality of the STRING_AGG function.

What changes are included in this PR?

Adds support for DISTINCT and ORDER_BY clauses by reusing the existing ARRAY_AGG functionality and building the whole STRING_AGG aggregation function on top of it. This way, the full STRING_AGG functionality is automatically implemented [almost] for free.

The rationale for reusing the ARRAY_AGG functionality is because both functions are very similar, with just two minor diferences:

  • STRING_AGG works only with strings, while ARRAY_AGG works with any type.
  • The return of STRING_AGG is the same as ARRAY_AGG, but with the resulting array of strings joined by a delimiter.

In order to have the full STRING_AGG functionality, some small addition is also needed for the ARRAY_AGG function, as the current implementation is missing support for DISTINCT + ORDER BY. See #14413.

Are these changes tested?

Yes, both in unit tests and sqllogictests.

Are there any user-facing changes?

Users will be able to issue STRING_AGG calls with DISTINCT and ORDER BY clauses.

@github-actions github-actions bot added logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Feb 2, 2025
@gabotechs gabotechs force-pushed the string-agg-missing-functionality branch from 59ac6aa to fb56b81 Compare February 2, 2025 17:34
@github-actions github-actions bot removed the logical-expr Logical plan and expressions label Feb 2, 2025
@gabotechs gabotechs force-pushed the string-agg-missing-functionality branch from fb56b81 to 808e417 Compare February 2, 2025 17:37
@gabotechs gabotechs changed the title String agg missing functionality STRING_AGG missing functionality Feb 3, 2025
@alamb
Copy link
Contributor

alamb commented Feb 4, 2025

Close/reopen to rerun CI checks

@@ -5568,6 +5573,16 @@ SELECT STRING_AGG(x,',') FROM strings WHERE g > 100
----
NULL

query T
SELECT STRING_AGG(DISTINCT x,',') FROM strings WHERE g > 100
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing space between the x and ','.

Copy link
Contributor Author

@gabotechs gabotechs Feb 20, 2025

Choose a reason for hiding this comment

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

🤷‍♂️ It's the format that all the previous STRING_AGG tests were following, I kept it for consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

consistency always wins over correctness!

@gabotechs gabotechs force-pushed the string-agg-missing-functionality branch from 4abf8a0 to b36a9c5 Compare February 20, 2025 16:17
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.

Implement distinct and order by clause for string_agg aggregate function
3 participants