[SPARK-25252][SQL] Support arrays of any types by to_json #22226

MaxGekk · 2018-08-24T19:29:04Z

What changes were proposed in this pull request?

In the PR, I propose to extended to_json and support any types as element types of input arrays. It should allow converting arrays of primitive types and arrays of arrays. For example:

select to_json(array('1','2','3'))
> ["1","2","3"]
select to_json(array(array(1,2,3),array(4)))
> [[1,2,3],[4]]

How was this patch tested?

Added a couple sql tests for arrays of primitive type and of arrays. Also I added round trip test from_json -> to_json.

SparkQA · 2018-08-24T21:50:56Z

Test build #95227 has finished for PR 22226 at commit e9a5f50.

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

MaxGekk · 2018-08-24T22:21:20Z

@HyukjinKwon Please, have a look at the PR.

SparkQA · 2018-08-25T02:26:10Z

Test build #95235 has finished for PR 22226 at commit c1755de.

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

gatorsmile · 2018-08-25T06:15:00Z

cc @dongjoon-hyun Try to review this PR?

maropu · 2018-08-27T09:12:51Z

@MaxGekk btw, why did you attach this pr to the resolved jira? follow-up?

MaxGekk · 2018-08-27T09:19:58Z

@maropu The JIRA ticket was about both to_json and from_json originally.

maropu · 2018-08-27T09:25:39Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

@@ -65,6 +66,8 @@ private[sql] class JacksonGenerator(
      (arr: SpecializedGetters, i: Int) => {
        writeObject(writeMapData(arr.getMap(i), mt, mapElementWriter))
      }
+    case _ => throw new UnsupportedOperationException(
+      s"Initial type ${dataType.catalogString} must be an array, a struct or a map")


nit: s"Initial type ${dataType.catalogString} must be an array, a struct or a map") -> s"Initial type ${dataType.catalogString} must be an ${ArrayType.simpleString}, a ${StructType.simpleString} or a ${MapType.simpleString}")?

Changed here and above

maropu · 2018-08-27T09:31:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonUtils.scala

