Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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 @@ -103,6 +103,7 @@ abstract class Optimizer(catalogManager: CatalogManager)
ReplaceNullWithFalseInPredicate,
PruneFilters,
SimplifyCasts,
SimplifyIf,
SimplifyCaseConversionExpressions,
RewriteCorrelatedScalarSubquery,
EliminateSerialization,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,17 @@ object SimplifyCasts extends Rule[LogicalPlan] {
}
}

/**
* Simplify if clauses of pattern `if(p, null, true|false)`, by replacing them with
* AND or OR clauses which are simpler and can better be pushed down
*/
object SimplifyIf extends Rule[LogicalPlan] {
val nullLiteral = Literal(null, BooleanType)
def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
case If(p, Literal(null, _), FalseLiteral) => And(p, nullLiteral)
case If(p, Literal(null, _), TrueLiteral) => Or(p, nullLiteral)
Copy link
Member

Choose a reason for hiding this comment

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

Is it common enough to write an if with a null with returning a bool? Adding a rule isn't free and it gives some overhead to all the queries basically during the planning. I would avoid doing it if it's pretty unlikely.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about merging these to the existing SimplifyConditionals as mentioned by @tanelk above? it will then just be a few more pattern matchings, right?

}
}

/**
* Removes nodes that are not necessary.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one or more
* contributor license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright ownership.
* The ASF licenses this file to You under the Apache License, Version 2.0
* (the "License"); you may not use this file except in compliance with
* the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.apache.spark.sql.catalyst.optimizer

import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.dsl.plans.DslLogicalPlan
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.plans.PlanTest
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules.RuleExecutor
import org.apache.spark.sql.types.BooleanType

class SimplifyIfSuite extends PlanTest {

object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("SimplifyIf", FixedPoint(1), SimplifyIf) :: Nil
}

val testRelation = LocalRelation('a.int)

test("simplify if when null is on the then branch") {
val nullLiteral = Literal(null, BooleanType)
assertEquivalent(If('a > 42, nullLiteral, false), And('a > 42, nullLiteral))
assertEquivalent(If('a > 42, nullLiteral, true), Or('a > 42, nullLiteral))
}

private def assertEquivalent(e1: Expression, e2: Expression) = {
val plan = testRelation.where(e1).analyze
val actual = Optimize.execute(plan)
val expected = testRelation.where(e2).analyze
comparePlans(actual, expected)
}

}