Uh oh!
There was an error while loading. Please reload this page.
[SPARK-38832][SQL][FOLLOWUP] Support propagate empty expression set for distinct key - #36281
[SPARK-38832][SQL][FOLLOWUP] Support propagate empty expression set for distinct key#36281ulysses-you wants to merge 2 commits into
Conversation
ulysses-you
commented
Apr 20, 2022
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
f2ceed8 to
1f0185fCompare
wangyum
left a comment
There was a problem hiding this comment.
Could we filter out empty distinctKey for safe?
cloud-fan
commented
Apr 21, 2022
@wangyum what's wrong with that code? |
wangyum
commented
Apr 21, 2022
For example: Set[ExpressionSet](ExpressionSet()).exists(_.subsetOf(ExpressionSet(ae.aggregateFunction.children.filterNot(_.foldable))))It is always true, it may have potential problems. |
ulysses-you
commented
Apr 21, 2022
if we get an unexpected empty ExpressionSet it should be a bug .. shall we trust the framework ? |
cloud-fan
commented
Apr 21, 2022
I don't see why empty expression set is special. If the framework has bugs and produces incorrect distinct keys, we will have query correctness bugs. |
I'm ok if you do not think it is needed. |
My point is empty expression set is informative and we shouldn't skip it. It means the data is distinct by Nil, so it has at most 1 row and can override any other distinct keys. |
cloud-fan
commented
Apr 22, 2022
thanks, merging to master! |
ulysses-you
commented
Apr 22, 2022
thank you @wangyum@cloud-fan |
What changes were proposed in this pull request?
DistinctKeyVisitorthat support propagate empty setWhy are the changes needed?
Make distinct keys can be used to optimize more case, see comment #36117 (comment)
Does this PR introduce any user-facing change?
Improve performance
How was this patch tested?
add test