[SPARK-12371][SQL] Runtime nullability check for NewInstance #10331

liancheng · 2015-12-16T16:59:09Z

This PR adds a new expression AssertNotNull to ensure non-nullable fields of products and case classes don't receive null values at runtime.

liancheng · 2015-12-16T17:00:48Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala

This is used to show nullability in the query plan / expression tree. I found it useful while debugging nullability issues.

SparkQA · 2015-12-16T17:26:23Z

Test build #47821 has finished for PR 10331 at commit 7c1f57d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

liancheng · 2015-12-17T00:50:56Z

Hm, similar as #10296, this PR also caught several existing nullability inconsistency bugs, which caused the last build failure. Trying to fix them.

yhuai · 2015-12-17T23:44:40Z

@liancheng Regarding the scope of this jira, my understanding is that when we create JVM objects, if there is any null values and we are trying to set them to primitive fields of the class, we should throw a Runtime exception to ask them to use Option or non-primitive JVM type (e.g. java.lang.Integer instead of Int). This is a runtime check. For example, we want to encode a row to a case class case class TestData(a: Int). If the first value of a row is null, we should throw the Runtime exception when we try to create a TestData instance because a is a primitive type field.

liancheng · 2015-12-20T18:19:23Z

@yhuai Thanks a lot for the explanation, I misunderstood the scope of the JIRA ticket. Updated this PR according to @marmbrus's [comment][1] in #10296. A new expression AssertNotNull is added to assert non-nullable constructor arguments are indeed non-null.

SparkQA · 2015-12-20T18:48:45Z

Test build #48084 has finished for PR 10331 at commit f32fc73.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

liancheng · 2015-12-20T18:48:51Z

retest this please

SparkQA · 2015-12-20T19:27:41Z

Test build #48086 has finished for PR 10331 at commit f32fc73.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

yhuai · 2015-12-21T04:30:59Z

sql/catalyst/src/test/resources/log4j.properties

revert these changes?

Oh yeah, thanks!

yhuai · 2015-12-21T04:39:40Z

Should we just keep the runtime part of changes?

liancheng · 2015-12-21T06:28:03Z

@yhuai Do you think we should move analysis phase checking into another PR or just drop that part? This check does find other nullability bugs (revealed by the Jenkins build failure). And I think Dataset nullability of Dataset schema should conforms to the underlying logical plan.

yhuai · 2015-12-21T06:41:01Z

Yeah, let's use this PR for the runtime check.

liancheng · 2015-12-21T07:56:15Z

@yhuai Narrowed down the scope of this PR. As we discussed offline, will open another one for the analysis phase check.

SparkQA · 2015-12-21T09:38:08Z

Test build #48103 has finished for PR 10331 at commit 94be50e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

SparkQA · 2015-12-21T14:27:12Z

Test build #48114 has finished for PR 10331 at commit 2c59a19.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

liancheng · 2015-12-21T16:44:34Z

cc @cloud-fan

yhuai · 2015-12-21T17:53:18Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala

Maybe we should revert the changes of this file?

SparkQA · 2015-12-21T18:17:15Z

Test build #48117 has finished for PR 10331 at commit 9f42052.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

yhuai · 2015-12-21T20:01:09Z

sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala

What does this do?

marmbrus · 2015-12-21T21:32:27Z

LGTM

cloud-fan · 2015-12-22T02:22:15Z

sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

My concern is: if the parent is null, we should shortcut the execution to return null directly, instead of going into the field and trigger the null check. However, looks like we only do this shortcut for product type by If(IsNull...), we may also need to handle array type and map type.

Good catch! Verified that the current mechanism doesn't play well with primitive arrays.

Actually what the test case I constructed reflected is another separate bug, which has been fixed in PR #10401.

Discussed with @cloud-fan offline. What he meant was, we should also add AssertNotNull for array types and map types, with which I totally agree, but I think it would be nice to be added in a separate PR.

SparkQA · 2015-12-22T03:50:23Z

Test build #48155 has finished for PR 10331 at commit 759c20d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

liancheng · 2015-12-22T08:29:43Z

retest this please

