You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ShayanGho (via GitHub)" <gi...@apache.org> on 2026/09/11 04:22:19 UTC
[PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "ShayanGho (via GitHub)" <gi...@apache.org>.
ShayanGho commented on code in PR #25181:
URL: https://github.com/apache/datafusion/pull/25181#discussion_r4052198552
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1684,6 +1684,63 @@ async fn simple_window_function() -> Result<()> {
roundtrip("SELECT RANK() OVER (PARTITION BY a ORDER BY b), d, sum(b) OVER (PARTITION BY a) FROM data;").await
}
+#[tokio::test]
+async fn stacked_windows_with_same_default_name_via_builder() -> Result<()> {
Review Comment:
Good catch, done in the latest commit. The builder test now orders both row_number windows by a, filters a > 1 between them, and asserts the round-tripped values against the original plan (rn1 = 2, rn2 = 1). I also confirmed it fails if the consumer references the inherited window column instead of the new alias.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on PR #25181:
URL: https://github.com/apache/datafusion/pull/25181#issuecomment-5757946259
Thanks @ShayanGho!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25181:
URL: https://github.com/apache/datafusion/pull/25181#issuecomment-5629632713
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25181?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `91.56627%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.91%. Comparing base ([`7e5f40a`](https://app.codecov.io/gh/apache/datafusion/commit/7e5f40a69aae9199c3c682806750e40d6bebb398?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4dbab95`](https://app.codecov.io/gh/apache/datafusion/commit/4dbab9531dadbcd8199d0e60de7bbc3645f80842?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25181?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...trait/src/logical\_plan/consumer/rel/project\_rel.rs](https://app.codecov.io/gh/apache/datafusion/pull/25181?src=pr&el=tree&filepath=datafusion%2Fsubstrait%2Fsrc%2Flogical_plan%2Fconsumer%2Frel%2Fproject_rel.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9zdWJzdHJhaXQvc3JjL2xvZ2ljYWxfcGxhbi9jb25zdW1lci9yZWwvcHJvamVjdF9yZWwucnM=) | 90.47% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/expr/src/utils.rs](https://app.codecov.io/gh/apache/datafusion/pull/25181?src=pr&el=tree&filepath=datafusion%2Fexpr%2Fsrc%2Futils.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy91dGlscy5ycw==) | 91.66% | [1 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25181?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25181 +/- ##
==========================================
- Coverage 81.91% 81.91% -0.01%
==========================================
Files 1132 1132
Lines 421117 421189 +72
Branches 421117 421189 +72
==========================================
+ Hits 344961 345016 +55
- Misses 55767 55776 +9
- Partials 20389 20397 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25181?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs commented on code in PR #25181:
URL: https://github.com/apache/datafusion/pull/25181#discussion_r4045597319
##########
datafusion/substrait/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -1684,6 +1684,63 @@ async fn simple_window_function() -> Result<()> {
roundtrip("SELECT RANK() OVER (PARTITION BY a ORDER BY b), d, sum(b) OVER (PARTITION BY a) FROM data;").await
}
+#[tokio::test]
+async fn stacked_windows_with_same_default_name_via_builder() -> Result<()> {
Review Comment:
Could we also assert the output values and add a filter between the two windows so rn1 and rn2 differ? Both currently produce identical values, so accidentally referencing the inherited window column would go undetected. I checked a case using row_number() OVER (ORDER BY a) with WHERE a > 1 between windows: the remaining row has rn1 = 2, rn2 = 1, and it passes with this fix.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "ShayanGho (via GitHub)" <gi...@apache.org>.
ShayanGho commented on PR #25181:
URL: https://github.com/apache/datafusion/pull/25181#issuecomment-5705785273
Hello @gabotechs, whenever you get a chance can you please reivew this PR?
This fixs the Substrait round-trip failure in #23007 (DuplicateUnqualifiedField on chained windows). Its a small consumer side change plus one helper in datafusion-expr.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): consume chained window functions whose default names collide [datafusion]
Posted by "gabotechs (via GitHub)" <gi...@apache.org>.
gabotechs merged PR #25181:
URL: https://github.com/apache/datafusion/pull/25181
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org