lyne7-sc commented on PR #24508:
URL: https://github.com/apache/datafusion/pull/24508#issuecomment-5366423176
Thanks for the review! I think this could cause a performance regression
when the window argument is a direct column or another cheap expression. In
these cases, the cost of filtering the entire batch and scattering the result
back to the original row positions may exceed the expression evaluation cost
that we skip.
As a low-risk mitigation, we could bypass `evaluate_selection` for
expressions that are known to be safe and cheap to evaluate on the original
batch, as follows:
```
let value = if can_evaluate_window_arg_unfiltered(expr) {
// These expressions cannot fail because of values in excluded rows.
expr.evaluate(batch)?
} else {
// Fallible expressions must only see selected rows.
expr.evaluate_selection(batch, selection)?
};
```
This could address the most obvious regressions for direct columns and
literals. However, it would only be a temporary mitigation and would not cover
other cheap expressions such as `column + 1`.
One possible long-term direction might be to keep the selected argument
values in compact form instead of scattering them back. We could maintain a
prefix-count array that maps each boundary in the original row space to the
corresponding boundary in the compact argument arrays:
```
selection: T F T F
prefix: 0 1 1 2 2
original frame: [start, end)
compact frame: [prefix[start], prefix[end])
```
Window frame calculation and output rows could remain in the original row
space, while only the ranges passed to the accumulator would be mapped into the
compact arrays. This could allow us to skip evaluating expressions for excluded
rows while still producing the correct window results. It could also avoid
per-argument scatter and the additional filtering before
`update_batch/retract_batch`.
I'm not sure whether this approach would improve performance in practice.
does this direction make sense to you?
--
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]
