STRING_AGG missing functionality #14412

gabotechs · 2025-02-02T17:27:38Z

Which issue does this PR close?

Closes Implement distinct and order by clause for string_agg aggregate function #8260.

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.

alamb · 2025-02-04T18:44:25Z

Close/reopen to rerun CI checks

datafusion/functions-aggregate/src/string_agg.rs

geoffreyclaude · 2025-02-19T16:56:02Z

datafusion/sqllogictest/test_files/aggregate.slt

 NULL

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


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

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

consistency always wins over correctness!

datafusion/sqllogictest/test_files/aggregate.slt

gabotechs · 2025-04-03T07:56:05Z

datafusion/functions-aggregate/src/string_agg.rs

 pub struct StringAgg {
    signature: Signature,
+    array_agg: ArrayAgg,
 }


I don't think I've seen this pattern much in DataFusion codebase about reusing another function as a building block. Any opinions about this approach?

seems good to me 👍

…nality

gabotechs · 2025-04-03T08:20:58Z

datafusion/functions-aggregate/src/string_agg.rs

+#[cfg(test)]
+mod tests {


I'm finding that having unit tests near the function definitions itself is significantly more ergonomic than writing sql logic tests for a couple of reasons:

We can tests the accumulators in isolation, allowing for finer grained control about batch updating, state generation, merging different states from different accumulators, etc...

The time it takes since a developer makes a code change, until the appropriate test is run is reduced significantly:

cargo test --lib string_agg::tests --manifest-path 1.40s user 1.53s system 132% cpu 2.204 total

VS

cargo test --test sqllogictests 33.61s user 7.17s system 365% cpu 11.164 total

Measured on a Mac M3

Not saying that we should not have sql logic tests, I think those are a must, but maybe having some testing tooling for folks to be able to contribute unit tests also here could improve DX.

I see that this is not an stablished pattern though, and I'm wondering what are people's take on this

In general I think that we should strive that sqllogictests cover all the "end user" visible behavior -- since they function as integration style test and make sure the functionality is all hooked up together correctly (not just working in isolation)

Unit tests / in module tests are good to cover cases that are hard to cover in sqllogictests

I think there is some more back story here: https://datafusion.apache.org/contributor-guide/testing.html#sqllogictests-tests

In terms of cycle times, sqllogictests do have the benefit you can update them without any code changes (so writing / updating them is sometimes faster than code), though you are right that testing code changes requires a recompilation

@logan-keede has been working on improving the build performance recently. Hopefully this will get better

Building project takes a *long* time (esp compilation time for datafusion core crate) #13814

🤔 it looks there's still some cards that can be played for making compilation times faster then. 👍 thanks for all that info!

alamb · 2025-04-04T10:40:51Z

@geoffreyclaude (who is helping review) when you think this PR is ready, can you please ping me and I'll give it a review?

geoffreyclaude

Looks pretty complete to me, you've covered all my initial suggestions!

geoffreyclaude · 2025-04-07T14:09:32Z

@alamb all good for me! You can give the final review.

alamb

Thank you @gabotechs and @geoffreyclaude -- I think this implementation is very nice, clean and well tested. 🏆

alamb · 2025-04-10T18:56:42Z

datafusion/functions-aggregate/src/string_agg.rs

 pub struct StringAgg {
    signature: Signature,
+    array_agg: ArrayAgg,
 }


seems good to me 👍

…nality (apache#14412)

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 mentioned this pull request Feb 2, 2025

Add support for DISTINCT + ORDER BY in ARRAY_AGG #14413

Merged

gabotechs force-pushed the string-agg-missing-functionality branch from 59ac6aa to fb56b81 Compare February 2, 2025 17:34

github-actions bot removed the logical-expr Logical plan and expressions label Feb 2, 2025

gabotechs force-pushed the string-agg-missing-functionality branch from fb56b81 to 808e417 Compare February 2, 2025 17:37

gabotechs mentioned this pull request Feb 2, 2025

array_agg cannot perform both distinct and order_by #12371

Closed

gabotechs changed the title ~~String agg missing functionality~~ STRING_AGG missing functionality Feb 3, 2025

alamb closed this Feb 4, 2025

alamb reopened this Feb 4, 2025

alamb closed this Feb 4, 2025

alamb reopened this Feb 4, 2025

gabotechs force-pushed the string-agg-missing-functionality branch from 808e417 to 4abf8a0 Compare February 15, 2025 12:34

This was referenced Feb 15, 2025

Add a STRING_AGG implementation based on ARRAY_AGG gabotechs/datafusion#1

Closed

Add missing STRING_AGG functionality #14682

Closed

STRING_AGG missing functionality gabotechs/datafusion#2

Closed

geoffreyclaude reviewed Feb 19, 2025

View reviewed changes

datafusion/functions-aggregate/src/string_agg.rs Outdated Show resolved Hide resolved

geoffreyclaude reviewed Feb 19, 2025

View reviewed changes

gabotechs force-pushed the string-agg-missing-functionality branch 2 times, most recently from b36a9c5 to c022671 Compare February 20, 2025 16:42

This was referenced Feb 24, 2025

Weekly Plan (Andrew Lamb) Feb 24, 2025 #14850

Closed

Weekly Plan (Andrew Lamb) March 3, 2025 #14978

Closed

gabotechs force-pushed the string-agg-missing-functionality branch 4 times, most recently from e204059 to f6be8e4 Compare April 3, 2025 07:55

github-actions bot added the documentation Improvements or additions to documentation label Apr 3, 2025

gabotechs commented Apr 3, 2025

View reviewed changes

Add a STRING_AGG implementation based on ARRAY_AGG for reusing funcio…

babc94b

…nality

gabotechs force-pushed the string-agg-missing-functionality branch from f6be8e4 to babc94b Compare April 3, 2025 08:07

gabotechs commented Apr 3, 2025

View reviewed changes

geoffreyclaude approved these changes Apr 7, 2025

View reviewed changes

alamb approved these changes Apr 10, 2025

View reviewed changes

gabotechs mentioned this pull request Apr 15, 2025

Release DataFusion 47.0.0 (April 2025) #15072

Closed

39 tasks

alamb merged commit cde8690 into apache:main Apr 15, 2025
29 checks passed

nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025

Add a STRING_AGG implementation based on ARRAY_AGG for reusing funcio…

71d5570

…nality (apache#14412)

		#[cfg(test)]
		mod tests {

STRING_AGG missing functionality #14412

STRING_AGG missing functionality #14412

Uh oh!

Conversation

gabotechs commented Feb 2, 2025 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Feb 4, 2025

Uh oh!

Uh oh!

geoffreyclaude Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

gabotechs Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffreyclaude Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabotechs Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

gabotechs Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

gabotechs Apr 4, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geoffreyclaude left a comment

Choose a reason for hiding this comment

Uh oh!

geoffreyclaude commented Apr 7, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Apr 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gabotechs commented Feb 2, 2025 •

edited by alamb

Loading

gabotechs Feb 20, 2025 •

edited

Loading

gabotechs Apr 3, 2025 •

edited

Loading

alamb commented Apr 4, 2025 •

edited

Loading