Skip to content

[SPARK-24913][SQL] Make AssertNotNull and AssertTrue non-deterministic #22947

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
mgaido91 wants to merge 6 commits into from
Closed

[SPARK-24913][SQL] Make AssertNotNull and AssertTrue non-deterministic #22947

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
332db30
[SPARK-24913][SQL] Make AssertNotNull and AssertTrue non-deterministic
mgaido91 Nov 5, 2018
895e095
fix test failures
mgaido91 Nov 5, 2018
4bd7c2d
fix test failures
mgaido91 Nov 5, 2018
0a02dd9
Revert "fix test failures"
mgaido91 Nov 7, 2018
129d4f7
Revert "fix test failures"
mgaido91 Nov 7, 2018
1caa284
introduce hasSideEffect
mgaido91 Nov 7, 2018
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
2 changes: 2 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ case class AssertTrue(child: Expression) extends UnaryExpression with ImplicitCa

override def nullable: Boolean = true

override lazy val deterministic: Boolean = false
Copy link
Member

@viirya viirya Nov 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, this two expressions are very commonly used in other expressions' children. Making them non-deterministic will make such expressions as non-deterministic too, e.g., encoders. It might have big impact.

Copy link
Contributor Author

@mgaido91 mgaido91 Nov 5, 2018

Choose a reason for hiding this comment

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

yes, that's true. The change may be big indeed. I am open to other proposals, as creating a new sideEffect flag in the Expression.

Copy link
Member

@dbtsai dbtsai Nov 6, 2018

Choose a reason for hiding this comment

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

Because of this, I'm leaning towards creating a new flag instead of making them non-deterministic.

Copy link
Contributor Author

@mgaido91 mgaido91 Nov 6, 2018

Choose a reason for hiding this comment

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

I agree, thanks you all the comments. I'll try and add a new flag. I'll update the PR as soon as I have a solution for that. Probably it will take some time, though, as I will have to recheck all the usage of determistic.... Thanks.

Copy link
Member

@gatorsmile gatorsmile Nov 6, 2018

Choose a reason for hiding this comment

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

https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFAssertTrue.java#L45 Hive also marks it as non-deterministic.

cc @maryannxue too

Copy link
Contributor Author

@mgaido91 mgaido91 Nov 7, 2018

Choose a reason for hiding this comment

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

@gatorsmile thanks for your comment. I think the main issue with making this non-determistic is that we have basically to skip the sanity check at

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Line 368 in 0a32238

case o if o.expressions.exists(!_.deterministic) &&
, because we have AssertNotNull is a lot of other different locations. Another option may be to special case AssertNotNull and avoid that check for it... What do you think?

Copy link
Contributor Author

@mgaido91 mgaido91 Nov 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

any comment on this @gatorsmile @maryannxue ?


override def inputTypes: Seq[DataType] = Seq(BooleanType)

override def dataType: DataType = NullType
Expand Down
2 changes: 2 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1632,6 +1632,8 @@ case class AssertNotNull(child: Expression, walkedTypePath: Seq[String] = Nil)
override def foldable: Boolean = false
override def nullable: Boolean = false

override lazy val deterministic: Boolean = false

override def flatArguments: Iterator[Any] = Iterator(child)

private val errMsg = "Null value appeared in non-nullable field:" +
Expand Down
10 changes: 10 additions & 0 deletions ...yst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute
import org.apache.spark.sql.catalyst.dsl.plans._
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.Literal.{FalseLiteral, TrueLiteral}
import org.apache.spark.sql.catalyst.expressions.objects.AssertNotNull
import org.apache.spark.sql.catalyst.plans.PlanTest
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules._
Expand Down Expand Up @@ -166,4 +167,13 @@ class SimplifyConditionalSuite extends PlanTest with PredicateHelper {
Literal(1))
)
}

test("SPARK-24913: don't skip AssertNotNull and AssertTrue") {
val ifWithAssertNotNull = If(AssertNotNull(UnresolvedAttribute("b")), Literal(1), Literal(1))
val ifWithAssertTrue = If(AssertTrue(UnresolvedAttribute("b")), Literal(1), Literal(1))
val plan = Filter(And(ifWithAssertNotNull, ifWithAssertTrue), OneRowRelation())
val optimized = Optimize.execute(plan).analyze
// optimization should not change the plan
comparePlans(plan, optimized, checkAnalysis = false)
}
}