[SPARK-19595][SQL] Support json array in from_json #16929

HyukjinKwon · 2017-02-14T16:41:52Z

What changes were proposed in this pull request?

This PR proposes to both,

Do not allow json arrays with multiple elements and return null in from_json with StructType as the schema.

Currently, it only reads the single row when the input is a json array. So, the codes below:

import org.apache.spark.sql.functions._
import org.apache.spark.sql.types._
val schema = StructType(StructField("a", IntegerType) :: Nil)
Seq(("""[{"a": 1}, {"a": 2}]""")).toDF("struct").select(from_json(col("struct"), schema)).show()

prints

+--------------------+
|jsontostruct(struct)|
+--------------------+
|                 [1]|
+--------------------+

This PR simply suggests to print this as null if the schema is StructType and input is json array.with multiple elements

+--------------------+
|jsontostruct(struct)|
+--------------------+
|                null|
+--------------------+

Support json arrays in from_json with ArrayType as the schema.

import org.apache.spark.sql.functions._
import org.apache.spark.sql.types._
val schema = ArrayType(StructType(StructField("a", IntegerType) :: Nil))
Seq(("""[{"a": 1}, {"a": 2}]""")).toDF("array").select(from_json(col("array"), schema)).show()

prints

+-------------------+
|jsontostruct(array)|
+-------------------+
|         [[1], [2]]|
+-------------------+

How was this patch tested?

Unit test in JsonExpressionsSuite, JsonFunctionsSuite, Python doctests and manual test.

HyukjinKwon · 2017-02-14T16:53:50Z

Cc @hvanhovell, could you please take a look and see if this makes sense?

SparkQA · 2017-02-14T19:08:14Z

Test build #72878 has finished for PR 16929 at commit acbce26.

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

zsxwing · 2017-02-14T21:47:49Z

also cc @marmbrus

marmbrus · 2017-02-14T22:01:36Z

I agree that its wrong to truncate, but why not just fix handling of arrays rather than disallow it?

HyukjinKwon · 2017-02-14T22:54:56Z

Sure, let me turn it to suuport. I thought disallowing was kind of a safe choice :)..

HyukjinKwon · 2017-02-18T18:23:36Z

@hvanhovell, @zsxwing and @marmbrus, I just updated and rebased. Could you take another look please?

SparkQA · 2017-02-18T20:19:36Z

Test build #73110 has finished for PR 16929 at commit 25cdd7d.

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

SparkQA · 2017-02-18T20:26:00Z

Test build #73113 has finished for PR 16929 at commit c7b5f2e.

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

SparkQA · 2017-02-18T20:30:22Z

Test build #73111 has finished for PR 16929 at commit 8122314.

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

SparkQA · 2017-02-18T21:19:44Z

Test build #73114 has finished for PR 16929 at commit d6fd39b.

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

HyukjinKwon · 2017-02-24T10:25:02Z

Hi @marmbrus, does this sounds good to you?

marmbrus · 2017-02-24T19:58:50Z

Hmm, I'm not sure we want to change this to a generator. I think that has performance consequences as well as possibly being surprising. I would probably make it possible to handle arrays (when the correct schema is given). If they want to explode they can run explode(from_json(...)).

marmbrus · 2017-02-24T19:59:01Z

/cc @brkyvz

HyukjinKwon · 2017-02-24T22:39:06Z

Sure, let me take a look and try.

HyukjinKwon · 2017-02-25T12:28:04Z

Let me clean up and fix the tests if failed with an updated PR description soon. It is still a wip.

SparkQA · 2017-02-25T13:27:01Z

Test build #73467 has finished for PR 16929 at commit 8c48436.

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

HyukjinKwon · 2017-02-26T04:10:11Z

Hi @marmbrus, @brkyvz and @hvanhovell, I think it is ready for a review.

SparkQA · 2017-02-26T05:38:55Z

Test build #73474 has finished for PR 16929 at commit 25086ed.

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

SparkQA · 2017-02-26T05:50:44Z

Test build #73475 has finished for PR 16929 at commit 2aaf609.

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

HyukjinKwon · 2017-02-28T04:22:54Z

Then, let me fix this as below:

