[branch-49] fix: string_agg not respecting ORDER BY #17058

nuno-faria · 2025-08-06T15:52:18Z

Which issue does this PR close?

Closes string_agg does not respect ORDER BY on 49.0.0 #17011.
Part of Release DataFusion 49.0.1 (patch) #17036

Rationale for this change

There was a regression in 49.0.0 in the string_agg function, causing it to not consider the ORDER BY clauses.

What changes are included in this PR?

Backported part of Support multiple ordered array_agg aggregations #16625 to version 49 (I don't think the entire PR can be backported since it appears to have breaking changes).
Added sqllogictests.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

datafusion/functions-aggregate/src/array_agg.rs

findepi · 2025-08-07T09:45:58Z

This PR adds new feature code.
Would be good to understand the source of regression #17011.
Is there a smaller & safer fix we could make in the 49 maintenance branch?

nuno-faria · 2025-08-07T09:56:01Z

Would be good to understand the source of regression #17011.

The issue comes from #16217, however I am not sure of the exact cause as that PR is very large.

Is there a smaller & safer fix we could make in the 49 maintenance branch?

I had found that the minimum/easiest fix would be to add the sort method and self.sort() in the execute method, but decided to backport is_input_pre_ordered just in case.

The `ArrayAgg` struct is stateful, therefore it must implement `AggregateUDFImpl::equals` and `hash_value` functions.

findepi · 2025-08-08T09:45:16Z

The issue comes from #16217, however I am not sure of the exact cause as that PR is very large.

I don't know either and the naive idea to revert that change on the release branch sounds much much less safe than adding a new but small feature work in lie of a bug fix.

I am therefore fine with this backport in current shape. @nuno-faria can you please make sure the build is green?

nuno-faria · 2025-08-08T10:10:13Z

I am therefore fine with this backport in current shape. @nuno-faria can you please make sure the build is green?

@findepi The build error comes from the cherry-picked #17065, since udf_equals_hash! does not exist in version 49. Should we backport udf_equals_hash! to 49.0.1 or cancel cherry picking #17065?

findepi · 2025-08-08T10:33:15Z

I think it's best to replace usage of udf_equals_hash with a direct implementation of equals and hash_value, delegating to Eq and Hash that are derived on the struct.

…or ArrayAgg

berkaysynnada

The fix looks good to me, thank you @nuno-faria, but it doesn’t fully address the change introduced in #16217.

I think you can focus on the changes in AggregateExec::try_new_with_schema() from #16217, because that PR modifies the planning results, and you are reacting to it by adapting the accumulator behavior (but I’m not sure if this approach causes any issues or actually improves the situation)

As I said, anyone can debug the point where the order requirements are calculated in AggregateExec::try_new_with_schema(), comparing the behavior before and after #16217, to see the real change that triggers this issue

--

before that PR, a SortExec emerges just before AggregateExec, and sends the sorted data. That SortExec is lost now

nuno-faria · 2025-08-09T17:10:38Z

Thanks @berkaysynnada for the pointers. I did a little more digging and found that the get_finer_aggregate_exprs_requirement function returns a different value for string_agg and array_agg. The source of the difference is that AggregateUDFImpl::reverse_expr is not defined for string_agg. This means the SortExec will not be created for string_agg, and that is still true in the main branch of datafusion.

# string_agg
| physical_plan after CombinePartialFinalAggregate           | OutputRequirementExec: order_by=[], dist_by=Unspecified
|                                                            |   AggregateExec: mode=Single, gby=[], aggr=[string_agg(t.k,Utf8(",")) ORDER BY [t.v ASC NULLS LAST]]
|                                                            |     DataSourceExec: partitions=1, partition_sizes=[1]
|                                                            |
| physical_plan after EnforceSorting                         | SAME TEXT AS ABOVE


