[SPARK-19828][R] Support array type in from_json in R #17178

HyukjinKwon · 2017-03-06T11:57:17Z

What changes were proposed in this pull request?

Since we could not directly define the array type in R, this PR proposes to support array types in R as string types that are used in structField as below:

jsonArr <- "[{\"name\":\"Bob\"}, {\"name\":\"Alice\"}]"
df <- as.DataFrame(list(list("people" = jsonArr)))
collect(select(df, alias(from_json(df$people, schema, asJsonArray = TRUE), "arrcol")))

prints

      arrcol
1 Bob, Alice

How was this patch tested?

Unit tests in test_sparkSQL.R.

HyukjinKwon · 2017-03-06T12:00:30Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

+
+  schemas <- list(structType(structField("age", "integer"), structField("height", "double")),
+                  "struct<age:integer,height:double>")
+  for (schema in schemas) {


I re-used the existing tests codes with the loop below:

- df <- as.DataFrame(j) - schema <- structType(structField("age", "integer"), - structField("height", "double")) + schemas <- list(structType(structField("age", "integer"), structField("height", "double")), + "struct<age:integer,height:double>") + for (schema in schemas) { + df <- as.DataFrame(j) ... + }

HyukjinKwon · 2017-03-06T12:01:28Z

R/pkg/R/functions.R

            column(jc)
          })

+setClassUnion("characterOrstructType", c("character", "structType"))


Can I use a class union here in this file?

HyukjinKwon · 2017-03-06T12:02:02Z

@felixcheung, this is a bit different with what we talked but I opened this because I thought you might like this more. This can take the type string given to structField now. If you are worried of this, let me change it back to the optional parameter one.

Could you check if this makes sense to you?

SparkQA · 2017-03-06T12:33:38Z

Test build #73997 has finished for PR 17178 at commit 10feb9d.

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

HyukjinKwon · 2017-03-06T23:34:50Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

+  # check if array type in string is correctly supported.
+  jsonArr <- "[{\"name\":\"Bob\"}, {\"name\":\"Alice\"}]"
+  df <- as.DataFrame(list(list("people" = jsonArr)))
+  arr <- collect(select(df, alias(from_json(df$people, "array<struct<name:string>>"), "arrcol")))


(Just in case, from_json takes both DataType and json string from DataType.json. In that sense, I thought it'd be nicer if it takes what structField takes in R)

felixcheung · 2017-03-07T07:06:02Z

R/pkg/inst/tests/testthat/test_sparkSQL.R

-  expect_equal(collect(select(df, from_json(df$a, schema)))[[1]][[1]], NA)
+
+  schemas <- list(structType(structField("age", "integer"), structField("height", "double")),
+                  "struct<age:integer,height:double>")


I wonder if this is too loosely-typed and format hard to explain/illustrate to R user
struct<age:integer,height:double>

Yes.. I persuaded myself that it is okay because it's a valid type string for structField. If you prefer optional parameter one, I could try. Otherwise, let me close this for now If you are not sure of both ways :).

it might be ok if this is well documented and well checked.
checkType in schema.R is strangely largely unused or untested though.

Let me give a shot at my best.

oh to clarify

"it might be ok if this is well documented and well checked. "-> if it's something we could formally document it might be ok

"checkType in schema.R is strangely largely unused or untested though. "-> I don't know the string specification is actually accidental - I see no use of it outside of checkType and it's not referenced anywhere. It's definitely not being tested as well

Actually, on 2nd thought, I recall there's a JIRA on accepting the JSON string as schema that is supported on Scala side. That might be a better way to go if we are to take a string, instead of inventing our own format. But that could be a bigger change.

Hm, @felixcheung, I think this resembles catalog string, maybe we could reuse CatalystSqlParser.parseDataType to make this more formal and to do not duplicate the efforts for defining a format or documentation. This might be a big change but if this is what we want in the future, I would like to argue that we should keep this way.

For JSON string schema, there is an overloaded version of from_json that takes that schema string. If we are going to expose it, I think it can be easily done.

However, I think you meant it is a bigger change because we need to provide a way to produce this JSON string from types. Up to my knowledge, we can only manually specify the type via this calalog string in R. Is this true? If so, I don't have a good idea for now to support this and I would rather close this if you so as well.

If it is a format that is documented and globally defined in Spark, it shouldn't be a problem to use that. If a variant of from_json is already taking a schema string, we should be able call it directly from R without having to make it public, although it might make sense to do so, if, say, read.format("json") supports it too.

I'm not super worry about having a way to produce JSON schema string for the R user - we would just accept such JSON schema string and assume the user can create it.

But yea, overall this feels very heavy-weighed to support multiple JSON objects in a column value. How about just take a bool flag like we started with? :)

