Improve performance of constant aggregate window expression #16234
Conversation
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
|
FYI @alamb @Dandandan |
jonathanc-n
reviewed
Jun 3, 2025
|
I ran some of my own benchmarks and it looks significantly faster 🚀 : |
Co-authored-by: Jonathan Chen <[email protected]>
|
🤖 |
Dandandan
approved these changes
Jun 3, 2025
|
🤖: Benchmark completed Details
|
ran the wrong benchmark 😢 -- will fix |
|
🤖 |
|
🤖: Benchmark completed Details
|
Quite nice! |
|
Those benchmarks look nice, seems to have been skewed on my computer for the other queries. |
alamb
approved these changes
Jun 3, 2025
|
🤖 |
2010YOUY01
approved these changes
Jun 4, 2025
| @@ -186,6 +186,10 @@ pub trait AggregateWindowExpr: WindowExpr { | |||
| accumulator: &mut Box<dyn Accumulator>, | |||
| ) -> Result<ScalarValue>; | |||
|
|
|||
| /// Indicates whether this window function always produces the same result | |||
| /// for all rows in the partition. | |||
| fn is_constant(&self) -> bool; | |||
There was a problem hiding this comment.
Suggested change
| fn is_constant(&self) -> bool; | |
| fn is_constant_in_partition(&self) -> bool; |
nit: It would be great to be more specific.
There was a problem hiding this comment.
Good point, I'll address it a bit later.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance of constant aggregate window expressions by bypassing repeated accumulator evaluations for unbounded window functions.
- Introduces the is_constant method in the AggregateWindowExpr trait
- Implements constant window optimization in window_expr.rs and adds a corresponding field and evaluation logic in aggregate.rs
- Updates the SlidingAggregateWindowExpr to reflect non-constant behavior
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| datafusion/physical-expr/src/window/window_expr.rs | Adds is_constant method and shortcut evaluation for constant windows |
| datafusion/physical-expr/src/window/sliding_aggregate.rs | Implements is_constant as false |
| datafusion/physical-expr/src/window/aggregate.rs | Adds is_window_constant field and logic to determine constant windows |
Comments suppressed due to low confidence (1)
datafusion/physical-expr/src/window/window_expr.rs:280
- Please add unit tests to explicitly verify that the constant window optimization behaves correctly for both constant and non-constant window cases.
if self.is_constant() {
Co-authored-by: Yongting You <[email protected]>
jonathanc-n
approved these changes
Jun 4, 2025
There was a problem hiding this comment.
Thanks @suibianwanwank, this looks good to me! @alamb Should be good for merge.
|
🚀 thank you |
29 tasks
andygrove
added a commit
to andygrove/datafusion
that referenced
this pull request
Jun 6, 2025
…pache#16234)" This reverts commit 0c30374.
|
We found a bug in this change during DataFusion 48 testing if we can't fix it shortly I think we should back out this PR and we can include it in DataFusion 49 |
|
@alamb I'm sorry for the issue that occurred. After a preliminary review, I suspect the cause might be: @andygrove Could you please help me confirm this? Thanks! |
Sure, trying this now! |
|
I still see test failures: and I suggest we revert this PR for now and then add more tests based on the failing tests in Spark/Comet so that we can have more confidence when the PR is updated. |
Update:@andygrove reverted the change on the branch-48 line:
Let's continue the discussion and debugging on |
kosiew
pushed a commit
to kosiew/datafusion
that referenced
this pull request
Jun 10, 2025
…6234) * Improve performance of constant aggregate window expression * Update datafusion/physical-expr/src/window/aggregate.rs Co-authored-by: Jonathan Chen <[email protected]> * fmt * Update datafusion/physical-expr/src/window/aggregate.rs Co-authored-by: Yongting You <[email protected]> * Rename * fmt --------- Co-authored-by: Jonathan Chen <[email protected]> Co-authored-by: Yongting You <[email protected]>
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
For unbounded aggregate window functions, the result is the same for all rows within a partition. Therefore, we can avoid repeatedly computing the window bounds and evaluating the function for each row, which can improve performance.
Related Work in DuckDB：https://github.com/duckdb/duckdb/blob/main/src/function/window/window_constant_aggregator.cpp
Benchmark results on h2o_small_window (run locally) show performance improvements for Q1 and Q3, with no noticeable changes in other queries.
Before Optimization
After Optimization
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?