Copilot commented on code in PR #23599:
URL: https://github.com/apache/datafusion/pull/23599#discussion_r3585021210
##########
datafusion/physical-optimizer/src/window_topn.rs:
##########
@@ -195,13 +200,35 @@ 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 {
+ let input_schema = result.schema();
+ let projection_exprs: Vec<(Arc<dyn PhysicalExpr>, String)> =
indices
+ .iter()
+ .map(|&idx| {
+ let field = input_schema.field(idx);
+ (
+ Arc::new(Column::new(field.name(), idx))
+ as Arc<dyn PhysicalExpr>,
+ field.name().clone(),
+ )
+ })
+ .collect();
Review Comment:
`input_schema.field(idx)` will panic if a `FilterExec` ever contains an
out-of-range projection index. Even if this “shouldn’t happen”, optimizer rules
generally shouldn’t be able to panic the process; validate indices before
indexing and bail out of the rewrite if they’re invalid.
--
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]
