zhuqi-lucas commented on code in PR #23599:
URL: https://github.com/apache/datafusion/pull/23599#discussion_r4178112753
##########
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:
Good catch — fixed in f6bd9ddd0d, and it turned out to be wider than the
projection path.
I went with the second option: `try_transform` now declines when
`filter.fetch().is_some()`. Preserving the fetch with an outer limit is not a
straight substitution — `PartitionedTopKExec` bounds rows *per partition*,
while `FilterExec::fetch` is a limit over the filtered output, so reproducing
it would mean adding a real limit operator and reasoning about where it sits
relative to the window. Declining keeps the rewrite honest and loses only the
narrow intersection of "embedded projection *and* fetch".
Worth flagging: this was not introduced here. `try_transform` on `main`
never reads `fetch` either — it just happens to bail on
`projection().is_some()` first, so a FilterExec with a fetch and no projection
was already being rewritten with its fetch dropped. The guard sits at the entry
and is unconditional, so it covers that case too.
Two regression tests, both asserting the plan comes back unchanged:
`filter_with_projection_and_fetch_is_declined` and
`filter_with_fetch_and_no_projection_is_declined` (the second is the
pre-existing case).
Also rebased onto main and resolved the conflicts — `find_window_below` now
returns a generic `intermediates` list rather than a single `proj_between`, so
the rewrite composes with that. One existing snapshot needed updating: the
`SortExec` under the window now stays in place, which the old expectation
predated.
--
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]
