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
Prev Previous commit
Next Next commit
improve
  • Loading branch information
cloud-fan committed Jan 21, 2017
commit e7d928c3ac781ac8863bdb531d6163ef9b3d6e1c
Original file line number Diff line number Diff line change
Expand Up @@ -74,21 +74,26 @@ class EquivalentExpressions {
// loop. So we can't evaluate sub-expressions containing `LambdaVariable` at the beginning.
expr.find(_.isInstanceOf[LambdaVariable]).isDefined

// There are some special expressions that we should not recurse into children.
// There are some special expressions that we should not recurse into all of its children.
// 1. CodegenFallback: it's children will not be used to generate code (call eval() instead)
// 2. conditional expressions: common subexpressions will always be evaluated at the
// beginning, so we should not recurse into condition expressions,
// whole children may not get accessed according to the condition.
val shouldRecurse = expr match {
case _: CodegenFallback => false
case _: If => false
case _: CaseWhenBase => false
case _: Coalesce => false
case _ => true
// 2. If: common subexpressions will always be evaluated at the beginning, but the true and
// false expressions in `If` may not get accessed, according to the predicate
// expression. We should only recurse into the predicate expression.
// 3. CaseWhen: like `If`, the children of `CaseWhen` only get accessed in a certain
// condition. We should only recurse into the first condition expression as it
// will always get accessed.
Copy link
Member

Choose a reason for hiding this comment

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

CaseWhen could be very deep.

CASE WHEN expr1 THEN expr2 [WHEN expr3 THEN expr4]* [ELSE expr5] END
When expr1 = true, returns expr2; when expr3 = true, return expr4; else return expr5.

Compared with the previous impl, will we miss some expression elimination chances?

Copy link
Member

Choose a reason for hiding this comment

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

nvm, CaseWhen implements CodegenFallback. Thus, the previous impl skips it.

// 4. Coalesce: it's also a conditional expression, we should only recurse into the first
// children, because others may not get accessed.
Copy link
Member

Choose a reason for hiding this comment

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

Although Coalesce might miss some expression elimination chances, I think it is very rare when users use the same expressions in Coalesce.

Could you update the comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coalesce may be just a small part of the whole expression tree, and the children of Coalesce may be same with other expressions inside the expression tree.

def childrenToRecurse: Seq[Expression] = expr match {
case _: CodegenFallback => Nil
case i: If => i.predicate :: Nil
case c: CaseWhenBase => c.children.head :: Nil
Copy link
Member

Choose a reason for hiding this comment

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

This case is not reachable, could we leave a comment above this?

case c: Coalesce => c.children.head :: Nil
case other => other.children
}

if (!skip && !addExpr(expr) && shouldRecurse) {
expr.children.foreach(addExprTree)
if (!skip && !addExpr(expr)) {
childrenToRecurse.foreach(addExprTree)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ class SubexpressionEliminationSuite extends SparkFunSuite {
equivalence.addExprTree(ifExpr)
// the `add` inside `If` should not be added
assert(equivalence.getAllEquivalentExprs.count(_.size > 1) == 0)
assert(equivalence.getAllEquivalentExprs.count(_.size == 1) == 1) // only ifExpr
// only ifExpr and its predicate expression
assert(equivalence.getAllEquivalentExprs.count(_.size == 1) == 2)
}
}

Expand Down