srielau commented on code in PR #59453:
URL: https://github.com/apache/spark/pull/59453#discussion_r4271530079
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -780,6 +789,26 @@ class JacksonParser(
}
}
+ /**
+ * Drain this object or array through `stopOn` and rethrow `cause`.
skipChildren() must run
+ * first: nextUntil(stopOn) can otherwise stop at the failed value's own end
token and leave
+ * parent siblings unconsumed, so an enclosing convertObject would treat
remaining names as
+ * its own. Skip failures are suppressed on `cause` so the original error is
preserved.
+ */
+ private def skipRestAndRethrow(
+ parser: JsonParser, stopOn: JsonToken, cause: Throwable): Nothing = {
+ try {
+ parser.skipChildren()
+ while (nextUntil(parser, stopOn)) {
Review Comment:
Done in 4c4878d. `skipRestAndRethrow` now captures the container's nesting
depth at START and unwinds while the parser is deeper than that, then consumes
remaining siblings only while still at that depth. An inner END token pops to
this container, so we no longer stop at the first matching END. Skip failures
stay suppressed on the original cause. Added `JsonFunctionsSuite` coverage for
VARIANT duplicate keys as a struct field, a map value, an array nested in the
variant value, and an `ARRAY<VARIANT>` element, with partial results off;
PERMISSIVE keeps the outer sibling and FAILFAST still reports
`VARIANT_DUPLICATE_KEY`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/OptimizeCsvJsonExprs.scala:
##########
@@ -472,6 +478,15 @@ object OptimizeCsvJsonExprs extends Rule[LogicalPlan] {
g.copy(child = j.copy(schema = prunedSchema), ordinal = 0, numFields = 1)
}
+ /**
+ * True when `dt` has a MAP whose key type is CHAR or VARCHAR at any nesting.
+ * Only keys: JacksonParser length-checks CHAR/VARCHAR map keys, not map
values.
Review Comment:
Done. The guard scaladoc now states that it preserves the SPARK-60108 key
check (flag-gated, no pad/trim) and that CHAR/VARCHAR values go through
`applyTextParseSemantics` regardless of that flag.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3543,6 +3543,81 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-60229: from_json(to_json) keeps CHAR/VARCHAR map key check") {
+ Seq(
+ ("CHAR(3)", "a", "t_spark_60229_char"),
+ ("VARCHAR(3)", "abcd", "t_spark_60229_varchar")
+ ).foreach { case (keyType, badKey, viewName) =>
+ withView(viewName) {
+ withSQLConf(
+ SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "false",
+ SQLConf.PRESERVE_CHAR_VARCHAR_TYPE_INFO.key -> "true") {
+ sql(s"""CREATE TEMPORARY VIEW $viewName AS
+ |SELECT from_json('{"$badKey":1}', 'MAP<$keyType, INT>') AS
m""".stripMargin)
+ }
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ checkAnswer(
+ sql(s"SELECT from_json(to_json(m), 'MAP<$keyType, INT>') FROM
$viewName"),
+ Row(null))
+ assertUnsupportedJsonMapKey(
+ s"""SELECT from_json(to_json(m), 'MAP<$keyType, INT>', map('mode',
'FAILFAST'))
+ |FROM $viewName""".stripMargin,
+ key = badKey,
+ dataTypeSql = keyType)
+ }
+ }
+ }
+ }
+
+ test("SPARK-60229: nested struct map key rejection does not steal sibling
fields") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ val schema = "s STRUCT<m MAP<CHAR(3), INT>, tail INT>, tail INT"
+ // Inner tail after the map: without draining s, the parent can take
inner tail=5
+ // and then hit s's END_OBJECT, never seeing outer tail=1.
+ val jsonAfter = """{"s":{"m":{"a":1},"tail":5},"tail":1}"""
+ // Inner tail after an already-parsed outer tail: without draining s,
inner
+ // tail=5 overwrites the sibling that was already 1.
+ val jsonBefore = """{"tail":1,"s":{"m":{"a":1},"tail":5}}"""
+ Seq(true, false).foreach { partial =>
+ withSQLConf(SQLConf.JSON_ENABLE_PARTIAL_RESULTS.key ->
partial.toString) {
+ val expected = if (partial) {
+ Row(Row(Row(null, 5), 1))
+ } else {
+ Row(Row(null, 1))
+ }
+ Seq(jsonAfter, jsonBefore).foreach { json =>
+ checkAnswer(sql(s"SELECT from_json('$json', '$schema')"), expected)
+ assertUnsupportedJsonMapKey(
+ s"""SELECT from_json('$json', '$schema', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ }
+ }
+ }
+
+ // Array element failure must drain to END_ARRAY so an inner field name
cannot
Review Comment:
Done. The comment now only asserts that a failing array element does not
leak inner names and that later siblings are still parsed; it no longer
attributes that to the `convertArray` drain arm.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -765,6 +771,9 @@ class JacksonParser(
case err: PartialValueException if enablePartialResults =>
badRecordException = badRecordException.orElse(Some(err.cause))
values += err.partialResult
+ case NonFatal(e) if !enablePartialResults =>
Review Comment:
Done. The `convertArray` drain arm is now `!isRoot &&
!enablePartialResults`, matching the nested-only comment and the
`convertObject` root handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
