Skip to content
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -362,9 +362,9 @@ case class JsonTuple(children: Seq[Expression])
@transient private lazy val fieldExpressions: Seq[Expression] = children.tail

// eagerly evaluate any foldable the field names
@transient private lazy val foldableFieldNames: IndexedSeq[String] = {
@transient private lazy val foldableFieldNames: IndexedSeq[Option[String]] = {
fieldExpressions.map {
case expr if expr.foldable => expr.eval().asInstanceOf[UTF8String].toString
case expr if expr.foldable => Option(expr.eval()).map(_.asInstanceOf[UTF8String].toString)
case _ => null
}.toIndexedSeq
}
Expand Down Expand Up @@ -417,7 +417,7 @@ case class JsonTuple(children: Seq[Expression])
val fieldNames = if (constantFields == fieldExpressions.length) {
// typically the user will provide the field names as foldable expressions
// so we can use the cached copy
foldableFieldNames
foldableFieldNames.map(_.orNull)
} else if (constantFields == 0) {
// none are foldable so all field names need to be evaluated from the input row
fieldExpressions.map(_.eval(input).asInstanceOf[UTF8String].toString)
Expand All @@ -426,7 +426,7 @@ case class JsonTuple(children: Seq[Expression])
// prefer the cached copy when available
foldableFieldNames.zip(fieldExpressions).map {
case (null, expr) => expr.eval(input).asInstanceOf[UTF8String].toString
case (fieldName, _) => fieldName
case (fieldName, _) => fieldName.orNull
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2034,4 +2034,25 @@ class JsonSuite extends QueryTest with SharedSQLContext with TestJsonData {
}
}
}

test("SPARK-21677: json_tuple throws NullPointException when column is null as string type") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this to spark/sql/core/src/test/resources/sql-tests/inputs/json-functions.sql and/or spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/JsonExpressionsSuite.scala?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an end-to-end test case. We also need to add unit test cases in JsonExpressionsSuite

Copy link
Member

@viirya viirya Aug 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end-to-end test at L2047 may not be able to move to JsonExpressionsSuite. We can have some unit test cases similar to L2039 in JsonExpressionsSuite as @gatorsmile suggested.

It is also good to have this end-to-end tests in json-functions.sql.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gatorsmile has added unit test case in JsonExpressionsSuite
@viirya also add end-to-end test in json-functions.sql

checkAnswer(sql(
"""
|SELECT json_tuple('{"a" : 1, "b" : 2}'
|, cast(NULL AS STRING), 'b'
|, cast(NULL AS STRING), 'a')
""".stripMargin), Row(null, "2", null, "1"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmchung Can we also add the test we discussed in slack which mixes constant field name and non constant one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viirya Done, the added test case contains column name, constant field name, and null field name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: move Row(null, "2", null, "1")) to the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, thanks


// mixes constant field name and non constant one
withTempView("jsonTable") {
Seq(("""{"a": 1, "b": 2}""", "a", "b"))
.toDF("jsonField", "a", "b")
.createOrReplaceTempView("jsonTable")

checkAnswer(
sql("""SELECT json_tuple(jsonField, b, cast(NULL AS STRING), 'a') FROM jsonTable"""),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: """ -> "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will move L2053 to json-functions.sql

Row("2", null, "1")
)
}
}
}