Repository: spark Updated Branches: refs/heads/branch-2.3 182bc85f2 -> b3d1b1bcb
Revert "[SPARK-25714] Fix Null Handling in the Optimizer rule BooleanSimplification" This reverts commit 182bc85f2db0b3268b9b93ff91210811b00e1636. Project: http://git-wip-us.apache.org/repos/asf/spark/repo Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/b3d1b1bc Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/b3d1b1bc Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/b3d1b1bc Branch: refs/heads/branch-2.3 Commit: b3d1b1bcbaaefba5d78a52794a3efaf4e7dc6d6c Parents: 182bc85 Author: gatorsmile <gatorsm...@gmail.com> Authored: Fri Oct 12 21:26:39 2018 -0700 Committer: gatorsmile <gatorsm...@gmail.com> Committed: Fri Oct 12 21:26:39 2018 -0700 ---------------------------------------------------------------------- .../sql/catalyst/expressions/predicates.scala | 35 ------ .../sql/catalyst/optimizer/expressions.scala | 34 ++---- .../optimizer/BooleanSimplificationSuite.scala | 111 ++++--------------- .../org/apache/spark/sql/DataFrameSuite.scala | 10 -- 4 files changed, 33 insertions(+), 157 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/spark/blob/b3d1b1bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala ---------------------------------------------------------------------- diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala index a29b136..a6d41ea 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala @@ -120,13 +120,6 @@ case class Not(child: Expression) override def inputTypes: Seq[DataType] = Seq(BooleanType) - // +---------+-----------+ - // | CHILD | NOT CHILD | - // +---------+-----------+ - // | TRUE | FALSE | - // | FALSE | TRUE | - // | UNKNOWN | UNKNOWN | - // +---------+-----------+ protected override def nullSafeEval(input: Any): Any = !input.asInstanceOf[Boolean] override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { @@ -381,13 +374,6 @@ case class And(left: Expression, right: Expression) extends BinaryOperator with override def sqlOperator: String = "AND" - // +---------+---------+---------+---------+ - // | AND | TRUE | FALSE | UNKNOWN | - // +---------+---------+---------+---------+ - // | TRUE | TRUE | FALSE | UNKNOWN | - // | FALSE | FALSE | FALSE | FALSE | - // | UNKNOWN | UNKNOWN | FALSE | UNKNOWN | - // +---------+---------+---------+---------+ override def eval(input: InternalRow): Any = { val input1 = left.eval(input) if (input1 == false) { @@ -451,13 +437,6 @@ case class Or(left: Expression, right: Expression) extends BinaryOperator with P override def sqlOperator: String = "OR" - // +---------+---------+---------+---------+ - // | OR | TRUE | FALSE | UNKNOWN | - // +---------+---------+---------+---------+ - // | TRUE | TRUE | TRUE | TRUE | - // | FALSE | TRUE | FALSE | UNKNOWN | - // | UNKNOWN | TRUE | UNKNOWN | UNKNOWN | - // +---------+---------+---------+---------+ override def eval(input: InternalRow): Any = { val input1 = left.eval(input) if (input1 == true) { @@ -581,13 +560,6 @@ case class EqualTo(left: Expression, right: Expression) override def symbol: String = "=" - // +---------+---------+---------+---------+ - // | = | TRUE | FALSE | UNKNOWN | - // +---------+---------+---------+---------+ - // | TRUE | TRUE | FALSE | UNKNOWN | - // | FALSE | FALSE | TRUE | UNKNOWN | - // | UNKNOWN | UNKNOWN | UNKNOWN | UNKNOWN | - // +---------+---------+---------+---------+ protected override def nullSafeEval(left: Any, right: Any): Any = ordering.equiv(left, right) override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = { @@ -625,13 +597,6 @@ case class EqualNullSafe(left: Expression, right: Expression) extends BinaryComp override def nullable: Boolean = false - // +---------+---------+---------+---------+ - // | <=> | TRUE | FALSE | UNKNOWN | - // +---------+---------+---------+---------+ - // | TRUE | TRUE | FALSE | UNKNOWN | - // | FALSE | FALSE | TRUE | UNKNOWN | - // | UNKNOWN | UNKNOWN | UNKNOWN | TRUE | - // +---------+---------+---------+---------+ override def eval(input: InternalRow): Any = { val input1 = left.eval(input) val input2 = right.eval(input) http://git-wip-us.apache.org/repos/asf/spark/blob/b3d1b1bc/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala ---------------------------------------------------------------------- diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala index 41ddbad..038f5b3 100644 --- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala +++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala @@ -268,31 +268,15 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper { case a And b if a.semanticEquals(b) => a case a Or b if a.semanticEquals(b) => a - // The following optimization is applicable only when the operands are nullable, - // since the three-value logic of AND and OR are different in NULL handling. - // See the chart: - // +---------+---------+---------+---------+ - // | p | q | p OR q | p AND q | - // +---------+---------+---------+---------+ - // | TRUE | TRUE | TRUE | TRUE | - // | TRUE | FALSE | TRUE | FALSE | - // | TRUE | UNKNOWN | TRUE | UNKNOWN | - // | FALSE | TRUE | TRUE | FALSE | - // | FALSE | FALSE | FALSE | FALSE | - // | FALSE | UNKNOWN | UNKNOWN | FALSE | - // | UNKNOWN | TRUE | TRUE | UNKNOWN | - // | UNKNOWN | FALSE | UNKNOWN | FALSE | - // | UNKNOWN | UNKNOWN | UNKNOWN | UNKNOWN | - // +---------+---------+---------+---------+ - case a And (b Or c) if !a.nullable && Not(a).semanticEquals(b) => And(a, c) - case a And (b Or c) if !a.nullable && Not(a).semanticEquals(c) => And(a, b) - case (a Or b) And c if !a.nullable && a.semanticEquals(Not(c)) => And(b, c) - case (a Or b) And c if !b.nullable && b.semanticEquals(Not(c)) => And(a, c) - - case a Or (b And c) if !a.nullable && Not(a).semanticEquals(b) => Or(a, c) - case a Or (b And c) if !a.nullable && Not(a).semanticEquals(c) => Or(a, b) - case (a And b) Or c if !a.nullable && a.semanticEquals(Not(c)) => Or(b, c) - case (a And b) Or c if !b.nullable && b.semanticEquals(Not(c)) => Or(a, c) + case a And (b Or c) if Not(a).semanticEquals(b) => And(a, c) + case a And (b Or c) if Not(a).semanticEquals(c) => And(a, b) + case (a Or b) And c if a.semanticEquals(Not(c)) => And(b, c) + case (a Or b) And c if b.semanticEquals(Not(c)) => And(a, c) + + case a Or (b And c) if Not(a).semanticEquals(b) => Or(a, c) + case a Or (b And c) if Not(a).semanticEquals(c) => Or(a, b) + case (a And b) Or c if a.semanticEquals(Not(c)) => Or(b, c) + case (a And b) Or c if b.semanticEquals(Not(c)) => Or(a, c) // Common factor elimination for conjunction case and @ (left And right) => http://git-wip-us.apache.org/repos/asf/spark/blob/b3d1b1bc/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala ---------------------------------------------------------------------- diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala index a0de5f6..6cd1108 100644 --- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala +++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala @@ -29,7 +29,7 @@ import org.apache.spark.sql.catalyst.rules._ import org.apache.spark.sql.internal.SQLConf import org.apache.spark.sql.types.BooleanType -class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with PredicateHelper { +class BooleanSimplificationSuite extends PlanTest with PredicateHelper { object Optimize extends RuleExecutor[LogicalPlan] { val batches = @@ -72,14 +72,6 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with } private def checkConditionInNotNullableRelation( - input: Expression, expected: Expression): Unit = { - val plan = testNotNullableRelationWithData.where(input).analyze - val actual = Optimize.execute(plan) - val correctAnswer = testNotNullableRelationWithData.where(expected).analyze - comparePlans(actual, correctAnswer) - } - - private def checkConditionInNotNullableRelation( input: Expression, expected: LogicalPlan): Unit = { val plan = testNotNullableRelationWithData.where(input).analyze val actual = Optimize.execute(plan) @@ -127,55 +119,42 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with 'a === 'b || 'b > 3 && 'a > 3 && 'a < 5) } - test("e && (!e || f) - not nullable") { - checkConditionInNotNullableRelation('e && (!'e || 'f ), 'e && 'f) + test("e && (!e || f)") { + checkCondition('e && (!'e || 'f ), 'e && 'f) - checkConditionInNotNullableRelation('e && ('f || !'e ), 'e && 'f) + checkCondition('e && ('f || !'e ), 'e && 'f) - checkConditionInNotNullableRelation((!'e || 'f ) && 'e, 'f && 'e) + checkCondition((!'e || 'f ) && 'e, 'f && 'e) - checkConditionInNotNullableRelation(('f || !'e ) && 'e, 'f && 'e) + checkCondition(('f || !'e ) && 'e, 'f && 'e) } - test("e && (!e || f) - nullable") { - Seq ('e && (!'e || 'f ), - 'e && ('f || !'e ), - (!'e || 'f ) && 'e, - ('f || !'e ) && 'e, - 'e || (!'e && 'f), - 'e || ('f && !'e), - ('e && 'f) || !'e, - ('f && 'e) || !'e).foreach { expr => - checkCondition(expr, expr) - } - } + test("a < 1 && (!(a < 1) || f)") { + checkCondition('a < 1 && (!('a < 1) || 'f), ('a < 1) && 'f) + checkCondition('a < 1 && ('f || !('a < 1)), ('a < 1) && 'f) - test("a < 1 && (!(a < 1) || f) - not nullable") { - checkConditionInNotNullableRelation('a < 1 && (!('a < 1) || 'f), ('a < 1) && 'f) - checkConditionInNotNullableRelation('a < 1 && ('f || !('a < 1)), ('a < 1) && 'f) + checkCondition('a <= 1 && (!('a <= 1) || 'f), ('a <= 1) && 'f) + checkCondition('a <= 1 && ('f || !('a <= 1)), ('a <= 1) && 'f) - checkConditionInNotNullableRelation('a <= 1 && (!('a <= 1) || 'f), ('a <= 1) && 'f) - checkConditionInNotNullableRelation('a <= 1 && ('f || !('a <= 1)), ('a <= 1) && 'f) + checkCondition('a > 1 && (!('a > 1) || 'f), ('a > 1) && 'f) + checkCondition('a > 1 && ('f || !('a > 1)), ('a > 1) && 'f) - checkConditionInNotNullableRelation('a > 1 && (!('a > 1) || 'f), ('a > 1) && 'f) - checkConditionInNotNullableRelation('a > 1 && ('f || !('a > 1)), ('a > 1) && 'f) - - checkConditionInNotNullableRelation('a >= 1 && (!('a >= 1) || 'f), ('a >= 1) && 'f) - checkConditionInNotNullableRelation('a >= 1 && ('f || !('a >= 1)), ('a >= 1) && 'f) + checkCondition('a >= 1 && (!('a >= 1) || 'f), ('a >= 1) && 'f) + checkCondition('a >= 1 && ('f || !('a >= 1)), ('a >= 1) && 'f) } - test("a < 1 && ((a >= 1) || f) - not nullable") { - checkConditionInNotNullableRelation('a < 1 && ('a >= 1 || 'f ), ('a < 1) && 'f) - checkConditionInNotNullableRelation('a < 1 && ('f || 'a >= 1), ('a < 1) && 'f) + test("a < 1 && ((a >= 1) || f)") { + checkCondition('a < 1 && ('a >= 1 || 'f ), ('a < 1) && 'f) + checkCondition('a < 1 && ('f || 'a >= 1), ('a < 1) && 'f) - checkConditionInNotNullableRelation('a <= 1 && ('a > 1 || 'f ), ('a <= 1) && 'f) - checkConditionInNotNullableRelation('a <= 1 && ('f || 'a > 1), ('a <= 1) && 'f) + checkCondition('a <= 1 && ('a > 1 || 'f ), ('a <= 1) && 'f) + checkCondition('a <= 1 && ('f || 'a > 1), ('a <= 1) && 'f) - checkConditionInNotNullableRelation('a > 1 && (('a <= 1) || 'f), ('a > 1) && 'f) - checkConditionInNotNullableRelation('a > 1 && ('f || ('a <= 1)), ('a > 1) && 'f) + checkCondition('a > 1 && (('a <= 1) || 'f), ('a > 1) && 'f) + checkCondition('a > 1 && ('f || ('a <= 1)), ('a > 1) && 'f) - checkConditionInNotNullableRelation('a >= 1 && (('a < 1) || 'f), ('a >= 1) && 'f) - checkConditionInNotNullableRelation('a >= 1 && ('f || ('a < 1)), ('a >= 1) && 'f) + checkCondition('a >= 1 && (('a < 1) || 'f), ('a >= 1) && 'f) + checkCondition('a >= 1 && ('f || ('a < 1)), ('a >= 1) && 'f) } test("DeMorgan's law") { @@ -238,46 +217,4 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with checkCondition('e || !'f, testRelationWithData.where('e || !'f).analyze) checkCondition(!'f || 'e, testRelationWithData.where(!'f || 'e).analyze) } - - protected def assertEquivalent(e1: Expression, e2: Expression): Unit = { - val correctAnswer = Project(Alias(e2, "out")() :: Nil, OneRowRelation()).analyze - val actual = Optimize.execute(Project(Alias(e1, "out")() :: Nil, OneRowRelation()).analyze) - comparePlans(actual, correctAnswer) - } - - test("filter reduction - positive cases") { - val fields = Seq( - 'col1NotNULL.boolean.notNull, - 'col2NotNULL.boolean.notNull - ) - val Seq(col1NotNULL, col2NotNULL) = fields.zipWithIndex.map { case (f, i) => f.at(i) } - - val exprs = Seq( - // actual expressions of the transformations: original -> transformed - (col1NotNULL && (!col1NotNULL || col2NotNULL)) -> (col1NotNULL && col2NotNULL), - (col1NotNULL && (col2NotNULL || !col1NotNULL)) -> (col1NotNULL && col2NotNULL), - ((!col1NotNULL || col2NotNULL) && col1NotNULL) -> (col2NotNULL && col1NotNULL), - ((col2NotNULL || !col1NotNULL) && col1NotNULL) -> (col2NotNULL && col1NotNULL), - - (col1NotNULL || (!col1NotNULL && col2NotNULL)) -> (col1NotNULL || col2NotNULL), - (col1NotNULL || (col2NotNULL && !col1NotNULL)) -> (col1NotNULL || col2NotNULL), - ((!col1NotNULL && col2NotNULL) || col1NotNULL) -> (col2NotNULL || col1NotNULL), - ((col2NotNULL && !col1NotNULL) || col1NotNULL) -> (col2NotNULL || col1NotNULL) - ) - - // check plans - for ((originalExpr, expectedExpr) <- exprs) { - assertEquivalent(originalExpr, expectedExpr) - } - - // check evaluation - val binaryBooleanValues = Seq(true, false) - for (col1NotNULLVal <- binaryBooleanValues; - col2NotNULLVal <- binaryBooleanValues; - (originalExpr, expectedExpr) <- exprs) { - val inputRow = create_row(col1NotNULLVal, col2NotNULLVal) - val optimizedVal = evaluateWithoutCodegen(expectedExpr, inputRow) - checkEvaluation(originalExpr, optimizedVal, inputRow) - } - } } http://git-wip-us.apache.org/repos/asf/spark/blob/b3d1b1bc/sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala ---------------------------------------------------------------------- diff --git a/sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala b/sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala index 47dc452..ced53ba 100644 --- a/sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala +++ b/sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala @@ -29,7 +29,6 @@ import org.scalatest.Matchers._ import org.apache.spark.SparkException import org.apache.spark.sql.catalyst.TableIdentifier import org.apache.spark.sql.catalyst.expressions.Uuid -import org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation import org.apache.spark.sql.catalyst.plans.logical.{Filter, OneRowRelation, Union} import org.apache.spark.sql.execution.{FilterExec, QueryExecution, WholeStageCodegenExec} import org.apache.spark.sql.execution.aggregate.HashAggregateExec @@ -2335,13 +2334,4 @@ class DataFrameSuite extends QueryTest with SharedSQLContext { checkAnswer(df.where("(NOT a) OR a"), Seq.empty) } - - test("SPARK-25714 Null handling in BooleanSimplification") { - withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> ConvertToLocalRelation.ruleName) { - val df = Seq(("abc", 1), (null, 3)).toDF("col1", "col2") - checkAnswer( - df.filter("col1 = 'abc' OR (col1 != 'abc' AND col2 == 3)"), - Row ("abc", 1)) - } - } } --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org For additional commands, e-mail: commits-h...@spark.apache.org
