fix(substrait): consume chained window functions whose default names collide - #25181
Open
ShayanGho wants to merge 2 commits into
Open
fix(substrait): consume chained window functions whose default names collide#25181ShayanGho wants to merge 2 commits into
ShayanGho wants to merge 2 commits into
Conversation
group_window_expr_by_sort_keys rejected any expression that was not a bare WindowFunction, so LogicalPlanBuilder::window_plan could not build a Window node whose output field carries an alias, even though LogicalPlanBuilder::window already accepts such expressions and both filter pushdown and the physical planner unwrap them. Look through one alias to derive the sort key and keep the aliased expression in the group. Nested aliases are still rejected, matching what filter pushdown tolerates. Co-authored-by: AI assistants <ai-assistants@users.noreply.github.com>
Substrait projections use positional references and omit intermediate window aliases. An inherited window column and a new window expression can therefore share the same default schema name, causing Window::try_new to reject the rebuilt plan with DuplicateUnqualifiedField. Reserve input schema names in a NameTracker, alias colliding window expressions, and rewrite their projection references. Use NamePreserver to preserve the projection's output names. Leave noncolliding window expressions unaliased. Add builder and SQL regressions checking output schemas and execution for chained windows with identical default names. Closes apache#23007 Co-authored-by: AI assistants <ai-assistants@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
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:
DataFusion's Substrait producer uses positional projections and does not preserve intermediate window aliases: a
ProjectRelcarries 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, andWindow::try_newrejects the plan.The SQL from the issue reproduces it, and so does a minimal plan built with
LogicalPlanBuilderthat stacks tworow_number()windows.What changes are included in this PR?
Two small changes, one per crate:
datafusion-expr:group_window_expr_by_sort_keys(used byLogicalPlanBuilder::window_plan) now looks through a singleExpr::Aliasto derive the sort key and keeps the aliased expression in its group, sowindow_plancan build aWindownode whose output field carries an alias.LogicalPlanBuilder::window, filter pushdown, and the physical planner already handle aliased window expressions, so this makeswindow_planconsistent with them. Nested aliases are still rejected, matching what filter pushdown tolerates.datafusion-substraitconsumer:from_project_relreserves the input schema's field names in aNameTracker(newreserve_schemahelper), 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 viaNamePreserver. 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 onmainwithImpossibly got non-window expr.datafusion-substrait: two round-trip tests inroundtrip_logical_plan.rs,stacked_windows_with_same_default_name_via_builder(minimalLogicalPlanBuilderform) andchained_windows_with_same_default_name(the SQL from the issue). Both failed before this change with theDuplicateUnqualifiedFielderror 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 fullDFSchemas because qualifiers and inferred functional dependencies can change during reconstruction: DataFusion's Substrait producer does not preserve subquery aliases, and the rebuiltWindownodes infer dependencies the original plan did not record.datafusion-substraittest 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
DuplicateUnqualifiedFieldnow consume successfully. Synthesized aliases of the form<default name>__temp__Nmay appear in the decoded logical plan; final output column names are preserved. Plans without such a collision are unchanged.