# array_agg
| physical_plan after CombinePartialFinalAggregate           | OutputRequirementExec: order_by=[], dist_by=Unspecified
|                                                            |   AggregateExec: mode=Single, gby=[], aggr=[array_agg(t.k) ORDER BY [t.v ASC NULLS LAST]]
|                                                            |     DataSourceExec: partitions=1, partition_sizes=[1]
|                                                            |
| physical_plan after EnforceSorting                         | OutputRequirementExec: order_by=[], dist_by=Unspecified
|                                                            |   AggregateExec: mode=Single, gby=[], aggr=[array_agg(t.k) ORDER BY [t.v ASC NULLS LAST]]
|                                                            |     SortExec: expr=[v@1 ASC NULLS LAST], preserve_partitioning=[false]
|                                                            |       DataSourceExec: partitions=1, partition_sizes=[1]

The fix is to simply implement AggregateUDFImpl::reverse_expr for StringAgg. @alamb @findepi what do you think of this approach? With it there is no need cherry pick a new feature into 49.0.1.

berkaysynnada · 2025-08-10T12:07:09Z

The fix is to simply implement AggregateUDFImpl::reverse_expr for StringAgg

I prefer this option. There’s no need to overwhelm operators if there wouldn’t be any benefit.

alamb · 2025-08-12T10:38:06Z

I am hoping we can make a release candidate with this fix in it today or tomorrow. Are we waiting on a PR for an alternate approach "The fix is to simply implement AggregateUDFImpl::reverse_expr for StringAgg" ?

Release DataFusion 49.0.1 (patch) #17036

nuno-faria · 2025-08-12T10:51:01Z

I am hoping we can make a release candidate with this fix in it today or tomorrow. Are we waiting on a PR for an alternate approach "The fix is to simply implement AggregateUDFImpl::reverse_expr for StringAgg" ?
* [Release DataFusion `49.0.1` (patch) #17036](https://github.com/apache/datafusion/issues/17036)

I will push the alternative solution then if no one disagrees.

nuno-faria · 2025-08-12T11:24:37Z

Fix pushed. It might also be worth thinking about pushing this fix to main, since currently the SortExec is also missing there for string_agg:

> create table t (k varchar, v int);
0 row(s) fetched.
Elapsed 0.005 seconds.

> insert into t values ('a', 2), ('b', 3), ('c', 1);
+-------+
| count |
+-------+
| 3     |
+-------+
1 row(s) fetched.
Elapsed 0.011 seconds.

> explain select string_agg(k, ',' order by v) from t;
+---------------+-------------------------------+
| plan_type     | plan                          |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
|               | │       AggregateExec       │ |
|               | │    --------------------   │ |
|               | │           aggr:           │ |
|               | │  string_agg(t.k, ,) ORDER │ |
|               | │   BY [t.v ASC NULLS LAST] │ |
|               | │                           │ |
|               | │        mode: Single       │ |
|               | └─────────────┬─────────────┘ |
|               | ┌─────────────┴─────────────┐ |
|               | │       DataSourceExec      │ |
|               | │    --------------------   │ |
|               | │         bytes: 296        │ |
|               | │       format: memory      │ |
|               | │          rows: 1          │ |
|               | └───────────────────────────┘ |
|               |                               |
+---------------+-------------------------------+
1 row(s) fetched.
Elapsed 0.010 seconds.

> explain select array_agg(k order by v) from t;
+---------------+-------------------------------+
| plan_type     | plan                          |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
|               | │       AggregateExec       │ |
|               | │    --------------------   │ |
|               | │            aggr           │ |
|               | │                           │ |
|               | │        mode: Single       │ |
|               | └─────────────┬─────────────┘ |
|               | ┌─────────────┴─────────────┐ |
|               | │          SortExec         │ |
|               | │    --------------------   │ |
|               | │     v@1 ASC NULLS LAST    │ |
|               | └─────────────┬─────────────┘ |
|               | ┌─────────────┴─────────────┐ |
|               | │       DataSourceExec      │ |
|               | │    --------------------   │ |
|               | │         bytes: 296        │ |
|               | │       format: memory      │ |
|               | │          rows: 1          │ |
|               | └───────────────────────────┘ |
|               |                               |
+---------------+-------------------------------+

findepi · 2025-08-12T11:29:19Z

Fix pushed. It might also be worth thinking about pushing this fix to main, since currently the SortExec is also missing there for string_agg:

I don't think it's imperative to implement the reverse_expr for string_agg on the maintenance branch. Is it required here, @berkaysynnada ?
I would feel more comfortable approving a smaller PR, especially when it targets a maintenance branch.

I am all for improving the state of things on main. I would prefer improvements such as adding reverse_expr for string_agg are reviewed and merged on main first, and then backported to maintenance branch.

berkaysynnada · 2025-08-12T11:47:06Z

@findepi I think it's fine to fix 49.0 with this patch, as it is indeed a bug fix rather than an improvement (if anyone requests it to be fixed). Besides, this should be fixed on main as well in any case.

@nuno-faria thank you for spotting this issue. I also recommend adding another slt test to assert the planning result. Otherwise, it’s good to go (after deciding where to land)

BTW, how does #16217 affect this? Were you able to see that those queries used array_agg before, and after that PR they now use string_agg? I’m a bit confused about that.

nuno-faria · 2025-08-13T08:30:09Z

I would feel more comfortable approving a smaller PR, especially when it targets a maintenance branch.

The new fix makes the PR smaller, since it only implements AggregateUDFImpl::reverse_expr for StringAgg.

I also recommend adding another slt test to assert the planning result. Otherwise, it’s good to go (after deciding where to land)

Updated. I think the fix should be good to go.

BTW, how does #16217 affect this? Were you able to see that those queries used array_agg before, and after that PR they now use string_agg? I’m a bit confused about that.

They still used string_agg, but the SortExec was dropped after #16217. I just used array_agg as a comparison since the order on that function still worked after #16217.

findepi · 2025-08-13T09:16:00Z

datafusion/functions-aggregate/src/string_agg.rs

        )))
    }

