[SPARK-21954][SQL] JacksonUtils should verify MapType's value type instead of key type #19167

viirya · 2017-09-08T08:28:06Z

What changes were proposed in this pull request?

JacksonUtils.verifySchema verifies if a data type can be converted to JSON. For MapType, it now verifies the key type. However, in JacksonGenerator, when converting a map to JSON, we only care about its values and create a writer for the values. The keys in a map are treated as strings by calling toString on the keys.

Thus, we should change JacksonUtils.verifySchema to verify the value type of MapType.

How was this patch tested?

Added tests.

HyukjinKwon · 2017-09-08T09:22:51Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/json/JacksonUtilsSuite.scala

+
+class JacksonUtilsSuite extends SparkFunSuite {
+
+  test("verifySchema") {


@viirya, would you mind if I ask to leave a simple e2e test alone for this PR? This one looks quite a simple fix to me and I think this won't require such many tests alone for this issue. I want to backport this but only leave strictly related changes here.

I think we could do something like SELECT to_json(struct(map(interval 1 second, 'a'))) or SELECT to_json(struct(map('a', interval 1 second))).

OK, I am fine with those test coverage improvement too together. Up to you.

HyukjinKwon · 2017-09-08T09:35:42Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/json/JacksonUtilsSuite.scala

+    val atomicTypes = DataTypeTestUtils.atomicTypes
+    val atomicArrayTypes = atomicTypes.map(ArrayType(_, containsNull = false))
+    val atomicMapTypes = for (keyType <- atomicTypes;
+      valueType <- atomicTypes) yield MapType(keyType, valueType, false)


I'd do this as below:

val atomicMapTypes = for { keyType <- atomicTypes valueType <- atomicTypes } yield MapType(keyType, valueType, false)

HyukjinKwon · 2017-09-08T09:37:08Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/json/JacksonUtilsSuite.scala

+
+class JacksonUtilsSuite extends SparkFunSuite {
+
+  test("verifySchema") {


OK, I am fine with those test coverage improvement too together. Up to you.

HyukjinKwon · 2017-09-08T09:47:01Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/json/JacksonUtilsSuite.scala

+
+    // For MapType, its keys are treated as a string basically when generating JSON, so we only
+    // care if the values are valid for JSON.
+    val alsoValidMapTypes = for (keyType <- atomicTypes ++ invalidTypes;


Would it be possible to move these supported cases into 45L so we loop twice, one for the vaild and one for invalid one?

SparkQA · 2017-09-08T11:08:43Z

Test build #81552 has finished for PR 19167 at commit 760edad.

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

viirya · 2017-09-09T04:44:13Z

@HyukjinKwon Thanks for review. I simplified the test cases. Please take a look when you are available.

HyukjinKwon · 2017-09-09T05:21:11Z

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

@@ -257,6 +257,18 @@ class JsonFunctionsSuite extends QueryTest with SharedSQLContext {
      "A type of keys and values in map() must be string, but got"))
  }

+  test("SPARK-21954: JacksonUtils should verify MapType's value type instead of key type") {


@viirya, how about puting this test around to_json unsupported type here and maybe use Scala function API for consistency?

Modified as suggestion.

HyukjinKwon

LGTM except for a minor comment.

HyukjinKwon · 2017-09-09T06:13:58Z

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

+      .select(struct(map(lit("a"), $"a._1".cast(CalendarIntervalType)).as("col1")).as("c"))
+    checkAnswer(
+      df2.select(to_json($"c")),
+      Row("""{"col1":{"interval -3 months 7 hours":"a"}}""") :: Nil)


val df2 = baseDf .select(struct(map($"a._1".cast(CalendarIntervalType), lit("a")).as("col1")).as("c")) ... checkAnswer( df2.select(to_json($"c")), Row("""{"col1":{"interval -3 months 7 hours":"a"}}""") :: Nil)

This case looks a supported case though. We could maybe make a separate test for this one.

Ok. Moved into a test.

SparkQA · 2017-09-09T06:37:57Z

Test build #81573 has finished for PR 19167 at commit 9bc8b2d.

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

SparkQA · 2017-09-09T06:47:02Z

Test build #81575 has finished for PR 19167 at commit 884c533.

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

SparkQA · 2017-09-09T07:04:45Z

Test build #81582 has finished for PR 19167 at commit fbf8a88.

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

SparkQA · 2017-09-09T07:04:46Z

Test build #81578 has finished for PR 19167 at commit 2cf6bbb.

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

HyukjinKwon · 2017-09-09T07:17:04Z

retest this please

SparkQA · 2017-09-09T10:09:01Z

Test build #81583 has finished for PR 19167 at commit fbf8a88.

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

…stead of key type ## What changes were proposed in this pull request? `JacksonUtils.verifySchema` verifies if a data type can be converted to JSON. For `MapType`, it now verifies the key type. However, in `JacksonGenerator`, when converting a map to JSON, we only care about its values and create a writer for the values. The keys in a map are treated as strings by calling `toString` on the keys. Thus, we should change `JacksonUtils.verifySchema` to verify the value type of `MapType`. ## How was this patch tested? Added tests. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes #19167 from viirya/test-jacksonutils. (cherry picked from commit 6b45d7e) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

HyukjinKwon · 2017-09-09T10:13:48Z

Merged to master and branch-2.2.

viirya · 2017-09-09T12:48:37Z

Thanks @HyukjinKwon

…stead of key type ## What changes were proposed in this pull request? `JacksonUtils.verifySchema` verifies if a data type can be converted to JSON. For `MapType`, it now verifies the key type. However, in `JacksonGenerator`, when converting a map to JSON, we only care about its values and create a writer for the values. The keys in a map are treated as strings by calling `toString` on the keys. Thus, we should change `JacksonUtils.verifySchema` to verify the value type of `MapType`. ## How was this patch tested? Added tests. Author: Liang-Chi Hsieh <viirya@gmail.com> Closes apache#19167 from viirya/test-jacksonutils. (cherry picked from commit 6b45d7e) Signed-off-by: hyukjinkwon <gurwls223@gmail.com>

Add test fro JacksonUtils.

760edad

HyukjinKwon reviewed Sep 8, 2017

View reviewed changes

viirya force-pushed the test-jacksonutils branch from 9bc8b2d to a15bdb7 Compare September 9, 2017 04:43

viirya force-pushed the test-jacksonutils branch from a15bdb7 to 884c533 Compare September 9, 2017 04:45

Address comments.

884c533

HyukjinKwon reviewed Sep 9, 2017

View reviewed changes

Address comment.

2cf6bbb

HyukjinKwon approved these changes Sep 9, 2017

View reviewed changes

Tweak the test.

fbf8a88

asfgit closed this in 6b45d7e Sep 9, 2017

viirya deleted the test-jacksonutils branch December 27, 2023 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-21954][SQL] JacksonUtils should verify MapType's value type instead of key type #19167

[SPARK-21954][SQL] JacksonUtils should verify MapType's value type instead of key type #19167

viirya commented Sep 8, 2017 •

edited

Loading

HyukjinKwon Sep 8, 2017 •

edited

Loading

HyukjinKwon Sep 8, 2017

HyukjinKwon Sep 8, 2017

HyukjinKwon Sep 8, 2017

HyukjinKwon Sep 8, 2017

SparkQA commented Sep 8, 2017

viirya commented Sep 9, 2017

HyukjinKwon Sep 9, 2017

viirya Sep 9, 2017

HyukjinKwon left a comment

HyukjinKwon Sep 9, 2017

viirya Sep 9, 2017

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

HyukjinKwon commented Sep 9, 2017

SparkQA commented Sep 9, 2017

HyukjinKwon commented Sep 9, 2017

viirya commented Sep 9, 2017


		class JacksonUtilsSuite extends SparkFunSuite {

		test("verifySchema") {

[SPARK-21954][SQL] JacksonUtils should verify MapType's value type instead of key type #19167

[SPARK-21954][SQL] JacksonUtils should verify MapType's value type instead of key type #19167

Conversation

viirya commented Sep 8, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

HyukjinKwon Sep 8, 2017 • edited Loading

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

Choose a reason for hiding this comment

SparkQA commented Sep 8, 2017

viirya commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

SparkQA commented Sep 9, 2017

HyukjinKwon commented Sep 9, 2017

SparkQA commented Sep 9, 2017

HyukjinKwon commented Sep 9, 2017

viirya commented Sep 9, 2017

viirya commented Sep 8, 2017 •

edited

Loading

HyukjinKwon Sep 8, 2017 •

edited

Loading