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 on reachability: `WindowTopN` runs before `LimitPushdown`, so
in the built-in pipeline the filter always has `fetch: None` — an outer `LIMIT`
lands as its own `GlobalLimitExec` (new `PROJ4` test). The guard is defensive
rather than a live-bug fix. It isn't specific to the projection path either —
`try_transform` on `main` never reads `fetch`, it just bails on
`projection().is_some()` first.
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]
