yashrb24 commented on PR #25358:
URL: https://github.com/apache/datafusion/pull/25358#issuecomment-6814253295
> Are there particular query patterns that would signal to me that I should
turn it on?
I intended this to help queries where we have an ORDER BY on a function that
does some (possibly expensive) computation over an input column(s). So patterns
like:
```sql
ORDER BY lower(col1), lower(col2), lower(col3), …
ORDER BY col1, col2, lower(col3), …
ORDER BY col1, lower(col2), lower(col3), …
```
etc. should find this useful especially when the earlier keys resolve the
ordering for most rows. Ofc this is also dependent on the data distribution
(specifically how many ties we have on the earlier keys)
I think there’s still some gotchas even after enabling this conservatively
1. The idea was that in the general case you don’t need to evaluate all the
keys for all the rows. For the rows where we do need to evaluate the later
keys, there will be some overhead which should get amortized by skipping their
evaluation for the remaining rows.
2. If the data distribution is such that we need to evaluate most of the
keys for most rows, then doing this staging will add overhead and we might have
been better off with the existing approach.
* On @kosiew’s suggestion, I’ve added a fallback to evaluate the
remaining keys together for the unresolved rows when we detect that further
staging is not useful (this needs more work).
* @alamb - your point about checking whether the further keys are needed
for every row (here
https://github.com/apache/datafusion/issues/25033#issuecomment-6745369469)
should also be addressed to an extent with this fallback. Ofc we can still
vectorize the expression evaluation over the gathered rows, but the overhead
from gathering rows and sorting individual tie groups may not always get
amortized by the skipped work.
Thanks to @kosiew, who has already identified from his microbenchmarks a
concrete bunch of cases where this is useful (as the author I should have
provided some microbenchmarks ideally to make my point :sweat_smile:).
> I don’t think we should be adding any experimental default-off features
unless there is a clear path to enabling it by default for everyone
I agree, and in #25033 I highlighted that this would mean considerable
changes in the mechanics of how sort works, and therefore was looking for some
feedback on getting this working without regressions.
Though if the consensus is that the current approach is not the way to do
it, or this optimization is not generally useful (tradeoffs considered) to have
here, I’ll be happy to try out a new approach or close this PR.
--
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]