from_json with StructType
- JSON array -> null
- JSON object -> Row(...)
from_json with ArrayType
- JSON array -> Array(Row(...), ...)
- JSON object -> Array(Row(...))

exposed APIs

def from_json(..., schema: StructType, ...) = ...
def from_json(..., schema: DataType, ...) = ...

SparkQA · 2017-02-28T06:22:31Z

Test build #73570 has started for PR 16929 at commit 72d6410.

SparkQA · 2017-02-28T06:32:38Z

Test build #73572 has started for PR 16929 at commit 54e60bb.

SparkQA · 2017-02-28T06:37:34Z

Test build #73573 has started for PR 16929 at commit 9f1e966.

HyukjinKwon · 2017-02-28T06:51:38Z

@brkyvz, @marmbrus - I think it is ready for another look. Could you see if I understood your comments correctly?

SparkQA · 2017-02-28T06:52:41Z

Test build #73574 has started for PR 16929 at commit 0c088bf.

HyukjinKwon · 2017-02-28T23:43:59Z

retest this please

SparkQA · 2017-03-01T02:08:56Z

Test build #73621 has finished for PR 16929 at commit 0c088bf.

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

brkyvz · 2017-03-03T22:11:54Z

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

+  @transient
+  lazy val converter = schema match {
+    case _: StructType =>
+      (rows: Seq[InternalRow]) => if (rows.length == 1) rows.head else null


this breaks previous behavior. I would still return the first element if rows.length > 1. Feel free to push back. Also wonder what @marmbrus thinks

I'm okay breaking previous behavior because I'd call truncating an array a bug.

We should list this in the release notes though (i.e. go tag the JIRA).

HyukjinKwon · 2017-03-04T02:11:36Z

@mambrus and @brkyvz, would there be other things I should double check?

brkyvz · 2017-03-04T03:47:36Z

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

+      InternalRow.fromSeq(1 :: Nil) ::
+      InternalRow.fromSeq(2 :: Nil) :: Nil
+    checkEvaluation(JsonToStruct(
+      schema, Map.empty, Literal(jsonData1), gmtId), expected)


could you put input and expected output in different rows for readability please

brkyvz · 2017-03-04T03:50:17Z

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

+      schema, Map.empty, Literal(jsonData1), gmtId), expected)
+
+    // json object: `Array(Row(...))`
+    val jsonData2 = """{"a": 1}"""


I would make each example a separate test. This way it's easier to figure out what breaks later.

e.g.

from_json - input=array, schema=array, output=array

from_json - input=object, schema=array, output=array of single object

from_json - input=empty json array, schema=array, output=empty array
...

brkyvz · 2017-03-04T03:51:52Z

@HyukjinKwon Implementation seems fine. Just left a cosmetic comment on your unit tests. Otherwise LGTM

HyukjinKwon · 2017-03-04T04:04:14Z

Thank you so much. Let me clean up.

SparkQA · 2017-03-04T06:35:30Z

Test build #73893 has finished for PR 16929 at commit 3d490e3.

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

felixcheung · 2017-03-05T20:33:29Z

should the ~~title~~ description of this PR be updated to reflect that we should support array of json?

HyukjinKwon · 2017-03-05T21:57:42Z

I just updated the PR description to prevent confusion.

HyukjinKwon · 2017-03-05T22:27:54Z

Cc @brkyvz and @marmbrus could this be merged by any chance?

brkyvz · 2017-03-05T22:30:52Z

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

@@ -372,6 +372,62 @@ class JsonExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
    )
  }