+    fn reverse_expr(&self) -> datafusion_expr::ReversedUDAF {


Can you please add this on main first?
This would make it impossible that anyone updating from 49.0.1 to a newer DF version experiences a behavioral regression.

PR open: #17165

findepi

Other than https://github.com/apache/datafusion/pull/17058/files#r2272648661, this LGTM

alamb · 2025-08-13T19:04:25Z

Other than https://github.com/apache/datafusion/pull/17058/files#r2272648661, this LGTM

@findepi is it ok with you to merge this PR now, or should we wait for the forward port to main to merge first?

fix: Implement AggregateUDFImpl::reverse_expr for StringAgg #17165

findepi · 2025-08-13T20:32:08Z

if there is urgency to merge & release 49.0.1, i am fine
if there isn't, I'd like to merge to main first.

alamb · 2025-08-14T11:29:56Z

if there is urgency to merge & release 49.0.1, i am fine if there isn't, I'd like to merge to main first.

I would like to get the 49.0.1 release candidate (as there is then still a 3 day voting period before we can release) so I will merge this one in and follow along on #17165 and try and help as possible

alamb · 2025-08-14T12:16:41Z

Thanks again @nuno-faria @findepi and @berkaysynnada for getting this done for the 49.0.1 release

* fix: string_agg not respecting ORDER BY * Fix equality of parametrizable ArrayAgg function (apache#17065) The `ArrayAgg` struct is stateful, therefore it must implement `AggregateUDFImpl::equals` and `hash_value` functions. * Implement AggregateUDFImpl::equals and AggregateUDFImpl::hash_value for ArrayAgg * Implement alternative fix * Remove 'use std::any::Any' * Add sqllogictest for string_agg plan * Revert as_any to their original implementations --------- Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> (cherry picked from commit f05b128)

* fix: string_agg not respecting ORDER BY * Fix equality of parametrizable ArrayAgg function (apache#17065) The `ArrayAgg` struct is stateful, therefore it must implement `AggregateUDFImpl::equals` and `hash_value` functions. * Implement AggregateUDFImpl::equals and AggregateUDFImpl::hash_value for ArrayAgg * Implement alternative fix * Remove 'use std::any::Any' * Add sqllogictest for string_agg plan * Revert as_any to their original implementations --------- (cherry picked from commit f05b128) Co-authored-by: Nuno Faria <nunofpfaria@gmail.com> Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

fix: string_agg not respecting ORDER BY

193b501

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Aug 6, 2025

nuno-faria mentioned this pull request Aug 6, 2025

string_agg does not respect ORDER BY on 49.0.0 #17011

Closed

findepi approved these changes Aug 7, 2025

View reviewed changes

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

alamb changed the title ~~fix: string_agg not respecting ORDER BY~~ [branch-49] fix: string_agg not respecting ORDER BY Aug 7, 2025

alamb mentioned this pull request Aug 7, 2025

[MAJOR] Equivalence System Overhaul #16217

Merged

Fix equality of parametrizable ArrayAgg function (apache#17065)

3009827

The `ArrayAgg` struct is stateful, therefore it must implement `AggregateUDFImpl::equals` and `hash_value` functions.

Implement AggregateUDFImpl::equals and AggregateUDFImpl::hash_value f…

401fffc

…or ArrayAgg

berkaysynnada reviewed Aug 8, 2025

View reviewed changes

alamb mentioned this pull request Aug 12, 2025

Release DataFusion 49.0.1 (patch) #17036

Closed

5 tasks

Implement alternative fix

f60436a

findepi requested a review from berkaysynnada August 12, 2025 11:26

nuno-faria added 3 commits August 13, 2025 09:20

Remove 'use std::any::Any'

7ce8808

Add sqllogictest for string_agg plan

7033812

Revert as_any to their original implementations

017aeb6

findepi reviewed Aug 13, 2025

View reviewed changes

findepi approved these changes Aug 13, 2025

View reviewed changes

nuno-faria mentioned this pull request Aug 13, 2025

fix: Implement AggregateUDFImpl::reverse_expr for StringAgg #17165

Merged

Merge branch 'branch-49' into fix_string_agg_regression

d1853e6

alamb merged commit f05b128 into apache:branch-49 Aug 14, 2025
27 checks passed

LiaCastaneda mentioned this pull request Aug 14, 2025

[branch-49] fix: string_agg not respecting ORDER BY (#17058) DataDog/datafusion#41

Merged

nuno-faria deleted the fix_string_agg_regression branch August 14, 2025 12:46

[branch-49] fix: string_agg not respecting ORDER BY #17058

[branch-49] fix: string_agg not respecting ORDER BY #17058

Uh oh!

Conversation

nuno-faria commented Aug 6, 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!

Uh oh!

findepi commented Aug 7, 2025

Uh oh!

nuno-faria commented Aug 7, 2025

Uh oh!

findepi commented Aug 8, 2025

Uh oh!

nuno-faria commented Aug 8, 2025

Uh oh!

findepi commented Aug 8, 2025

Uh oh!

berkaysynnada left a comment • 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!

nuno-faria commented Aug 9, 2025

Uh oh!

berkaysynnada commented Aug 10, 2025

Uh oh!

alamb commented Aug 12, 2025

Uh oh!

nuno-faria commented Aug 12, 2025

Uh oh!

nuno-faria commented Aug 12, 2025

Uh oh!

findepi commented Aug 12, 2025

Uh oh!

berkaysynnada commented Aug 12, 2025

Uh oh!

nuno-faria commented Aug 13, 2025

Uh oh!

findepi Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

nuno-faria Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 13, 2025

Uh oh!

findepi commented Aug 13, 2025

Uh oh!

alamb commented Aug 14, 2025

Uh oh!

Uh oh!

alamb commented Aug 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nuno-faria commented Aug 6, 2025 •

edited by alamb

Loading

berkaysynnada left a comment •

edited

Loading