findepi commented on code in PR #17165:
URL: https://github.com/apache/datafusion/pull/17165#discussion_r2272809054
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -6162,6 +6162,28 @@ from t;
----
a,c,d,b
+query TT
+explain select string_agg(k, ',' order by v) from t;
+----
+logical_plan
+01)Aggregate: groupBy=[[]], aggr=[[string_agg(t.k, Utf8(",")) ORDER BY [t.v
ASC NULLS LAST]]]
+02)--TableScan: t projection=[k, v]
+physical_plan
+01)AggregateExec: mode=Single, gby=[], aggr=[string_agg(t.k,Utf8(",")) ORDER
BY [t.v ASC NULLS LAST]]
+02)--SortExec: expr=[v@1 ASC NULLS LAST], preserve_partitioning=[false]
+03)----DataSourceExec: partitions=1, partition_sizes=[1]
+
+query TT
+explain select string_agg(k, ',' order by v desc) from t;
Review Comment:
Is single aggregation enough to exercise the `reverse_expr` API?
Or maybe we need two functions, one using reverse ordering of the other?
I checked that the test does not pass without changes in `string_agg.rs` and
passes with them.
This is likely related to
https://github.com/apache/datafusion/blob/e1a5cdf6db791ea50b4028d6eb11ffafd940b29b/datafusion/physical-plan/src/aggregates/mod.rs#L1201
call site, but it looks it should never matter when there is only one function.
What am I missing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
