Repository navigation
string_agg does not respect ORDER BY on 49.0.0 #17011
Description
Describe the bug
The string_agg function does not respect the ORDER BY clause in the 49.0.0 release. It does work after e1a5cdf I believe (#16625).
I discovered this while attempting to upgrade datafusion-python to datafusion 49.0.0, which causes some tests to fail. Would it be possible to release a minor update to allow datafusion-python to support datafusion 49?
To Reproduce
On the released 49.0.0 version:
> create table t (k varchar, v int);
0 row(s) fetched.
Elapsed 0.055 seconds.
> insert into t values ('a', 2), ('b', 3), ('c', 1);
+-------+
| count |
+-------+
| 3 |
+-------+
1 row(s) fetched.
Elapsed 0.065 seconds.
> select string_agg(k, ',' order by v) from t;
+---------------------------------------------------------+
| string_agg(t.k,Utf8(",")) ORDER BY [t.v ASC NULLS LAST] |
+---------------------------------------------------------+
| a,b,c |
+---------------------------------------------------------+
1 row(s) fetched.Expected behavior
Should return:
+---------------------------------------------------------+
| string_agg(t.k,Utf8(",")) ORDER BY [t.v ASC NULLS LAST] |
+---------------------------------------------------------+
| c,a,b |
+---------------------------------------------------------+
Additional context
No response
Activity
findepi commentedon Aug 4, 2025
Thanks for linking!
That PR focused on
array_aggand AFAIR added tests only for that funciton.Would you be willing to contribute tests for
string_aggas well, to make sure it does not regress in future releases?nuno-faria commentedon Aug 4, 2025
@findepi Good idea, I have submitted a PR that adds logic tests for
string_aggwithORDER BY.About the bug in
49.0.0, is a49.0.1release with the fix feasible?findepi commentedon Aug 4, 2025
Thanks, approved!
Technically speaking this issue is a feature missing in 49.0.0 that already present on the main branch.
It will be released in 50.0.0, you can track it here: #16799
Releasing 49.0.1 is a good idea if we have bug fixes, especially for regressions found out after the release.
I feel your frustration -- the feature already exists, but it may be still a month or two until it is released and usable downstream. For that reason I am in favor of more frequent (and more automated!) releases, even every week. You can see prior discussion and chime in
findepi commentedon Aug 4, 2025
BTW recently there was a x.0.1 release:
48.0.1#16486it looks it was in "fix release regressions" category
nuno-faria commentedon Aug 4, 2025
Thanks for the feedback. Since the
string_agg+ORDER BYworks with version48.0.0and below, I believe it could also fall under the "fix regressions" category.My main need for the patch would be to upgrade
datafusion-pythonto version 49, otherwise it must wait until version 50 (or remove the failing tests).alamb commentedon Aug 4, 2025
Indeed, if this feature used to work and now does not I think it is a regression and we should fix it as a patch release
There were a few other items that people have suggested for inclusion. Here is a ticket to track a possible
49.0.1release:49.0.1(patch) #17036alamb commentedon Aug 4, 2025
Sounds like we should have tried harder to test datafusion-python with the 49.0.0 release candidate -- I think it just fell through the cracks
Thank you for helping drive this @nuno-faria
findepi commentedon Aug 4, 2025
i didn't know it did. Then it's a correctness regression warranting a backport release.
I confirmed the following SQL returns correct results in 48.0.0 and incorrect result in 49.0.0 (lack of sorting)
i got biased by the "It does work after e1a5cdf" mention, which was supposed to be a new feature work, not a regression fix.
nuno-faria commentedon Aug 5, 2025
No problem, I should have rephrased that better.
Meanwhile, it appears that #16625 adds breaking changes, so it might not be simple to just cherry pick it to
49.0.1. We might need to patch a new fix directly. I think the bug has been present since #16217.nuno-faria commentedon Aug 6, 2025
I backported the fix here: #17058.
I don't think the entire PR #16625 can be backported due to breaking changes.
LiaCastaneda commentedon Aug 12, 2025
👋 We’re seeing what might be a related error after upgrading to DF v49:
SanityCheckPlan caused by Error during planning: Plan: ... does not satisfy order requirements ...Just wanted to check if this could be related - in our case, the query doesn’t just return incorrect results, but it fails with the error. I can share a reproducer later & open a different issue if that’s helpful 🙇♀️ . I tried using @nuno-faria’s PR to see if it fixed the issue, but it seems to fail in the same way.alamb commentedon Aug 12, 2025
this sounds like a different issue -- a new ticket / reproducer would be most helpful. Thank you @LiaCastaneda
LiaCastaneda commentedon Aug 14, 2025
We just verified it gets fixed in #17129 👍
alamb commentedon Aug 14, 2025
Note we (well really @AdamGS ) also backported the fix to 49.0.1: #17143
So it should be fixed in that release as well
LiaCastaneda commentedon Aug 14, 2025
Thank you 🙇♀️
alamb commentedon Aug 18, 2025