-    def verifyType(name: String, dataType: DataType): Unit = dataType match {
-      case NullType | BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType |
-           DoubleType | StringType | TimestampType | DateType | BinaryType | _: DecimalType =>
+  def verifyType(name: String, dataType: DataType): Unit = dataType match {


I extracted it to use outside

maropu · 2018-08-27T09:32:37Z

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

+-- to_json - array type
+select to_json(array('1','2','3'));
+select to_json(array(array(1,2,3),array(4)));
+


super nit: add space afiter ,, e.g., select to_json(array('1', '2', '3'));

maropu · 2018-08-27T09:39:48Z

Probably, you'd be better to file separate jira for each function.

HyukjinKwon · 2018-08-27T14:54:01Z

+1 for separate JIRA.

MaxGekk · 2018-08-27T17:35:36Z

Probably, you'd be better to file separate jira for each function.
+1 for separate JIRA.

I created the JIRA ticket: https://issues.apache.org/jira/browse/SPARK-25252

SparkQA · 2018-08-27T19:11:28Z

Test build #95291 has finished for PR 22226 at commit 906a301.

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

HyukjinKwon · 2018-08-28T03:55:11Z

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

@@ -613,8 +613,7 @@ case class JsonToStructs(
 }

 /**
- * Converts a [[StructType]], [[ArrayType]] of [[StructType]]s, [[MapType]]
- * or [[ArrayType]] of [[MapType]]s to a json output string.
+ * Converts a [[StructType]], [[ArrayType]] or [[MapType]] to a json output string.


not a big deal but JSON while we are here

HyukjinKwon · 2018-08-28T03:55:18Z

python/pyspark/sql/functions.py

    into a JSON string. Throws an exception, in the case of an unsupported type.

-    :param col: name of column containing the struct, array of the structs, the map or
-        array of the maps.
+    :param col: name of column containing a struct, an array or a map.
    :param options: options to control converting. accepts the same options as the json datasource


HyukjinKwon · 2018-08-28T03:55:38Z

Seems okay but I or someone else should take a closer look before getting this in.

SparkQA · 2018-08-28T10:08:33Z

Test build #95331 has finished for PR 22226 at commit e91a33c.

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

SparkQA · 2018-08-28T15:31:47Z

Test build #95347 has finished for PR 22226 at commit 2c3a641.

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

BryanCutler · 2018-08-30T19:35:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

@@ -28,7 +27,7 @@ import org.apache.spark.sql.catalyst.util.{ArrayData, DateTimeUtils, MapData}
 import org.apache.spark.sql.types._

 /**
- * `JackGenerator` can only be initialized with a `StructType` or a `MapType`.
+ * `JackGenerator` can only be initialized with a `StructType`, a `MapType` ot `ArrayType`.


BryanCutler · 2018-08-30T19:38:41Z

R/pkg/R/functions.R

-#' \code{to_json}: Converts a column containing a \code{structType}, array of \code{structType},
-#' a \code{mapType} or array of \code{mapType} into a Column of JSON string.
+#' \code{to_json}: Converts a column containing a \code{structType}, a \code{mapType}
+#' or an array into a Column of JSON string.


does \code{arrayType} work here?

I am not sure

it should
could we add some tests for this in R?

Let's add one simple python doctest as well

I added tests for Python and R. Please, take a look at them.

BryanCutler · 2018-08-30T19:41:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

-  require(dataType.isInstanceOf[StructType] || dataType.isInstanceOf[MapType],
+  // `JackGenerator` can only be initialized with a `StructType`, a `MapType` or a `ArrayType`.
+  require(dataType.isInstanceOf[StructType] || dataType.isInstanceOf[MapType]
+    || dataType.isInstanceOf[ArrayType],
    s"JacksonGenerator only supports to be initialized with a ${StructType.simpleString} " +


maybe need a , between ${StructType.simpleString} and ${MapType.simpleString}?

BryanCutler · 2018-08-30T19:42:44Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

   * Throws an exception, in the case of an unsupported type.
   *
-   * @param e a column containing a struct or array of the structs.
+   * @param e a column containing a struct, a array or a map.


typo: a array -> an array

BryanCutler · 2018-08-30T19:44:30Z

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

@@ -469,4 +469,53 @@ class JsonFunctionsSuite extends QueryTest with SharedSQLContext {

    checkAnswer(sql("""select json[0] from jsonTable"""), Seq(Row(null)))
  }
+
+  test("to_json - array of primitive type") {


typo: primitive type -> primitive types

SparkQA · 2018-08-31T02:11:11Z

Test build #95496 has finished for PR 22226 at commit 2f73c9c.

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

SparkQA · 2018-09-01T14:01:13Z

Test build #95575 has finished for PR 22226 at commit ce3b17c.

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

HyukjinKwon

Looks good. Let me take another look before getting this in.

HyukjinKwon · 2018-09-03T04:04:19Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonGenerator.scala

  }

  // `ValueWriter` for array data storing rows of the schema.
  private lazy val arrElementWriter: ValueWriter = dataType match {
+    case at: ArrayType => makeWriter(at.elementType)
    case st: StructType =>


Can we do case _: StructType | _: MapType => makeWriter(dataType)?

HyukjinKwon · 2018-09-03T04:06:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonUtils.scala

-    def verifyType(name: String, dataType: DataType): Unit = dataType match {
-      case NullType | BooleanType | ByteType | ShortType | IntegerType | LongType | FloatType |
-           DoubleType | StringType | TimestampType | DateType | BinaryType | _: DecimalType =>
+  def verifyType(name: String, dataType: DataType): Unit = dataType match {


We can do:

def verifyType(name: String, dataType: DataType): Unit = { dataType match { case ... } }

to reduce the diff.

SparkQA · 2018-09-03T18:23:20Z

Test build #95622 has finished for PR 22226 at commit 72d2628.

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

HyukjinKwon · 2018-09-04T09:14:47Z

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

-    case mt: MapType => mt
-    case ArrayType(mt: MapType, _) => mt
-  }
+  lazy val rowSchema = child.dataType


nit: rowSchema -> intputSchema. I named this to rowSchema because it was always the schema for the row itself. Now, it seems can be other types as well.

Let's make it val.

I tried to remove lazy and got many errors on tests like:

Invalid call to dataType on unresolved object, tree: 'a org.apache.spark.sql.catalyst.analysis.UnresolvedException: Invalid call to dataType on unresolved object, tree: 'a at org.apache.spark.sql.catalyst.analysis.UnresolvedAttribute.dataType(unresolved.scala:105) at org.apache.spark.sql.catalyst.expressions.StructsToJson.<init>(jsonExpressions.scala:665)

If you don't mind, I will keep it lazy.

HyukjinKwon · 2018-09-04T09:18:12Z

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

-      case ArrayType(_: MapType, _) =>
-        (arr: Any) =>
-          gen.write(arr.asInstanceOf[ArrayData])
-          getAndReset()
    }
  }

  override def dataType: DataType = StringType

  override def checkInputDataTypes(): TypeCheckResult = child.dataType match {


nit: child.dataType -> inputSchema

HyukjinKwon · 2018-09-04T09:18:39Z

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

    }
  }

  override def dataType: DataType = StringType

  override def checkInputDataTypes(): TypeCheckResult = child.dataType match {
-    case _: StructType | ArrayType(_: StructType, _) =>
+    case _: StructType =>


nit: case _: StructType and use it instead of rowSchema.asInstanceOf[StructType].

HyukjinKwon · 2018-09-04T09:19:01Z

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

      try {
        JacksonUtils.verifySchema(rowSchema.asInstanceOf[StructType])
        TypeCheckResult.TypeCheckSuccess
      } catch {
        case e: UnsupportedOperationException =>
          TypeCheckResult.TypeCheckFailure(e.getMessage)
      }
-    case _: MapType | ArrayType(_: MapType, _) =>
+    case _: MapType =>


nit case mapType: Maptype => and use it below likewise.

HyukjinKwon · 2018-09-04T09:22:10Z

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

@@ -685,33 +679,29 @@ case class StructsToJson(
        (row: Any) =>


child.dataType -> intputSchema

HyukjinKwon · 2018-09-04T09:35:22Z

R/pkg/R/functions.R

-#' \code{to_json}: Converts a column containing a \code{structType}, array of \code{structType},
-#' a \code{mapType} or array of \code{mapType} into a Column of JSON string.
+#' \code{to_json}: Converts a column containing a \code{structType}, a \code{mapType}
+#' or an array into a Column of JSON string.


\code{arrayType}. It seems missed.

HyukjinKwon

LGTM otherwise

SparkQA · 2018-09-04T15:31:19Z

Test build #95671 has finished for PR 22226 at commit 90c9687.

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

HyukjinKwon · 2018-09-06T04:35:17Z

Merged to master.

MaxGekk added 2 commits August 24, 2018 20:57

Support arrays of any types in to_json

fb72726

sql tests for array of primitive type and of arrays

e9a5f50

Revert the struct and map types for backward compatibility

c1755de

maropu reviewed Aug 27, 2018

View reviewed changes

MaxGekk added 2 commits August 27, 2018 16:28

Changing array to ArrayType.simpleString and so for map and struct

db9a358

Minor changes - adding gaps

906a301

MaxGekk changed the title ~~[SPARK-24391][SQL] Support arrays of any types by to_json~~ [SPARK-25252][SQL] Support arrays of any types by to_json Aug 27, 2018

HyukjinKwon reviewed Aug 28, 2018

View reviewed changes

json -> JSON

e91a33c

Fix SQL tests: json -> JSON

2c3a641

BryanCutler reviewed Aug 30, 2018

View reviewed changes

Addressing Bryan's review comments

2f73c9c

MaxGekk added 2 commits September 1, 2018 11:01

Testing arrays in to_json for PySpark

252c924

Tests for R

ce3b17c

HyukjinKwon reviewed Sep 3, 2018

View reviewed changes

MaxGekk added 2 commits September 3, 2018 11:37

Minor refactoring to reduce diff

7d1cde4

Code simplification for maps and structs as elements of arrays

72d2628

HyukjinKwon reviewed Sep 4, 2018

View reviewed changes

HyukjinKwon approved these changes Sep 4, 2018

View reviewed changes

Addressing Hyukjin Kwon's review comments

90c9687

asfgit closed this in d749d03 Sep 6, 2018

MaxGekk deleted the to_json-array branch August 17, 2019 13:33

		@@ -685,33 +679,29 @@ case class StructsToJson(
		(row: Any) =>

[SPARK-25252][SQL] Support arrays of any types by to_json #22226

[SPARK-25252][SQL] Support arrays of any types by to_json #22226

Conversation

MaxGekk commented Aug 24, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Aug 24, 2018

MaxGekk commented Aug 24, 2018

SparkQA commented Aug 25, 2018

gatorsmile commented Aug 25, 2018

maropu commented Aug 27, 2018 • edited Loading

MaxGekk commented Aug 27, 2018

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

maropu commented Aug 27, 2018

HyukjinKwon commented Aug 27, 2018

MaxGekk commented Aug 27, 2018

SparkQA commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Aug 28, 2018

SparkQA commented Aug 28, 2018

SparkQA commented Aug 28, 2018

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

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 Aug 31, 2018

SparkQA commented Sep 1, 2018

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 3, 2018

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

Choose a reason for hiding this comment

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

SparkQA commented Sep 4, 2018

HyukjinKwon commented Sep 6, 2018

maropu commented Aug 27, 2018 •

edited

Loading