kosiew commented on code in PR #23599:
URL: https://github.com/apache/datafusion/pull/23599#discussion_r3712323598
##########
datafusion/physical-optimizer/src/window_topn.rs:
##########
@@ -195,13 +200,41 @@ impl WindowTopN {
.ok()?;
// Step 9: If ProjectionExec was between Filter and Window, rebuild it
- let result = match proj_between {
+ let mut result = match proj_between {
Some(proj) => Arc::clone(&child_as_arc(proj))
.with_new_children(vec![new_window])
.ok()?,
None => new_window,
};
+ // Step 10: Re-apply the FilterExec's embedded projection (if any)
+ // as an outer ProjectionExec. The projection indices refer to
+ // columns in `filter.input().schema()`, which equals `result`'s
+ // schema at this point (Steps 8-9 preserve schema), so the
+ // indices remain valid.
+ if let Some(indices) = filter_projection {
Review Comment:
Nice improvement capturing and restoring the embedded projection.
One thing I noticed is that the rewrite removes the `FilterExec` but does
not preserve its `fetch`. `FilterExecBuilder` supports both an embedded
projection and `with_fetch`, and `FilterExec::execute` applies the fetch after
evaluating the predicate.
For example, if a matching projected filter has `fetch=1`, this rewrite
currently produces only the outer `ProjectionExec` over the rewritten window.
That returns all `rn <= K` rows instead of just one.
Could we either preserve the fetch with an equivalent outer limit/fetch
operator, or skip this rewrite when `filter.fetch().is_some()`?
It would also be great to add a regression test covering the projection plus
fetch case that executes the plan, or otherwise verifies that the row limit is
preserved.
--
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]
