Copilot commented on code in PR #26421:
URL: https://github.com/apache/datafusion/pull/26421#discussion_r4311246896
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -1458,9 +1458,14 @@ fn try_push_into_inputs(
// this input (e.g., through Filter → existing extraction
projection).
// This ensures the input's output schema is stable and won't
change
// when the TopDown pass later visits children.
- match try_push_input(&proj_plan, alias_generator)? {
- Some(pushed) => new_inputs.push(pushed),
- None => new_inputs.push(proj_plan),
+ let pushed = match try_push_input(&proj_plan, alias_generator)? {
+ Some(pushed) => pushed,
+ None => proj_plan,
+ };
+ if matches!(node, LogicalPlan::Union(_)) {
+ new_inputs.push(align_union_input(pushed, &per_input[idx])?);
+ } else {
+ new_inputs.push(pushed);
Review Comment:
The `matches!(node, LogicalPlan::Union(_))` check is invariant inside this
loop. Consider hoisting it to a single `let is_union = ...;` before iterating
inputs, and then branching on `is_union` inside the loop. This keeps the loop
body simpler and reduces the chance of accidentally diverging behavior
per-iteration later.
##########
datafusion/optimizer/src/extract_leaf_expressions.rs:
##########
@@ -1481,6 +1486,27 @@ fn try_push_into_inputs(
Ok(Some(new_node))
}
+/// Projects one input of a `Union` to the extracted aliases, in pair order,
+/// followed by the needed columns, in column order.
+///
+/// `Union` matches its inputs by position, not by name. A merge into an
+/// existing projection appends the aliases after the existing expressions,
+/// while a new projection puts them first. Without this, two inputs that take
+/// different paths disagree on the position of each column, and the union
+/// reads the alias of one input from an unrelated column of another input.
+fn align_union_input(
+ input: LogicalPlan,
+ target: &ExtractionTarget,
+) -> Result<LogicalPlan> {
+ let exprs: Vec<Expr> = target
+ .pairs
+ .iter()
+ .map(|(_, alias)| Expr::Column(Column::new_unqualified(alias)))
+ .chain(target.columns.iter().map(|col| Expr::Column(col.clone())))
+ .collect();
Review Comment:
This always adds a new `Projection` layer for every `Union` input, even when
the input is already in the desired layout. Consider an early-return fast path
(optional) that detects when `exprs` is already an identity projection over
`input` (same number/order of columns) and returns `input` directly to avoid
plan bloat and extra optimizer work.
--
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]