Doh, I meant there is a public from_json that takes JSON string schema -

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

Lines 3061 to 3062 in 369a148

def from_json(e: Column, schema: String, options: java.util.Map[String, String]): Column =

from_json(e, DataType.fromJson(schema), options)

Yup, let me give a shot to provide an option for it first to show if it looks okay to you.

yap that's what I thought. sounds good!

HyukjinKwon · 2017-03-10T17:13:13Z

@felixcheung, I tried to add an optional parameter, asArray. Could you take a look and see if it makes sense?

SparkQA · 2017-03-10T19:14:34Z

Test build #74326 has finished for PR 17178 at commit 5c2450d.

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

felixcheung · 2017-03-12T19:24:44Z

a couple of thoughts

a default FALSE parameter might be less intuitive - do you think we could come up with a default TRUE parameter (ok, single?)
if it's asX, typically it would be more R-like to name it as.X, eg. as.array
array might be a bit vague - does that require the column value to be a JSON array, or just multiple JSON object spanning multiple line?

[
    {
        "title": "1",
        edition: 3,
        year: 2011
    },
    {
        "title": "2",
        edition: 2,
        year: 2009
    }
]

felixcheung · 2017-03-12T19:26:35Z

R/pkg/R/functions.R

+          function(x, schema, asArray = FALSE, ...) {
+            if (asArray) {
+              jschema <- callJStatic("org.apache.spark.sql.api.r.SQLUtils",
+                                     "createArrayType",


do we need a wrapper, actually? can't we call newJObject?

Ah, sure. Let me try.

It seems we'd better use

DataTypes.createArrayType

per

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/types/ArrayType.scala

Lines 49 to 56 in 04ee8cf

* The data type for collections of multiple values.

* Internally these are represented as columns that contain a ``scala.collection.Seq``.

*

* Please use `DataTypes.createArrayType()` to create a specific instance.

*

* An [[ArrayType]] object comprises two fields, `elementType: [[DataType]]` and

* `containsNull: Boolean`. The field of `elementType` is used to specify the type of

* array elements. The field of `containsNull` is used to specify if the array has `null` values.

Let me remove that new wrapper and use the original one.

HyukjinKwon · 2017-03-12T23:52:30Z

Thank you @felixcheung. Let me try to handle the comments soon.

HyukjinKwon · 2017-03-13T10:16:05Z

a default FALSE parameter might be less intuitive - do you think we could come up with a default TRUE parameter (ok, single?)
if it's asX, typically it would be more R-like to name it as.X, eg. as.array

Hmmm.. I am not too sure. Maybe, as.jsonobj.......? jsonobj?.. object?...as.single... struct... ? Not sure. Let me leave it for a day and revisit. If I can't think of a better one, then let me go for single.

array might be a bit vague - does that require the column value to be a JSON array, or just multiple JSON object spanning multiple line?

Yup, it requires that input is a JSON array (multiple line case should be already fine regardless of this option because that limitation came from initially reading the data from files before parsing JSONs. If these multiple line JSON strings are already read in a column, it should be fine for parsing.).

HyukjinKwon · 2017-03-13T10:53:29Z

How about as.struct?

felixcheung · 2017-03-14T06:29:50Z

seems like object is the right term in JSON lango - how about as.json.object? otherwise as.json.array (the opposite) or as.json.object.array might be good option

HyukjinKwon · 2017-03-14T08:58:07Z

Let me go for as.json.array actually..

HyukjinKwon · 2017-03-14T09:26:44Z

Doh! it seems it goes failed for lint (surprising) ...

R/functions.R:2456:31: style: Words within variable and function names should be separated by '_' rather than '.'.
          function(x, schema, as.json.array = TRUE, ...) {
                              ^~~~~~~~~~~~~
lintr checks failed.

Let me go for asJsonArray.

HyukjinKwon · 2017-03-14T10:28:37Z

R/pkg/R/functions.R

 #' @note from_json since 2.2.0
 setMethod("from_json", signature(x = "Column", schema = "structType"),
-          function(x, schema, ...) {
+          function(x, schema, asJsonArray = FALSE, ...) {


@felixcheung, I am still not fully sure of this name, asJsonArray. I am okay if you have a better one.

SparkQA · 2017-03-14T12:30:56Z

Test build #74507 has finished for PR 17178 at commit d01f952.

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

felixcheung · 2017-03-14T17:59:53Z

that's ridiculous ... dot in names is the norm https://stat.ethz.ch/R-manual/R-devel/library/utils/html/read.table.html

anyway

felixcheung

I think it's fine then.
one comment

felixcheung · 2017-03-14T18:00:35Z

R/pkg/R/functions.R

 #'
 #' @param x Column containing the JSON string.
 #' @param schema a structType object to use as the schema to use when parsing the JSON string.
+#' @param asJsonArray indicating if input string is JSON array or object.


maybe clarifying
JSON array or object. -> JSON array of objects or a single object.

HyukjinKwon · 2017-03-14T22:08:46Z

@felixcheung, thank you for your close look and asking my opinion.

SparkQA · 2017-03-15T00:12:00Z

Test build #74559 has finished for PR 17178 at commit 8ec7cd0.

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

felixcheung · 2017-03-15T02:51:44Z

thanks!
merged to master.

Support array in from_json in R

10feb9d

HyukjinKwon commented Mar 6, 2017

View reviewed changes

felixcheung reviewed Mar 7, 2017

View reviewed changes

HyukjinKwon added 2 commits March 11, 2017 02:07

Add asArray option for from_json

f13ad18

Remove unrelated changes

5c2450d

felixcheung requested changes Mar 12, 2017

View reviewed changes

Clean up and name it asJsonArray

d01f952

HyukjinKwon commented Mar 14, 2017

View reviewed changes

felixcheung reviewed Mar 14, 2017

View reviewed changes

Minor comments

8ec7cd0

asfgit closed this in d1f6c64 Mar 15, 2017

HyukjinKwon mentioned this pull request Apr 9, 2017

Add R code style guide apache/spark-website#43

Merged

HyukjinKwon deleted the SPARK-19828 branch January 2, 2018 03:43

	def from_json(e: Column, schema: String, options: java.util.Map[String, String]): Column =
	from_json(e, DataType.fromJson(schema), options)

	* The data type for collections of multiple values.
	* Internally these are represented as columns that contain a ``scala.collection.Seq``.
	*
	* Please use `DataTypes.createArrayType()` to create a specific instance.
	*
	* An [[ArrayType]] object comprises two fields, `elementType: [[DataType]]` and
	* `containsNull: Boolean`. The field of `elementType` is used to specify the type of
	* array elements. The field of `containsNull` is used to specify if the array has `null` values.

[SPARK-19828][R] Support array type in from_json in R #17178

[SPARK-19828][R] Support array type in from_json in R #17178

Uh oh!

Conversation

HyukjinKwon commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

HyukjinKwon Mar 6, 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!

HyukjinKwon Mar 6, 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!

HyukjinKwon commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Mar 7, 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!

Choose a reason for hiding this comment

Uh oh!

felixcheung Mar 7, 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!

HyukjinKwon Mar 8, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Mar 10, 2017

Uh oh!

SparkQA commented Mar 10, 2017

Uh oh!

felixcheung commented Mar 12, 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!

HyukjinKwon commented Mar 12, 2017

Uh oh!

HyukjinKwon commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Mar 13, 2017

Uh oh!

felixcheung commented Mar 14, 2017

Uh oh!

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

Uh oh!

Uh oh!

HyukjinKwon commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 14, 2017

Uh oh!

felixcheung commented Mar 14, 2017

Uh oh!

HyukjinKwon commented Mar 6, 2017 •

edited

Loading

HyukjinKwon Mar 6, 2017 •

edited

Loading

HyukjinKwon Mar 6, 2017 •

edited

Loading

HyukjinKwon commented Mar 6, 2017 •

edited

Loading

HyukjinKwon Mar 7, 2017 •

edited

Loading

felixcheung Mar 7, 2017 •

edited

Loading

HyukjinKwon Mar 8, 2017 •

edited

Loading

HyukjinKwon commented Mar 13, 2017 •

edited

Loading

HyukjinKwon commented Mar 14, 2017 •

edited

Loading