[SPARK-35912][SQL] Fix cast struct contains null value to string/struct #33146

cfmcgrady · 2021-06-30T03:36:00Z

What changes were proposed in this pull request?

This PR fixes an issue that cast the struct which contains null value to other type has a difference result when we enable/disable codegen.

Here is an example:

  val schema = StructType(Seq(StructField("value",
    StructType(Seq(
      StructField("x", IntegerType, nullable = false),
      StructField("y", IntegerType, nullable = false)
    ))
  )))
  schema.printTreeString()
  // root
  // |-- value: struct (nullable = true)
  // |    |-- x: integer (nullable = false)
  // |    |-- y: integer (nullable = false)
  
  val testDS = Seq("""{"value":{"x":1}}""").toDS
  val jsonDS = spark.read.schema(schema).json(testDS)

  jsonDS.show()
  // incorrect result
  // +---------+
  // |    value|
  // +---------+
  // |{1, null}|
  // +---------+

  spark.sql("set spark.sql.codegen.wholeStage=false")
  jsonDS.show()
  // the result we expected due to y.nullable = false.
  // +------+
  // | value|
  // +------+
  // |{1, 0}|
  // +------+

Actually, the result should be depending on the field nullable setting. this bug also happens when we cast struct to struct.

Does this PR introduce any user-facing change?

No, only bug fix.

How was this patch tested?

New test.

AmplabJenkins · 2021-06-30T03:37:28Z

Can one of the admins verify this patch?

cfmcgrady · 2021-06-30T05:46:20Z

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

