Repository navigation
Conversation
github-actions
Bot
added
optimizer
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?
Closes #26691.
Rationale for this change
A projection can change an aggregate value while keeping its output name. For example, ordering by
-max(v)in descending order must retain the groups with the smallest original maxima. TopK aggregation currently follows the reused name and can discard those groups before the projection runs.What changes are included in this PR?
Track the sort column through the corresponding projection output index. Continue through direct column references, including renamed or reordered outputs, and stop at a computed expression. Treat materialized CTEs as a boundary because their producer and output continuation can use different column positions.
The regression tests cover the reported MAX query, a computed projection over DISTINCT, a direct sort column with a computed sibling that should retain TopK aggregation, and an outer sort over a materialized CTE with reordered output columns.
What is the testing strategy for this PR?
The new tests fail with two result mismatches on the unchanged upstream source. With the production change applied, the same complete
aggregates_topk.sltfile passes. The positive EXPLAIN assertion retainsAggregateExec lim=[2].Focused validation: https://github.com/xgbtxy/contribution-checks/actions/runs/40091933143
On upstream base
92f170cb7ea5ad87bdff8fde89ac7a1541f3b0fa, formatting, all-target/all-feature Clippy, extended workspace tests, core/CLI tests, and the complete repository lint suite have passed.Required checks: https://github.com/xgbtxy/contribution-checks/actions/runs/40091939116
Materialized CTE control: https://github.com/xgbtxy/contribution-checks/actions/runs/40091936040
Fork CI for the submitted commit: https://github.com/xgbtxy/datafusion/actions/runs/40095216860 (28 jobs passed). The fork coverage upload lacked a token; the native tests completed successfully.
Are there any user-facing changes?
Queries that order computed aggregate or DISTINCT outputs with reused column names return the correct groups. There are no public API changes.
Developed with AI assistance. The original five-row reproducer was reported by @jayzhan211 in #26691.