nuno-faria commented on PR #17058: URL: https://github.com/apache/datafusion/pull/17058#issuecomment-3182742707
> 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. -- 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