-      val javaType = JavaCode.javaType(ft)
-      code"""
-         |${if (i != 0) code"""$buffer.append(",");""" else EmptyBlock}
-         |if ($row.isNullAt($i)) {


When the actual value is null, for primitive type field, row.isNullAt(i) return ture, but row.getXXX return a default value.

For exmaple:

val r = new org.apache.spark.sql.catalyst.expressions.GenericInternalRow(Array(1, null)) println(r.getInt(0)) // 1 println(r.getInt(1)) // 0 println(r.isNullAt(1)) // true

so we cann't only check row.isNullAt(i) here, we need to do the same logical like BoundReference.doGenCode(), add nullable check.

HyukjinKwon · 2021-06-30T07:50:19Z

how is the cache issue related to the cast?

cfmcgrady · 2021-06-30T12:45:15Z

how is the cache issue related to the cast?

HI, @HyukjinKwon
The root cause is cast struct to other types, updated the pr description.

cfmcgrady · 2021-06-30T12:48:17Z

cc @cloud-fan @viirya @maropu

cloud-fan · 2021-06-30T13:11:33Z

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

@@ -406,19 +406,21 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
        if (row.numFields > 0) {
          val st = fields.map(_.dataType)
          val toUTF8StringFuncs = st.map(castToString)
-          if (row.isNullAt(0)) {
+          if (fields(0).nullable && row.isNullAt(0)) {


if fields(0).nullable is false, how can row.isNullAt(0) be true?

(I have the same question)

If user create dataframe from spark.internalCreateDataFrame(), the row.isNullAt() may be true even though the schema nullable is false.
For instance:

val schema = StructType(Seq( StructField("x", StructType(Seq( StructField("y", IntegerType, true), StructField("z", IntegerType, false) ))))) val rdd = spark.sparkContext.parallelize(Seq(InternalRow(InternalRow(1, null)))) val df = spark.internalCreateDataFrame(rdd, schema) df.show // current master branch output // +---------+ // | x| // +---------+ // |{1, null}| // +---------+

Although the spark.internalCreateDataFrame() is sql package private API, but spark.read.json() and spark.read.csv() call it without null value handled.(the example show in pr description)

Then we need to fix the nullability. There are so many places in the Spark codebase that relies on nullability to do optimizations. It's not possible to change all of them to not trust the nullability anymore.

Can we fix spark.read.json() to set the nullability correctly?

Ok, let me try.

HyukjinKwon · 2021-06-30T23:30:11Z

Hey mind explaining why cast path issue is related to being cached?

maropu · 2021-07-01T01:59:24Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala

+
+  test("SPARK-35912: Cast struct contains the null value to string") {
+    Seq(true, false).foreach {
+      case nullable =>


nit:

Seq(true, false).foreach { nullable =>

maropu · 2021-07-01T01:59:58Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala

+  }
+
+  test("SPARK-35912: Cast struct contains the null value to struct") {
+    Seq(true, false).foreach { case nullable =>


nit: Seq(true, false).foreach { nullable =>

maropu · 2021-07-01T02:00:12Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala

+      checkEvaluation(ret, expected)
+    }
+  }
+


nit: please remove this blank.

maropu · 2021-07-01T02:03:20Z

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

@@ -406,19 +406,21 @@ abstract class CastBase extends UnaryExpression with TimeZoneAwareExpression wit
        if (row.numFields > 0) {
          val st = fields.map(_.dataType)
          val toUTF8StringFuncs = st.map(castToString)
-          if (row.isNullAt(0)) {
+          if (fields(0).nullable && row.isNullAt(0)) {


(I have the same question)

cfmcgrady · 2021-07-01T04:00:18Z

Hey mind explaining why cast path issue is related to being cached?

Actually, the cached result is what we want. The issue is that spark.read.json is not an expected result when the input schema has a nullable setting to false. From the log, we found it's a cast operation.

=== Result of Batch Preparations ===
 CollectLimit 21                                    CollectLimit 21
!+- Project [cast(value#5 as string) AS value#18]   +- *(1) Project [cast(value#5 as string) AS value#18]
!   +- Scan ExistingRDD[value#5]                       +- *(1) Scan ExistingRDD[value#5]

HyukjinKwon · 2021-07-01T04:43:48Z

Shouldn't it fail instead of setting it as 0 or null? I feel like the handling should be done somewhere in JacksonParser.

cfmcgrady · 2021-07-01T12:06:45Z

Shouldn't it fail instead of setting it as 0 or null? I feel like the handling should be done somewhere in JacksonParser.

Thanks for your suggestion, I'll try.

cfmcgrady · 2021-07-05T08:44:28Z

Create a new PR

BTW, Shall we merge this PR? The cast issue may occur when the user create dataframe from API spark.internalCreateDataFrame(). cc @cloud-fan @maropu

cloud-fan · 2021-07-05T08:54:00Z

spark.internalCreateDataFrame() is a private API and users should be responsible if they do something wrong. We can't merge this PR, because we can't sacrifice the performance in the happy path for invalid usage of private APIs from users.

github-actions bot added the SQL label Jun 30, 2021

cfmcgrady commented Jun 30, 2021

View reviewed changes

cfmcgrady changed the title ~~[WIP][SPARK-35912][SQL] Fix cast struct contains null value to string~~ [SPARK-35912][SQL] Fix cast struct contains null value to string/struct Jun 30, 2021

cloud-fan reviewed Jun 30, 2021

View reviewed changes

maropu reviewed Jul 1, 2021

View reviewed changes

cfmcgrady added 3 commits July 1, 2021 20:04

fix cast struct contains null value.

2f53caf

fix struct to struct

1a6cdd3

nit

25c396c

cfmcgrady force-pushed the SPARK-35912 branch from 5e2b67d to 25c396c Compare July 1, 2021 12:07

cfmcgrady mentioned this pull request Jul 5, 2021

[SPARK-35912][SQL] Fix nullability of spark.read.json #33212

Closed

cfmcgrady closed this Jul 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-35912][SQL] Fix cast struct contains null value to string/struct #33146

[SPARK-35912][SQL] Fix cast struct contains null value to string/struct #33146

cfmcgrady commented Jun 30, 2021 •

edited

Loading

AmplabJenkins commented Jun 30, 2021

cfmcgrady Jun 30, 2021

HyukjinKwon commented Jun 30, 2021

cfmcgrady commented Jun 30, 2021

cfmcgrady commented Jun 30, 2021

cloud-fan Jun 30, 2021

maropu Jul 1, 2021

cfmcgrady Jul 1, 2021 •

edited

Loading

cloud-fan Jul 1, 2021

cfmcgrady Jul 1, 2021

HyukjinKwon commented Jun 30, 2021

maropu Jul 1, 2021

maropu Jul 1, 2021

maropu Jul 1, 2021

maropu Jul 1, 2021

cfmcgrady commented Jul 1, 2021

HyukjinKwon commented Jul 1, 2021 •

edited

Loading

cfmcgrady commented Jul 1, 2021

cfmcgrady commented Jul 5, 2021

cloud-fan commented Jul 5, 2021

[SPARK-35912][SQL] Fix cast struct contains null value to string/struct #33146

[SPARK-35912][SQL] Fix cast struct contains null value to string/struct #33146

Conversation

cfmcgrady commented Jun 30, 2021 • edited Loading

What changes were proposed in this pull request?

Does this PR introduce any user-facing change?

How was this patch tested?

AmplabJenkins commented Jun 30, 2021

Choose a reason for hiding this comment

HyukjinKwon commented Jun 30, 2021

cfmcgrady commented Jun 30, 2021

cfmcgrady commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmcgrady commented Jul 1, 2021

HyukjinKwon commented Jul 1, 2021 • edited Loading

cfmcgrady commented Jul 1, 2021

cfmcgrady commented Jul 5, 2021

cloud-fan commented Jul 5, 2021

cfmcgrady commented Jun 30, 2021 •

edited

Loading

cfmcgrady Jul 1, 2021 •

edited

Loading

HyukjinKwon commented Jul 1, 2021 •

edited

Loading