ShayanGho opened a new pull request, #25181:
URL: https://github.com/apache/datafusion/pull/25181
## Which issue does this PR close?
- Closes #23007.
## Rationale for this change
A logical plan that contains two window aggregates with the same default
schema name, distinguished only by aliases, executes fine in DataFusion but
cannot be consumed back after a Substrait round trip. The consumer fails with:
```
SchemaError(DuplicateUnqualifiedField { name: "avg(data.b) PARTITION BY
[data.a] ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW" })
```
DataFusion's Substrait producer uses positional projections and does not
preserve intermediate window aliases: a `ProjectRel` carries expressions and an
output mapping. The aliases that kept the two columns apart in DataFusion (`AS
avg1`, `AS avg2`) are dropped on the wire. When the consumer rebuilds the outer
projection, the inherited window column and the new window expression both come
back under the same default name, and `Window::try_new` rejects the plan.
The SQL from the issue reproduces it, and so does a minimal plan built with
`LogicalPlanBuilder` that stacks two `row_number()` windows.
## What changes are included in this PR?
Two small changes, one per crate:
- **`datafusion-expr`**: `group_window_expr_by_sort_keys` (used by
`LogicalPlanBuilder::window_plan`) now looks through a single `Expr::Alias` to
derive the sort key and keeps the aliased expression in its group, so
`window_plan` can build a `Window` node whose output field carries an alias.
`LogicalPlanBuilder::window`, filter pushdown, and the physical planner already
handle aliased window expressions, so this makes `window_plan` consistent with
them. Nested aliases are still rejected, matching what filter pushdown
tolerates.
- **`datafusion-substrait` consumer**: `from_project_rel` reserves the input
schema's field names in a `NameTracker` (new `reserve_schema` helper), aliases
any new window expression whose default name would collide, and rewrites the
projection to reference the alias while preserving the projection's own output
names via `NamePreserver`. Plans without a collision are built exactly as
before.
## What is the testing strategy for this PR?
- `datafusion-expr`: a unit test
(`test_group_window_expr_by_sort_keys_aliased_window_expr`) asserting that an
aliased window expression is grouped by the inner function's sort key and kept
aliased. It failed on `main` with `Impossibly got non-window expr`.
- `datafusion-substrait`: two round-trip tests in
`roundtrip_logical_plan.rs`,
`stacked_windows_with_same_default_name_via_builder` (minimal
`LogicalPlanBuilder` form) and `chained_windows_with_same_default_name` (the
SQL from the issue). Both failed before this change with the
`DuplicateUnqualifiedField` error above. They assert Arrow schema equality
(names, types, nullability) and execute the consumed plan. They do not compare
plan text, because the consumer must synthesize an alias for the colliding
window, so the text legitimately differs. They compare Arrow schemas rather
than full `DFSchema`s because qualifiers and inferred functional dependencies
can change during reconstruction: DataFusion's Substrait producer does not
preserve subquery aliases, and the rebuilt `Window` nodes infer dependencies
the original plan did not record.
- The full `datafusion-substrait` test run passes unchanged (273 passed, 6
ignored across the lib, integration, and doc tests), confirming the
non-colliding path is unaffected.
This change was developed with AI assistance. I have reviewed and understand
every line and stand behind it.
## Are there any user-facing changes?
No API changes. Plans consumed from Substrait that previously failed with
`DuplicateUnqualifiedField` now consume successfully. Synthesized aliases of
the form `<default name>__temp__N` may appear in the decoded logical plan;
final output column names are preserved. Plans without such a collision are
unchanged.
--
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]
