Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -463,6 +463,8 @@ object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
case If(Literal(null, _), _, falseValue) => falseValue
case If(cond, trueValue, falseValue)
if cond.deterministic && trueValue.semanticEquals(falseValue) => trueValue
case If(cond, l @ Literal(null, _), FalseLiteral) if !cond.nullable => And(cond, l)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the other way around? e.g. case If(cond, FalseLiteral, l @ Literal(null, _))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah it's covered in the followup #29603

case If(cond, l @ Literal(null, _), TrueLiteral) if !cond.nullable => Or(Not(cond), l)

case e @ CaseWhen(branches, elseValue) if branches.exists(x => falseOrNullLiteral(x._1)) =>
// If there are branches that are always false, remove them.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,14 +221,14 @@ class BooleanSimplificationSuite extends PlanTest with ExpressionEvalHelper with

test("Complementation Laws - null handling") {
checkCondition('e && !'e,
testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), false)).analyze)
testRelationWithData.where(And(Literal(null, BooleanType), 'e.isNull)).analyze)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I think it is better to test BooleanSimplification like original, but BooleanSimplificationSuite's optimizer mixes SimplifyConditionals and BooleanSimplification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. What is the recommended way for adding test suite for optimizers? should each test suite only use the corresponding optimizer rule, or it's OK to also add those relevant ones?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we only test corresponding rule in each test suite. But some suites use multiple rules like this one. For this one, although we can still only test BooleanSimplification if we add another optimizer, but seems too much. Maybe currently it's good enough. Maybe add a comment.

checkCondition(!'e && 'e,
testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), false)).analyze)
testRelationWithData.where(And(Literal(null, BooleanType), 'e.isNull)).analyze)

checkCondition('e || !'e,
testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), true)).analyze)
testRelationWithData.where(Or('e.isNotNull, Literal(null, BooleanType))).analyze)
checkCondition(!'e || 'e,
testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), true)).analyze)
testRelationWithData.where(Or('e.isNotNull, Literal(null, BooleanType))).analyze)
}

test("Complementation Laws - negative case") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import org.apache.spark.sql.catalyst.rules._
import org.apache.spark.sql.types.{BooleanType, IntegerType}


class SimplifyConditionalSuite extends PlanTest with PredicateHelper {
class SimplifyConditionalSuite extends PlanTest with ExpressionEvalHelper with PredicateHelper {

object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("SimplifyConditionals", FixedPoint(50),
Expand Down Expand Up @@ -165,4 +165,30 @@ class SimplifyConditionalSuite extends PlanTest with PredicateHelper {
Literal(1))
)
}

test("simplify if when then clause is null and else clause is boolean") {
val p = IsNull('a)
val nullLiteral = Literal(null, BooleanType)
assertEquivalent(If(p, nullLiteral, FalseLiteral), And(p, nullLiteral))
assertEquivalent(If(p, nullLiteral, TrueLiteral), Or(IsNotNull('a), nullLiteral))

// the rule should not apply to nullable predicate
Seq(TrueLiteral, FalseLiteral).foreach { b =>
assertEquivalent(If(GreaterThan('a, 42), nullLiteral, b),
If(GreaterThan('a, 42), nullLiteral, b))
}

// check evaluation also
Seq(TrueLiteral, FalseLiteral).foreach { b =>
checkEvaluation(If(b, nullLiteral, FalseLiteral), And(b, nullLiteral).eval(EmptyRow))
checkEvaluation(If(b, nullLiteral, TrueLiteral), Or(Not(b), nullLiteral).eval(EmptyRow))
}

// should have no effect on expressions with nullable if condition
assert((Factorial(5) > 100L).nullable)
Seq(TrueLiteral, FalseLiteral).foreach { b =>
checkEvaluation(If(Factorial(5) > 100L, nullLiteral, b),
If(Factorial(5) > 100L, nullLiteral, b).eval(EmptyRow))
}
}
}