fix: Implement AggregateUDFImpl::reverse_expr for StringAgg

[nuno-faria]

Which issue does this PR close?

• Related string_agg does not respect ORDER BY on 49.0.0 #17011.
• Context [branch-49] fix: string_agg not respecting ORDER BY #17058 (comment), [branch-49] fix: string_agg not respecting ORDER BY #17058 (comment).

Rationale for this change

The SortExec is missing from the physical plan of string_agg functions. In the current branch it works since the AggregateExec ends up sorting the results, but on 49.0.0 its broken. This PR helps avoid future regressions by implementing AggregateUDFImpl::reverse_expr for StringAgg, which brings back the SortExec.

What changes are included in this PR?

• Implemented AggregateUDFImpl::reverse_expr for StringAgg.
• Added sqllogictests to check the plans.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

[findepi]

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

datafusion/datafusion/physical-plan/src/aggregates/mod.rs

Line 1201 in e1a5cdf

if let Some(reverse_aggr_expr) = aggr_expr.reverse_expr() {

call site, but it looks it should never matter when there is only one function.

What am I missing?

[nuno-faria]

Without reverse_expr the requirement in that function will stay None, which means get_finer_aggregate_exprs_requirement returns an empty list. Maybe @berkaysynnada can clarify the behavior.

[findepi]

Are we hitting this loop exit path as expected?

datafusion/datafusion/physical-plan/src/aggregates/mod.rs

Lines 1188 to 1195 in e1a5cdf

	// If the common requirement is finer than the current expression's,
	// we can skip this expression. If the latter is finer than the former,
	// adopt it if it is satisfied by the equivalence properties. Otherwise,
	// defer the analysis to the reverse expression.
	let forward_finer = determine_finer(&requirement, &aggr_req);
	if let Some(finer) = forward_finer {
	if !finer {
	continue;

[nuno-faria]

Doesn't look like it, determine_finer returns true. Should it exit there? If it did I think we would have the same problem, i.e., requirement would remain None.

[findepi]

This LGTM but i clearly don't understand fully how this works.
In my understanding the reverse_expr should by no means be needed, nor used, if there is only one aggregate function in a query. Either I am wrong, or there is some other problem remaining.

[alamb]

I added a test using string_agg with both orderings in 84d58ac and it looks good to me

Thank you @nuno-faria and @findepi

[alamb]

I believe this should be included in DataFusion 50 as well so that we don't have a regression after DF 49. I will merge it now and make a backport