forked from facebookincubator/velox
-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix min/max(x, n) (facebookincubator#8311)
Summary: Velox has an optimization for Window operation with incremental aggregation when there are peer rows with the same frame. In this situation, the aggregation result is only computed at the first row of the peer and the rest rows simply copy this result. This optimization assumes that results of incremental aggregation in Window operation on peer rows should be the same. However, min/max(x, n) in Velox breaks this assumption because their extractValues() method causes the accumulator to be cleared, making the peer rows after the first row expect a different result. This diff fixes min/max(x, n) to make the extraction method not clear the accumulator. The behavior after the fix also align with Presto's. This diff also adds a method testIncrementalAggregation in testAggregations to check that extractValues() doesn't change accumulator afterwards for all aggregation functions. After this fix, only min_by/max_by(x, y, n) doesn't pass testIncrementalAggregation. This diff fixes facebookincubator#8138. Differential Revision: D52638334
- Loading branch information
1 parent
7cf5a00
commit 170a048
Showing
2 changed files
with
51 additions
and
17 deletions.
There are no files selected for viewing
This file contains 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
This file contains 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