liancheng · 2015-12-22T08:37:20Z

The last build failure was irrelevant.

SparkQA · 2015-12-22T10:19:55Z

Test build #48178 has finished for PR 10331 at commit 759c20d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class AssertNotNull(\n

liancheng · 2015-12-22T11:21:33Z

Merging to master.

This PR adds a new expression `AssertNotNull` to ensure non-nullable fields of products and case classes don't receive null values at runtime. Author: Cheng Lian <[email protected]> Closes apache#10331 from liancheng/dataset-nullability-check.

liancheng force-pushed the dataset-nullability-check branch from 77a0649 to 7c1f57d Compare December 16, 2015 16:59

liancheng reviewed Dec 16, 2015
View reviewed changes

liancheng mentioned this pull request Dec 16, 2015

[SPARK-12335][SPARK-12336][SPARK-12341][SPARK-12342][SQL] Fixes several expression nullablility bugs #10296

Closed

liancheng changed the title ~~[SPARK-12323][SQL] Checks Dataset nullability during resolution~~ [SPARK-12371][SQL] Checks Dataset nullability during resolution Dec 16, 2015

liancheng force-pushed the dataset-nullability-check branch from 7c1f57d to 1c32728 Compare December 20, 2015 18:15

liancheng changed the title ~~[SPARK-12371][SQL] Checks Dataset nullability during resolution~~ [SPARK-12371][SQL] Dataset nullability check Dec 20, 2015

yhuai reviewed Dec 21, 2015
View reviewed changes

liancheng changed the title ~~[SPARK-12371][SQL] Dataset nullability check~~ [SPARK-12371][SQL] Runtime nullability check for NewInstance Dec 21, 2015

liancheng changed the title ~~[SPARK-12371][SQL] Runtime nullability check for NewInstance~~ [SPARK-12371][SQL] Runtime nullability check for NewInstance Dec 21, 2015

yhuai reviewed Dec 21, 2015
View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala Outdated

Copy link

Contributor

yhuai Dec 21, 2015

Choose a reason for hiding this comment

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

What does this do?

liancheng added 2 commits December 22, 2015 09:35

Checks Dataset nullability during resolution

c1589bd

Runtime nullability check for NewInstance

5b62827

liancheng added 7 commits December 22, 2015 09:54

Fixes typo in exception message

eb3d005

Reverts log4j.properties changes

482e096

Narrows down the scope of this PR

f4cb448

Supports Java bean

bff92d5

Fixes test failures

b66de75

Reverts unrelated changes

cf3fe16

Fixes compilation error introduced while rebasing

759c20d

cloud-fan reviewed Dec 22, 2015
View reviewed changes

liancheng force-pushed the dataset-nullability-check branch from 9f42052 to 759c20d Compare December 22, 2015 03:03

asfgit closed this in 42bfde2 Dec 22, 2015

liancheng deleted the dataset-nullability-check branch February 1, 2016 18:45

[SPARK-12371][SQL] Runtime nullability check for NewInstance #10331

[SPARK-12371][SQL] Runtime nullability check for NewInstance #10331

Uh oh!

Conversation

liancheng commented Dec 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 16, 2015

Uh oh!

liancheng commented Dec 17, 2015

Uh oh!

yhuai commented Dec 17, 2015

Uh oh!

liancheng commented Dec 20, 2015

Uh oh!

SparkQA commented Dec 20, 2015

Uh oh!

liancheng commented Dec 20, 2015

Uh oh!

SparkQA commented Dec 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhuai commented Dec 21, 2015

Uh oh!

liancheng commented Dec 21, 2015

Uh oh!

yhuai commented Dec 21, 2015

Uh oh!

liancheng commented Dec 21, 2015

Uh oh!

SparkQA commented Dec 21, 2015

Uh oh!

SparkQA commented Dec 21, 2015

Uh oh!

liancheng commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Dec 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 22, 2015

Uh oh!

liancheng commented Dec 22, 2015

Uh oh!

liancheng commented Dec 22, 2015

Uh oh!

SparkQA commented Dec 22, 2015

Uh oh!

liancheng commented Dec 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants