[SPARK-21677][SQL] json_tuple throws NullPointException when column is null as string type #18930

jmchung · 2017-08-13T02:14:24Z

What changes were proposed in this pull request?

scala> Seq(("""{"Hyukjin": 224, "John": 1225}""")).toDS.selectExpr("json_tuple(value, trim(null))").show()
...
java.lang.NullPointerException
	at ...

Currently the null field name will throw NullPointException. As a given field name null can't be matched with any field names in json, we just output null as its column value. This PR achieves it by returning a very unlikely column name __NullFieldName in evaluation of the field names.

How was this patch tested?

Added unit test.

jmchung · 2017-08-13T02:14:55Z

cc @viirya

viirya · 2017-08-13T02:17:01Z

cc @HyukjinKwon

viirya · 2017-08-13T02:18:48Z

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

  @transient private lazy val fieldExpressions: Seq[Expression] = children.tail

+  // toString on null will throw NullPointerException so that return a very unlikely column name
+  private val nullFieldName = "__NullFieldName"


A field name given with constant null will be replaced with this pseudo field name.

HyukjinKwon · 2017-08-13T05:15:48Z

ok to test

SparkQA · 2017-08-13T07:04:50Z

Test build #80579 has finished for PR 18930 at commit ffa575a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-08-13T09:04:16Z

retest this please.

SparkQA · 2017-08-13T11:50:24Z

Test build #80587 has finished for PR 18930 at commit ffa575a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-08-13T16:34:21Z

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

  @transient private lazy val fieldExpressions: Seq[Expression] = children.tail

+  // a field name given with constant null will be replaced with this pseudo field name
+  private val nullFieldName = "__NullFieldName"


@jmchung, could we maybe compute this foldable related optimization ahead -
https://github.com/jmchung/spark/blob/ffa575a6731fef3e0731b73e0f7311cb024e831b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala#L425-L439 and remove this fake field name?

I think we can make a function for the above codes first and then use it for computation for each row. Did I understand correctly?

I tried a rough version I thought - https://github.com/jmchung/spark/compare/SPARK-21677...HyukjinKwon:tmp-18930?expand=1, @viirya what do you think about this?

Yeah, I've also considered using Option here. But don't want to come out Option version from me first, so we can experience review process. It looks good to me.

@HyukjinKwon @viirya Yep, we've discarded the fake field name and use Option here. We made a slight revision to deal with the None in foldableFieldNames instead of creating a new function.

SparkQA · 2017-08-14T17:59:23Z

Test build #80634 has finished for PR 18930 at commit 5d71263.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-08-15T03:53:52Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

+        |SELECT json_tuple('{"a" : 1, "b" : 2}'
+        |, cast(NULL AS STRING), 'b'
+        |, cast(NULL AS STRING), 'a')
+      """.stripMargin), Row(null, "2", null, "1"))


@jmchung Can we also add the test we discussed in slack which mixes constant field name and non constant one?

@viirya Done, the added test case contains column name, constant field name, and null field name.

…ield name

viirya · 2017-08-15T04:59:53Z

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


  // eagerly evaluate any foldable the field names
-  @transient private lazy val foldableFieldNames: IndexedSeq[String] = {
+  @transient private lazy val foldableFieldNames: Array[Option[String]] = {


I think we should continue to use IndexedSeq which is more efficient as foldableFieldNames will be used for many times.

@viirya ok, thanks

SparkQA · 2017-08-15T07:04:50Z

Test build #80656 has finished for PR 18930 at commit 0078445.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

…erformance

SparkQA · 2017-08-15T10:52:16Z

Test build #80664 has finished for PR 18930 at commit 5c69df5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-08-15T13:35:53Z

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

      }
    }

+    // Array[String]


Can we remove this line?

viirya · 2017-08-15T13:36:39Z

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

-    }.toIndexedSeq
-  }
+    }
+  }.toIndexedSeq


Can we move toIndexedSeq to inner block, i.e. after the map?

@viirya Done: (1) remove redundant comment (2) move toIndexedSeq after the map

viirya · 2017-08-15T13:40:19Z

LGTM except for minor comments.

HyukjinKwon

LGTM too except for the comment above.

HyukjinKwon · 2017-08-15T14:25:29Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

    }
  }
+
+  test("SPARK-21677: json_tuple throws NullPointException when column is null as string type") {


Could we move this to spark/sql/core/src/test/resources/sql-tests/inputs/json-functions.sql and/or spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala?

This is just an end-to-end test case. We also need to add unit test cases in JsonExpressionsSuite

The end-to-end test at L2047 may not be able to move to JsonExpressionsSuite. We can have some unit test cases similar to L2039 in JsonExpressionsSuite as @gatorsmile suggested.

It is also good to have this end-to-end tests in json-functions.sql.

@gatorsmile has added unit test case in JsonExpressionsSuite
@viirya also add end-to-end test in json-functions.sql

SparkQA · 2017-08-15T17:00:43Z

Test build #80688 has finished for PR 18930 at commit ab16929.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-08-15T17:23:48Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

+        .createOrReplaceTempView("jsonTable")
+
+      checkAnswer(
+        sql("""SELECT json_tuple(jsonField, b, cast(NULL AS STRING), 'a') FROM jsonTable"""),


Nit: """ -> "

will move L2053 to json-functions.sql

gatorsmile · 2017-08-15T18:40:31Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/json/JsonSuite.scala

+        |SELECT json_tuple('{"a" : 1, "b" : 2}'
+        |, cast(NULL AS STRING), 'b'
+        |, cast(NULL AS STRING), 'a')
+      """.stripMargin), Row(null, "2", null, "1"))


Nit: move Row(null, "2", null, "1")) to the next line.

…t case in json-functions.sql

SparkQA · 2017-08-16T16:49:01Z

Test build #80740 has finished for PR 18930 at commit e0e0c74.

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

gatorsmile · 2017-08-16T16:58:59Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+describe function extended json_tuple;
+select json_tuple('{"a" : 1, "b" : 2}', cast(NULL AS STRING), 'b', cast(NULL AS STRING), 'a')
+create temporary view jsonTable(jsonField, a, b) as select * from values '{"a": 1, "b": 2}', 'a', 'b';
+SELECT json_tuple(jsonField, b, cast(NULL AS STRING), 'a') FROM jsonTable


To generate the result file, you need to run the command

SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "sql/test-only *SQLQueryTestSuite"

gatorsmile · 2017-08-16T16:59:19Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+describe function extended json_tuple;
+select json_tuple('{"a" : 1, "b" : 2}', cast(NULL AS STRING), 'b', cast(NULL AS STRING), 'a')
+create temporary view jsonTable(jsonField, a, b) as select * from values '{"a": 1, "b": 2}', 'a', 'b';
+SELECT json_tuple(jsonField, b, cast(NULL AS STRING), 'a') FROM jsonTable


Also add an extra space at the end of this file

gatorsmile · 2017-08-16T17:00:15Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+describe function json_tuple;
+describe function extended json_tuple;
+select json_tuple('{"a" : 1, "b" : 2}', cast(NULL AS STRING), 'b', cast(NULL AS STRING), 'a')
+create temporary view jsonTable(jsonField, a, b) as select * from values '{"a": 1, "b": 2}', 'a', 'b';


To be consistent with the other SQL commands, use upper cases for SQL keywords

gatorsmile · 2017-08-16T17:00:27Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

 select from_json();
+-- json_tuple
+describe function json_tuple;
+describe function extended json_tuple;


no need to add these two desc commands.

viirya · 2017-08-17T02:22:25Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+describe function extended json_tuple;
+select json_tuple('{"a" : 1, "b" : 2}', cast(NULL AS STRING), 'b', cast(NULL AS STRING), 'a')
+create temporary view jsonTable(jsonField, a, b) as select * from values '{"a": 1, "b": 2}', 'a', 'b';
+SELECT json_tuple(jsonField, b, cast(NULL AS STRING), 'a') FROM jsonTable


Remember to drop the created view DROP VIEW IF EXISTS jsonTable;

@gatorsmile @viirya Thank you for your time to review the code. SQL statements are consistent in style and the golden file of json-functions.sql also committed.

…son-functions.sql

SparkQA · 2017-08-17T05:24:49Z

Test build #80758 has finished for PR 18930 at commit 5191ed4.

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

viirya · 2017-08-17T05:28:12Z

retest this please.

SparkQA · 2017-08-17T07:04:49Z

Test build #80768 has finished for PR 18930 at commit 5191ed4.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-08-17T07:10:37Z

retest this please

SparkQA · 2017-08-17T09:52:30Z

Test build #80774 has finished for PR 18930 at commit 5191ed4.

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

HyukjinKwon · 2017-08-17T10:02:21Z

retest this please

SparkQA · 2017-08-17T12:39:56Z

Test build #80783 has finished for PR 18930 at commit 5191ed4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-08-17T12:44:40Z

LGTM

HyukjinKwon · 2017-08-17T13:00:02Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

 select from_json();
+-- json_tuple
+SELECT json_tuple('{"a" : 1, "b" : 2}', CAST(NULL AS STRING), 'b', CAST(NULL AS STRING), 'a');
+CREATE TEMPORARY VIEW jsonTable(jsonField, a, b) AS SELECT * FROM VALUES ('{"a": 1, "b": 2}', 'a', 'b');


Looks 'a' field is not used here.

I suggest we can rewrite to:

CREATE TEMPORARY VIEW jsonTable(jsonField, a) AS SELECT * FROM VALUES ('{"a": 1, "b": 2}', 'a'); SELECT json_tuple(jsonField, 'b', CAST(NULL AS STRING), a) FROM jsonTable;

ok, we have altered the 'a' to table field a.

HyukjinKwon · 2017-08-17T13:00:28Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+CREATE TEMPORARY VIEW jsonTable(jsonField, a, b) AS SELECT * FROM VALUES ('{"a": 1, "b": 2}', 'a', 'b');
+SELECT json_tuple(jsonField, b, CAST(NULL AS STRING), 'a') FROM jsonTable;
+-- Clean up
+DROP VIEW IF EXISTS jsonTable;


It looks the project style does not require a newline at the end but I would personally add this ..

+1. I remember @gatorsmile has also suggested to add it.

@HyukjinKwon @viirya Fixed.

HyukjinKwon · 2017-08-17T13:00:40Z

LGTM too.

SparkQA · 2017-08-17T16:14:24Z

Test build #80788 has finished for PR 18930 at commit ff3b9da.

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

HyukjinKwon · 2017-08-17T16:16:09Z

retest this please

SparkQA · 2017-08-17T18:25:09Z

Test build #80789 has finished for PR 18930 at commit ff3b9da.

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

gatorsmile · 2017-08-17T20:14:44Z

sql/core/src/test/resources/sql-tests/inputs/json-functions.sql

+CREATE TEMPORARY VIEW jsonTable(jsonField, a) AS SELECT * FROM VALUES ('{"a": 1, "b": 2}', 'a');
+SELECT json_tuple(jsonField, 'b', CAST(NULL AS STRING), a) FROM jsonTable;
+-- Clean up
+DROP VIEW IF EXISTS jsonTable;


Nit: just FYI, we do not need to drop the temp view in SQLQueryTestSuite.

This is not a big deal. Thus, I will merge it when the test can pass.

gatorsmile · 2017-08-17T20:15:20Z

retest this please

SparkQA · 2017-08-17T22:40:54Z

Test build #80805 has finished for PR 18930 at commit ff3b9da.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2017-08-17T23:00:13Z

Thanks! Merging to master.

viirya · 2017-08-17T23:17:25Z

Thanks @HyukjinKwon @gatorsmile

jmchung · 2017-08-18T02:12:43Z

Thanks @viirya @HyukjinKwon @gatorsmile , I learned a lot from this journey.

jmchung added 3 commits August 12, 2017 16:37

fix null field name and add corresponding test

596c280

add description

796041f

modify the comment about nullFieldName

f07a9f7

jmchung changed the title ~~Spark 21677~~ [SPARK-21677][SQL] json_tuple throws NullPointException when column is null as string type Aug 13, 2017

viirya reviewed Aug 13, 2017

View reviewed changes

make properly comment on pseudo field name

ffa575a

HyukjinKwon reviewed Aug 13, 2017

View reviewed changes

discard fake field name, apply Option instead

5d71263

viirya reviewed Aug 15, 2017

View reviewed changes

added unit test that mixes constant field name and non constant one f…

0078445

…ield name

viirya reviewed Aug 15, 2017

View reviewed changes

use IndexedSeq instead of Array on foldableFieldNames to get better p…

5c69df5

…erformance

viirya reviewed Aug 15, 2017

View reviewed changes

remove redundant comment and move toIndexedSeq after the map

ab16929

HyukjinKwon approved these changes Aug 15, 2017

View reviewed changes

gatorsmile reviewed Aug 15, 2017

View reviewed changes

move test case from JsonSuite to JsonExpressionsSuite and add new tes…

e0e0c74

…t case in json-functions.sql

gatorsmile reviewed Aug 16, 2017

View reviewed changes

viirya reviewed Aug 17, 2017

View reviewed changes

consistent style in SQL and append the corresponding golden file of j…

5191ed4

…son-functions.sql

HyukjinKwon reviewed Aug 17, 2017

View reviewed changes

add a newline at the end and alter SQL

ff3b9da

gatorsmile reviewed Aug 17, 2017

View reviewed changes

asfgit closed this in 7ab9518 Aug 17, 2017

[SPARK-21677][SQL] json_tuple throws NullPointException when column is null as string type #18930

[SPARK-21677][SQL] json_tuple throws NullPointException when column is null as string type #18930

Uh oh!

Conversation

jmchung commented Aug 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

jmchung commented Aug 13, 2017

Uh oh!

viirya commented Aug 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 13, 2017

Uh oh!

SparkQA commented Aug 13, 2017

Uh oh!

viirya commented Aug 13, 2017

Uh oh!

SparkQA commented Aug 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 14, 2017

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 15, 2017

Uh oh!

SparkQA commented Aug 15, 2017

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!

viirya commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon left a comment

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!

viirya Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 15, 2017

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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Aug 14, 2017 •

edited

Loading

viirya commented Aug 15, 2017 •

edited

Loading

viirya Aug 16, 2017 •

edited

Loading