[SPARK-22472][SQL] add null check for top-level primitive values #19707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kiszk
reviewed
Nov 9, 2017
| assert(e.getCause.isInstanceOf[NullPointerException]) | ||
|
|
||
| withTempPath { path => | ||
| Seq(new Integer(1), null).toDF("i").write.parquet(path.getCanonicalPath) |
There was a problem hiding this comment.
not a big deal, but toDF("i") is more explicit about column name.
|
LGTM except one minor comment |
|
Test build #83644 has finished for PR 19707 at commit
|
| expr | ||
| } else { | ||
| AssertNotNull(expr, walkedTypePath) | ||
| } |
There was a problem hiding this comment.
Hi, @cloud-fan .
It looks great. Can we add a test case in ScalaReflectionSuite, too?
| assert(e.getCause.isInstanceOf[NullPointerException]) | ||
|
|
||
| withTempPath { path => | ||
| Seq(new Integer(1), null).toDF("i").write.parquet(path.getCanonicalPath) |
There was a problem hiding this comment.
Is this PR orthogonal to data source formats?
Could you test more data source like JSON, here?
There was a problem hiding this comment.
It doesn't matter, I just need a dataframe, I can even use Seq(Some(1), None).toDF, but using parquet is more convincing.
|
Test build #83654 has finished for PR 19707 at commit
|
gatorsmile
approved these changes
Nov 10, 2017
|
LGTM |
asfgit
pushed a commit
that referenced
this pull request
Nov 10, 2017
## What changes were proposed in this pull request? One powerful feature of `Dataset` is, we can easily map SQL rows to Scala/Java objects and do runtime null check automatically. For example, let's say we have a parquet file with schema `<a: int, b: string>`, and we have a `case class Data(a: Int, b: String)`. Users can easily read this parquet file into `Data` objects, and Spark will throw NPE if column `a` has null values. However the null checking is left behind for top-level primitive values. For example, let's say we have a parquet file with schema `<a: Int>`, and we read it into Scala `Int`. If column `a` has null values, we will get some weird results. ``` scala> val ds = spark.read.parquet(...).as[Int] scala> ds.show() +----+ |v | +----+ |null| |1 | +----+ scala> ds.collect res0: Array[Long] = Array(0, 1) scala> ds.map(_ * 2).show +-----+ |value| +-----+ |-2 | |2 | +-----+ ``` This is because internally Spark use some special default values for primitive types, but never expect users to see/operate these default value directly. This PR adds null check for top-level primitive values ## How was this patch tested? new test Author: Wenchen Fan <wenchen@databricks.com> Closes #19707 from cloud-fan/bug. (cherry picked from commit 0025dde) Signed-off-by: gatorsmile <gatorsmile@gmail.com> # Conflicts: # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala
|
Thanks! Merged to master/2.2 |
MatthewRBruce
pushed a commit
to Shopify/spark
that referenced
this pull request
Jul 31, 2018
## What changes were proposed in this pull request? One powerful feature of `Dataset` is, we can easily map SQL rows to Scala/Java objects and do runtime null check automatically. For example, let's say we have a parquet file with schema `<a: int, b: string>`, and we have a `case class Data(a: Int, b: String)`. Users can easily read this parquet file into `Data` objects, and Spark will throw NPE if column `a` has null values. However the null checking is left behind for top-level primitive values. For example, let's say we have a parquet file with schema `<a: Int>`, and we read it into Scala `Int`. If column `a` has null values, we will get some weird results. ``` scala> val ds = spark.read.parquet(...).as[Int] scala> ds.show() +----+ |v | +----+ |null| |1 | +----+ scala> ds.collect res0: Array[Long] = Array(0, 1) scala> ds.map(_ * 2).show +-----+ |value| +-----+ |-2 | |2 | +-----+ ``` This is because internally Spark use some special default values for primitive types, but never expect users to see/operate these default value directly. This PR adds null check for top-level primitive values ## How was this patch tested? new test Author: Wenchen Fan <wenchen@databricks.com> Closes apache#19707 from cloud-fan/bug. (cherry picked from commit 0025dde) Signed-off-by: gatorsmile <gatorsmile@gmail.com> # Conflicts: # sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/ScalaReflectionSuite.scala # sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
One powerful feature of
Datasetis, we can easily map SQL rows to Scala/Java objects and do runtime null check automatically.For example, let's say we have a parquet file with schema
<a: int, b: string>, and we have acase class Data(a: Int, b: String). Users can easily read this parquet file intoDataobjects, and Spark will throw NPE if columnahas null values.However the null checking is left behind for top-level primitive values. For example, let's say we have a parquet file with schema
<a: Int>, and we read it into ScalaInt. If columnahas null values, we will get some weird results.This is because internally Spark use some special default values for primitive types, but never expect users to see/operate these default value directly.
This PR adds null check for top-level primitive values
How was this patch tested?
new test