Disable AGG(IF()) rewrite for some corner cases#16566
Merged
highker merged 2 commits intoprestodb:masterfrom Aug 6, 2021
Merged
Disable AGG(IF()) rewrite for some corner cases#16566highker merged 2 commits intoprestodb:masterfrom
highker merged 2 commits intoprestodb:masterfrom
Conversation
yuanzhanhku
force-pushed
the
second
branch
2 times, most recently
from
05985bb to
36c9af2
Compare
yuanzhanhku
changed the title
kaikalur
requested changes
Aug 5, 2021
.../src/main/java/com/facebook/presto/operator/aggregation/arrayagg/SetAggregationFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/query/TestFilteredAggregations.java
Outdated
Show resolved
Hide resolved
kaikalur
approved these changes
Aug 5, 2021
highker
approved these changes
Aug 6, 2021
highker
reviewed
Aug 6, 2021
presto-main/src/test/java/com/facebook/presto/sql/query/TestFilteredAggregations.java
Outdated
Show resolved
Hide resolved
This rewrite will filter out the null values. It could change the behavior if the aggregation is also applied on NULLs. Also change SetAggregationFunction::isCalledOnNullInput and SetUnionFunction::isCalledOnNullInput to return true, as they are actually applied on NULL inputs. Add more tests for AGG(IF()) rewrites.
Not sure if non-deterministic functions could cause issues. But to be safe, disable the rewrite when the IF contains any non-deterministic functions to avoid some corner cases.
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
3 participants
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.
The rewrite could change the behavior of certain aggregations such as
set_agg(). See the added tests for example. To be safe, we disable it
for all aggregation functions returning arrays.
Test plan
Added query tests in TestFilteredAggregations.java.