Skip to content

[SPARK-13995][SQL] Extract correct IsNotNull constraints for Expression #11809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
viirya wants to merge 13 commits into from
Closed

[SPARK-13995][SQL] Extract correct IsNotNull constraints for Expression #11809

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c2f9b05
init import.
viirya Mar 17, 2016
b4e6033
Add test.
viirya Mar 18, 2016
d6bb43b
Address comments.
viirya Mar 22, 2016
3d08625
Add nullIntolerant as a method to Expression.
viirya Mar 22, 2016
5fa1760
Address comments.
viirya Mar 22, 2016
c819572
Merge remote-tracking branch 'upstream/master' into constraint-cast
viirya Mar 22, 2016
3373396
Fix test.
viirya Mar 22, 2016
56ca15f
Use trait for null intolerant expression.
viirya Mar 23, 2016
c8fb736
Or is not null intolerant.
viirya Mar 23, 2016
81c46c7
Add more tests.
viirya Mar 23, 2016
6f7751d
Merge remote-tracking branch 'upstream/master' into constraint-cast
viirya Mar 24, 2016
8e8dd72
Consider IsNotNull(IsNotNull(expr)) case. Add a test for it too.
viirya Mar 24, 2016
ec84c5f
Merge remote-tracking branch 'upstream/master' into constraint-cast
viirya Mar 31, 2016
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
Next Next commit
init import.
  • Loading branch information
@viirya
viirya committed Mar 17, 2016
commit c2f9b058f6ad243e35518bc1758fe7ef7ac2c25f
22 changes: 22 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,3 +202,25 @@ object Unions {
}
}
}

/**
* A pattern that finds the original expression from a sequence of casts.
*/
object Casts {
def unapply(expr: Expression): Option[Attribute] = expr match {
case c: Cast => collectCasts(expr)
case _ => None
}

private def collectCasts(e: Expression): Option[Attribute] = {
Copy link
Member

@sameeragarwal sameeragarwal Mar 21, 2016

Choose a reason for hiding this comment

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

Can use pattern matching to simplify this block. Perhaps something along the lines of:

  private def collectCasts(expr: Expression): Option[Attribute] = {
    expr match {
      case e: Cast => collectCasts(e.child)
      case e: Attribute => Some(e)
      case _ => None
    }
  }

Copy link
Contributor

@marmbrus marmbrus Mar 21, 2016

Choose a reason for hiding this comment

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

:)

Copy link
Member

@gatorsmile gatorsmile Mar 21, 2016

Choose a reason for hiding this comment

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

Great! Also add the tag @tailrec ?

if (e.isInstanceOf[Cast]) {
collectCasts(e.children(0))
Copy link
Member

@sameeragarwal sameeragarwal Mar 21, 2016

Choose a reason for hiding this comment

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

e.child for better readability?

} else {
if (e.isInstanceOf[Attribute]) {
Some(e.asInstanceOf[Attribute])
Copy link
Contributor

@marmbrus marmbrus Mar 21, 2016

Choose a reason for hiding this comment

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

Any time you see isInstanceOf / asInstanceOf you might want to be using matching instead...

e match {
  case a: Attribute => Some(a)
  case Cast(e, _) => Some(collectCasts(e))
  case _ => None
}

} else {
None
}
}
}
}
8 changes: 8 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.spark.sql.catalyst.plans

import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.planning.Casts
import org.apache.spark.sql.catalyst.trees.TreeNode
import org.apache.spark.sql.types.{DataType, StructType}

Expand All @@ -36,6 +37,13 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]] extends TreeNode[PlanT
.union(constructIsNotNullConstraints(constraints))
.filter(constraint =>
constraint.references.nonEmpty && constraint.references.subsetOf(outputSet))
.map(_.transform {
Copy link
Member

@sameeragarwal sameeragarwal Mar 21, 2016

Choose a reason for hiding this comment

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

Instead of adding this here, how about we add something like .map(_.transform { case IsNotNull(Casts(a)) => IsNotNull(a) }) in constructIsNotNullConstraints? Wouldn't that avoid the redundant outputSet check?

case n @ IsNotNull(c) =>
c match {
case Casts(a) if outputSet.contains(a) => IsNotNull(a)
case _ => n
}
})
}

/**
Expand Down