[SPARK-20350] Add optimization rules to apply Complementation Laws. #17650
Conversation
ptkool
force-pushed
the
apply_complementation_laws
branch
from
5245d32 to
688b2f0
Compare
|
ok to test |
| PruneFilters) :: Nil | ||
| } | ||
|
|
||
| val testRelation = LocalRelation('a.int, 'b.int, 'c.int, 'd.string) |
There was a problem hiding this comment.
How about adding another boolean column?
val testRelation = LocalRelation('a.int, 'b.int, 'c.int, 'd.string, 'e.boolean)
Our filter conditions are not allowed to accept the non-boolean predicates
|
LGTM cc @cloud-fan @hvanhovell |
|
Test build #75839 has finished for PR 17650 at commit
|
|
|
||
| case a And b if Not(a).semanticEquals(b) => FalseLiteral | ||
| case a Or b if Not(a).semanticEquals(b) => TrueLiteral | ||
| case a And b if a.semanticEquals(Not(b)) => FalseLiteral |
There was a problem hiding this comment.
Logically it feels like duplication of code from line 156 ... but unfortunately Not is not smart enough to realise that. I think if you override the semanticEquals in Not then you should be able to get rid of this line. The advantage being we would make the expression smart enough to figure this out by itself rather than handling this in outside code (which is possibly more places in the code).
Same applies for line 159.
There was a problem hiding this comment.
I meant something like this for Not:
override def semanticEquals(other: Expression): Boolean = other match {
case Not(otherChild) => child.semanticEquals(otherChild)
case _ => child match {
case Not(innerChild) =>
// eliminate double negation
innerChild.semanticEquals(other)
case _ =>
super.semanticEquals(other)
}
}
There was a problem hiding this comment.
do we need this? Not(Not(a)) will be simplified to a
| comparePlans(actual, expected) | ||
| } | ||
|
|
||
| test("Complementation Laws") { |
There was a problem hiding this comment.
How about double negation ? ie. 'a && !(!'a)
There was a problem hiding this comment.
Is this really required for this PR?
| } | ||
|
|
||
| test("Complementation Laws") { | ||
| checkCondition('a && !'a, LocalRelation(testRelation.output, Seq.empty)) |
There was a problem hiding this comment.
just checkCondition('a && !'a, testRelation)?
There was a problem hiding this comment.
I thought that doing this would make the test a bit misleading. I wanted to make sure it was clear that the resulting plan node was a LocalRelation node that produced no data.
There was a problem hiding this comment.
ah i see. Then we need to create a new test relation with data, and test if the result is a empty relation here
|
LGTM |
|
Test build #75953 has finished for PR 17650 at commit
|
|
thanks, merging to master/2.2! |
## What changes were proposed in this pull request? Apply Complementation Laws during boolean expression simplification. ## How was this patch tested? Tested using unit tests, integration tests, and manual tests. Author: ptkool <[email protected]> Author: Michael Styles <[email protected]> Closes #17650 from ptkool/apply_complementation_laws. (cherry picked from commit 63824b2) Signed-off-by: Wenchen Fan <[email protected]>
|
Just FYI, we found a bug in this rule, regarding NULL handling. |
|
I am fixing it now. Thanks! |
## What changes were proposed in this pull request? Apply Complementation Laws during boolean expression simplification. ## How was this patch tested? Tested using unit tests, integration tests, and manual tests. Author: ptkool <[email protected]> Author: Michael Styles <[email protected]> Closes apache#17650 from ptkool/apply_complementation_laws.
5 participants
What changes were proposed in this pull request?
Apply Complementation Laws during boolean expression simplification.
How was this patch tested?
Tested using unit tests, integration tests, and manual tests.