+  test("from_json - input=array, schema=array, output=array") {


these are great! thanks!

brkyvz · 2017-03-05T22:33:52Z

Merging to master. Thanks!

HyukjinKwon · 2017-03-05T22:40:42Z

Thank you @brkyvz.

HyukjinKwon changed the title ~~[SPARK-19595][SQL] Do not allow json array in from_json~~ [WIP][SPARK-19595][SQL] Do not allow json array in from_json Feb 14, 2017

HyukjinKwon force-pushed the disallow-array branch from acbce26 to 7f07acf Compare February 18, 2017 18:17

HyukjinKwon changed the title ~~[WIP][SPARK-19595][SQL] Do not allow json array in from_json~~ [SPARK-19595][SQL] Support json array in from_json Feb 18, 2017

HyukjinKwon force-pushed the disallow-array branch 2 times, most recently from ef783bd to 25cdd7d Compare February 18, 2017 18:20

HyukjinKwon changed the title ~~[SPARK-19595][SQL] Support json array in from_json~~ [WIP][SPARK-19595][SQL] Support json array in from_json Feb 25, 2017

HyukjinKwon force-pushed the disallow-array branch from d6fd39b to 8c48436 Compare February 25, 2017 12:18

HyukjinKwon changed the title ~~[WIP][SPARK-19595][SQL] Support json array in from_json~~ [SPARK-19595][SQL] Support json array in from_json Feb 26, 2017

Support ArrayType for json array

a0a7091

HyukjinKwon force-pushed the disallow-array branch from 2aaf609 to a0a7091 Compare February 26, 2017 04:09

Minor cleanups

ef007f9

Permissively support array in from_json and make a one for DataType

72d6410

Revert unrelated newline change and cleanup comments

54e60bb

Relocate test

9f1e966

Deduplicate type handling

0c088bf

brkyvz reviewed Mar 3, 2017

View reviewed changes

brkyvz reviewed Mar 4, 2017

View reviewed changes

Address comments and clean up tests

3d490e3

brkyvz reviewed Mar 5, 2017

View reviewed changes

asfgit closed this in 369a148 Mar 5, 2017

HyukjinKwon deleted the disallow-array branch January 2, 2018 03:44

MaxGekk mentioned this pull request Jul 22, 2018

[SPARK-24391][SQL] Support arrays of any types by from_json #21439

Closed

[SPARK-19595][SQL] Support json array in from_json #16929

[SPARK-19595][SQL] Support json array in from_json #16929

Conversation

HyukjinKwon commented Feb 14, 2017 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

HyukjinKwon commented Feb 14, 2017

SparkQA commented Feb 14, 2017

zsxwing commented Feb 14, 2017

marmbrus commented Feb 14, 2017

HyukjinKwon commented Feb 14, 2017 • edited Loading

HyukjinKwon commented Feb 18, 2017

SparkQA commented Feb 18, 2017

SparkQA commented Feb 18, 2017

SparkQA commented Feb 18, 2017

SparkQA commented Feb 18, 2017

HyukjinKwon commented Feb 24, 2017

marmbrus commented Feb 24, 2017

marmbrus commented Feb 24, 2017

HyukjinKwon commented Feb 24, 2017

HyukjinKwon commented Feb 25, 2017

SparkQA commented Feb 25, 2017

HyukjinKwon commented Feb 26, 2017

SparkQA commented Feb 26, 2017

SparkQA commented Feb 26, 2017

HyukjinKwon commented Feb 28, 2017 • edited Loading

SparkQA commented Feb 28, 2017

SparkQA commented Feb 28, 2017

SparkQA commented Feb 28, 2017

HyukjinKwon commented Feb 28, 2017

SparkQA commented Feb 28, 2017

HyukjinKwon commented Feb 28, 2017

SparkQA commented Mar 1, 2017

brkyvz Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

marmbrus Mar 3, 2017

Choose a reason for hiding this comment

marmbrus Mar 3, 2017

Choose a reason for hiding this comment

HyukjinKwon commented Mar 4, 2017

brkyvz Mar 4, 2017

Choose a reason for hiding this comment

brkyvz Mar 4, 2017

Choose a reason for hiding this comment

brkyvz commented Mar 4, 2017

HyukjinKwon commented Mar 4, 2017

SparkQA commented Mar 4, 2017

felixcheung commented Mar 5, 2017 • edited Loading

HyukjinKwon commented Mar 5, 2017

HyukjinKwon commented Mar 5, 2017

brkyvz Mar 5, 2017

Choose a reason for hiding this comment

brkyvz commented Mar 5, 2017

HyukjinKwon commented Mar 5, 2017

HyukjinKwon commented Feb 14, 2017 •

edited

Loading

HyukjinKwon commented Feb 14, 2017 •

edited

Loading

HyukjinKwon commented Feb 28, 2017 •

edited

Loading

brkyvz Mar 3, 2017 •

edited

Loading

felixcheung commented Mar 5, 2017 •

edited

Loading