[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
GitHub user MaxGekk opened a pull request: https://github.com/apache/spark/pull/21439 [SPARK-24391][SQL] Support arrays of any types by from_json ## What changes were proposed in this pull request? The PR removes a restriction for element types of array type which exists in `from_json` for the root type. Currently, the function can handle only arrays of structs. Even array of primitive types is disallowed. The PR allows arrays of any types currently supported by JSON datasource. Here is an example of an array of a primitive type: ``` scala> import org.apache.spark.sql.functions._ scala> val df = Seq("[1, 2, 3]").toDF("a") scala> val schema = new ArrayType(IntegerType, false) scala> val arr = df.select(from_json($"a", schema)) scala> arr.printSchema root |-- jsontostructs(a): array (nullable = true) ||-- element: integer (containsNull = true) ``` and result of converting of the json string to the `ArrayType`: ``` scala> arr.show ++ |jsontostructs(a)| ++ | [1, 2, 3]| ++ ``` ## How was this patch tested? I added a few positive and negative tests: - array of primitive types - array of arrays - array of structs - array of maps You can merge this pull request into a Git repository by running: $ git pull https://github.com/MaxGekk/spark-1 from_json-array Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21439.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21439 commit 3a7559b8809757ba32491a9c882ec40c8986c3b0 Author: Maxim Gekk Date: 2018-05-26T16:46:08Z Support arrays by from_json commit b601a9365e12744c408a8c198a94a5c6a9e4607e Author: Maxim Gekk Date: 2018-05-26T17:08:49Z Fix comments --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r191298844 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -523,6 +523,8 @@ case class JsonToStructs( // can generate incorrect files if values are missing in columns declared as non-nullable. val nullableSchema = if (forceNullableSchema) schema.asNullable else schema + val unpackArray: Boolean = options.get("unpackArray").map(_.toBoolean).getOrElse(false) --- End diff -- private? (This is not related to this pr though, `nullableSchema` also can be private?) --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r191298921 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -523,6 +523,8 @@ case class JsonToStructs( // can generate incorrect files if values are missing in columns declared as non-nullable. val nullableSchema = if (forceNullableSchema) schema.asNullable else schema + val unpackArray: Boolean = options.get("unpackArray").map(_.toBoolean).getOrElse(false) --- End diff -- Can you make the option `unpackArray` case-insensitive? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r191299100 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -523,6 +523,8 @@ case class JsonToStructs( // can generate incorrect files if values are missing in columns declared as non-nullable. val nullableSchema = if (forceNullableSchema) schema.asNullable else schema + val unpackArray: Boolean = options.get("unpackArray").map(_.toBoolean).getOrElse(false) --- End diff -- If we add this new option here, I feel we'd be better to document somewhere (e.g., `sq/functions.scala`) --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204226404 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -61,6 +61,7 @@ class JacksonParser( dt match { case st: StructType => makeStructRootConverter(st) case mt: MapType => makeMapRootConverter(mt) + case at: ArrayType => makeArrayRootConverter(at) --- End diff -- This change accepts the json datasource form that the master can't parse? If so, I think we need tests in `JsonSuite`, too. cc: @HyukjinKwon --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204231416 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -61,6 +61,7 @@ class JacksonParser( dt match { case st: StructType => makeStructRootConverter(st) case mt: MapType => makeMapRootConverter(mt) + case at: ArrayType => makeArrayRootConverter(at) --- End diff -- > This change accepts the json datasource form that the master can't parse? Right, it accept arrays of any types comparing to the master which accepts arrays of structs only > If so, I think we need tests in JsonSuite ... I added a few tests to `JsonSuite` or do you mean some concrete test case? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204231723 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -61,6 +61,7 @@ class JacksonParser( dt match { case st: StructType => makeStructRootConverter(st) case mt: MapType => makeMapRootConverter(mt) + case at: ArrayType => makeArrayRootConverter(at) --- End diff -- You've already added tests in `JsonSuite`? It seems there are tests in `JsonFunctionsSuite`, `JsonExpressionSuite`, and `SQLQueryTestSuite` now? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204232671 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -61,6 +61,7 @@ class JacksonParser( dt match { case st: StructType => makeStructRootConverter(st) case mt: MapType => makeMapRootConverter(mt) + case at: ArrayType => makeArrayRootConverter(at) --- End diff -- Sorry, I didn't catch you meant specific class. Just in case, what is the reason for adding tests to `JsonSuite`? I changed behavior of a function, so, `JsonFunctionsSuite` is perfect place for new tests, I believe. At the moment, you cannot specify a schema of json objects different from `StructType` in `DataFrameReader`. In this way, the `ArrayType` can come into `JacksonParser` only from json functions (`from_json`). I can add similar test to `JsonSuite` as in `JsonFunctionsSuite` but it doesn't make any sense from my point of view. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204233568 --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala --- @@ -423,7 +423,9 @@ class JsonExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper with val input = """{"a": 1}""" val schema = ArrayType(StructType(StructField("a", IntegerType) :: Nil)) val output = InternalRow(1) :: Nil -checkEvaluation(JsonToStructs(schema, Map.empty, Literal(input), gmtId), output) +checkEvaluation( + JsonToStructs(schema, Map.empty, Literal(input), gmtId), + output) --- End diff -- Looks unrelated change. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204233840 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -61,6 +61,7 @@ class JacksonParser( dt match { case st: StructType => makeStructRootConverter(st) case mt: MapType => makeMapRootConverter(mt) + case at: ArrayType => makeArrayRootConverter(at) --- End diff -- ah, ok. You touched the the `JacksonParser.scala` file, so I though there were some behaivour changes in the json datasorce. But I notice that there are not, so the current tests are enough. Thanks. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204248469 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- why do we need this? This means that if the user asks for an array of data and the data doesn't contain an array we return an array with a single element. This seems wrong to me. I'd rather return null or throw an exception. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204250446 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- This is for backward compatibility to the behavior introduced by this PR: https://github.com/apache/spark/pull/16929, most likely by this: https://github.com/apache/spark/pull/16929/files#diff-6626026091295ad8c0dfb66ecbcd04b1R506 . Even special test was added: https://github.com/apache/spark/pull/16929/files#diff-88230f171af0b7a40791a867f9dd3a36R382 . Please, ask author @HyukjinKwon about the it. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user HyukjinKwon commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204263502 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- If an array of data is empty, and the schema is an array, it should return an empty array, https://github.com/apache/spark/pull/16929/files#diff-88230f171af0b7a40791a867f9dd3a36R389 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204274790 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- I think it is a super weird case...can we put this special handling code for back-compatibility in `JsonToStructs`? Ether way, I think we should leave code comments here to make others understood easily? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user mgaido91 commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204300575 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- I think what was done in that PR is pretty different and it is actually the opposite of what we are doing here. Indeed, there we are returning an array of structs when a struct is specified as schema and the JSON contains an array. Here we are returning an array with one struct when the schema is an array of struct and there is a struct instead of an array. Despite I don't really like the behavior introduced in the PR you mentioned, I can understand it, as it was a way to support array of struct (the only at the moment) and I don't think we can change it before 3.0 at least for backward compatibility. But since here we are introducing a new behavior, if an array is required and a struct is found, I think returning an array with one element is a wrong/unexpected behavior and returning null would be what I'd expect as a user. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204553995 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- I can only say that this `case START_OBJECT` was added to handle the case when an user specified a schema as an `array` of `struct`, and a `struct` is found in the input json. See the existing test case which I pointed out above: https://github.com/apache/spark/pull/16929/files#diff-88230f171af0b7a40791a867f9dd3a36R382 . I don't want to change the behavior in the PR and potentially break user's apps. What I would propose is to put the functionality under a `spark.sql.legacy.*` flag which could be deleted in Spark 3.0. @maropu Initially I put the behavior under a flag in `JsonToStructs` but Reynold asked me to support new behavior without any new sticks/flags. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user viirya commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204915146 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- Shall we add a comment on top of this `case` to explain it? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user viirya commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204931175 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/JsonFunctionsSuite.scala --- @@ -136,12 +136,11 @@ class JsonFunctionsSuite extends QueryTest with SharedSQLContext { test("from_json invalid schema") { --- End diff -- Not a invalid schema now. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user viirya commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204932903 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -544,34 +544,27 @@ case class JsonToStructs( timeZoneId = None) override def checkInputDataTypes(): TypeCheckResult = nullableSchema match { -case _: StructType | ArrayType(_: StructType, _) | _: MapType => +case _: StructType | _: ArrayType | _: MapType => super.checkInputDataTypes() case _ => TypeCheckResult.TypeCheckFailure( s"Input schema ${nullableSchema.catalogString} must be a struct or an array of structs.") --- End diff -- `or an array of structs.` -> `or an array.` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user viirya commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r204933936 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -544,34 +544,27 @@ case class JsonToStructs( timeZoneId = None) --- End diff -- Pleas also update the comment of `JsonToStructs`: `Converts an json input string to a [[StructType]] or [[ArrayType]] of [[StructType]]s`. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r205953494 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,17 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => --- End diff -- I am adding a comment for this. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r192465130 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -548,7 +553,9 @@ case class JsonToStructs( forceNullableSchema = SQLConf.get.getConf(SQLConf.FROM_JSON_FORCE_NULLABLE_SCHEMA)) override def checkInputDataTypes(): TypeCheckResult = nullableSchema match { -case _: StructType | ArrayType(_: StructType, _) | _: MapType => +case ArrayType(_: StructType, _) if unpackArray => --- End diff -- Even if `unpackArray` is `false`, the next branch in line 558 still do `super.checkInputDataTypes()` for any `ArrayType` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r192488365 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,13 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) --- End diff -- In line 87: ``` val array = convertArray(parser, elementConverter) // Here, as we support reading top level JSON arrays and take every element // in such an array as a row, this case is possible. if (array.numElements() == 0) { Nil } else { array.toArray[InternalRow](schema).toSeq } ``` Should we also follow this? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r192501912 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala --- @@ -523,6 +523,11 @@ case class JsonToStructs( // can generate incorrect files if values are missing in columns declared as non-nullable. val nullableSchema = if (forceNullableSchema) schema.asNullable else schema + private val caseInsensitiveOptions = CaseInsensitiveMap(options) + private val unpackArray: Boolean = { --- End diff -- Why do we need this? Can you add comments about it? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gengliangwang commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r192502051 --- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala --- @@ -423,7 +423,9 @@ class JsonExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper with val input = """{"a": 1}""" val schema = ArrayType(StructType(StructField("a", IntegerType) :: Nil)) val output = InternalRow(1) :: Nil -checkEvaluation(JsonToStructs(schema, Map.empty, Literal(input), gmtId, true), output) +checkEvaluation( + JsonToStructs(schema, Map("unpackArray" -> "true"), Literal(input), gmtId, true), --- End diff -- add case for `unpackArray` as `false` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r194491865 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,13 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) --- End diff -- The code in line 87 returns `null` for json input `[]` if schema is `StructType(StructField("a", IntegerType) :: Nil)`. I would explain why we should return `null` in that case: we *extract* struct from the array. If the array is _empty_, it means there is nothing to extract and we returns `null` for the nothing. In case when schema is `ArrayType(...)`, I believe we should return `empty` array for empty JSON array `[]` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gatorsmile commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r209461334 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala --- @@ -101,6 +102,21 @@ class JacksonParser( } } + private def makeArrayRootConverter(at: ArrayType): JsonParser => Seq[InternalRow] = { +val elemConverter = makeConverter(at.elementType) +(parser: JsonParser) => parseJsonToken[Seq[InternalRow]](parser, at) { + case START_ARRAY => Seq(InternalRow(convertArray(parser, elemConverter))) + case START_OBJECT if at.elementType.isInstanceOf[StructType] => +// This handles the case when an input JSON object is a structure but +// the specified schema is an array of structures. In that case, the input JSON is --- End diff -- Could you add an example here, like what we did in `makeStructRootConverter `? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user gatorsmile commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r209461516 --- Diff: sql/core/src/test/resources/sql-tests/inputs/json-functions.sql --- @@ -39,3 +39,8 @@ select from_json('{"a":1, "b":"2"}', 'struct'); -- infer schema of json literal select schema_of_json('{"c1":0, "c2":[1]}'); select from_json('{"c1":[1, 2, 3]}', schema_of_json('{"c1":[0]}')); + +-- from_json - array type +select from_json('[1, 2, 3]', 'array'); --- End diff -- Add more cases ? select from_json('[3, null, 4]', 'array') select from_json('[3, "str", 4]', 'array') --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user MaxGekk commented on a diff in the pull request: https://github.com/apache/spark/pull/21439#discussion_r209468355 --- Diff: sql/core/src/test/resources/sql-tests/inputs/json-functions.sql --- @@ -39,3 +39,8 @@ select from_json('{"a":1, "b":"2"}', 'struct'); -- infer schema of json literal select schema_of_json('{"c1":0, "c2":[1]}'); select from_json('{"c1":[1, 2, 3]}', schema_of_json('{"c1":[0]}')); + +-- from_json - array type +select from_json('[1, 2, 3]', 'array'); --- End diff -- added --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21439: [SPARK-24391][SQL] Support arrays of any types by...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21439 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org