cloud-fan commented on code in PR #59453:
URL: https://github.com/apache/spark/pull/59453#discussion_r4269042385
##########
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:
**Nit (P3):** The premise here is not accurate. JacksonParser does check
CHAR/VARCHAR map values: its `StringType` converter ends with
`CharVarcharUtils.applyTextParseSemantics` (JacksonParser.scala:379). That pads
CHAR values and raises `EXCEED_LIMIT_LENGTH` for map values too, independent of
`charVarcharStandardSemantics`; the existing `SPARK-59274: JSON map value
overflow keeps EXCEED_LIMIT_LENGTH` test asserts it. Keys differ because the
SPARK-60108 key check (`makeMapKeyChecker`) is gated on the captured flag and
never pads or trims. That is the check this guard exists to preserve. Suggest
stating the actual boundary:
```suggestion
* Only keys: this preserves the SPARK-60108 map-key check, which runs
only under the captured
* standard semantics and never pads or trims. CHAR/VARCHAR values get
pad/length semantics from
* applyTextParseSemantics regardless of that flag and are outside this
guard.
```
**Verification:**
- **Inspection:** The guard documentation matches JacksonParser's key check
(flag-gated, no pad or trim) and its unconditional value semantics.
##########
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:
**Nit (P3):** For a top-level JSON array, `makeStructRootConverter` calls
`convertArray(..., isRoot = true, arrayAsStructs = true)`. With partial results
off, a failing element now drains the rest of the root array before rethrowing
the same exception; for a multiLine file that is the rest of the file.
`parse()` (via `tryWithResource`) and the non-streaming `parseIterator` branch
(via `fail()`) then close the parser without reading it. The drain has no
consumer, and the bad-record outcome is unchanged. The mirrored `convertObject`
arm never runs at the root, because the earlier `isRoot ||
enablePartialResults` arm handles it, and the comment below says this arm is
for nested arrays. Suggest excluding the root:
```suggestion
case NonFatal(e) if !isRoot && !enablePartialResults =>
```
**Verification:**
- **Compatibility:** A multiLine top-level array whose early element fails,
with partial results off, still produces the same bad-record outcome in
PERMISSIVE and FAILFAST.
- **Inspection:** The convertArray drain arm excludes the root case,
consistent with its comment and with the convertObject arm.
##########
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:
**Nit (P3):** With partial results off, this case does not depend on the
`convertArray` arm. The element struct's own `convertObject` arm drains through
the element's `END_OBJECT` before `convertArray` sees the error. Without the
`convertArray` arm, the root's `isRoot` arm resumes on that `END_OBJECT`. The
next token is `END_ARRAY`, whose `currentName` is `arr`, so the `arr` converter
fails conversion and the error is recorded first-error-wins. The root then
parses `after=2`. The result is still `Row(1, null, 2)` with the same FAILFAST
map-key error, so nothing fails if the `convertArray` arm is removed or broken.
Suggest rewording the comment to describe what is asserted: a failing array
element does not leak inner field names to the parent, and later siblings are
still parsed. Drop the `convertArray` attribution.
**Verification:**
- **Inspection:** The comment no longer attributes the array case's
protection to the convertArray drain arm.
##########
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:
**Nit (P3):** This loop stops at the first `stopOn` token after wherever the
child left the parser, so it is correct only when the child failed at its own
depth. `parseVariant` breaks that assumption. `VariantBuilder.buildJson`
recurses into nested objects and throws `VARIANT_DUPLICATE_KEY` from
`finishWritingObject` at an inner `END_OBJECT` (the default is
`spark.sql.variant.allowDuplicateKeys=false`).
Example with `spark.sql.json.enablePartialResults=false`:
```sql
SELECT from_json('{"s":{"v":{"a":{"k":1,"k":2}},"b":7},"b":5}',
's STRUCT<v: VARIANT, b: INT>, b INT')
```
1. The failure leaves the parser on `a`'s `END_OBJECT`.
2. `skipChildren()` is a no-op on an END token, and `nextUntil(END_OBJECT)`
stops at `v`'s `END_OBJECT`, still inside `s`.
3. The root then reads `s.b` as its own `b` and ends at `s`'s `END_OBJECT`,
so the outer `b=5` is never read.
PERMISSIVE returns `{s: null, b: 7}`; before this PR it returned `{s: null,
b: null}`. `MAP<STRING, VARIANT>` hits the same problem through the
`convertMap` arm. The `convertArray` arm is defeated only when the failure sits
inside an array nested in the variant element.
This affects only the internal partial-off mode. The default partial-results
recovery gives the same value for this input, but this PR does not change it.
**Recommended change:** Make the partial-off drain unwind to the failing
container's own boundary. Record the container's nesting depth from the
parser's parsing context when the container starts. On a non-partial child
failure, advance (skipping children) until the parser is back at that depth,
then consume the remaining tokens through the container's END token before
rethrowing. Keep suppressing skip-time failures on the original cause. Add JSON
parsing regression coverage with enablePartialResults=false, in the JSON
functions or CHAR/VARCHAR test families, for VARIANT children whose failure
sits below the variant's top level. Cover a struct field and a map value
failing inside a nested variant object, and an array element failing inside an
array nested in the variant element. The current drain gives a wrong sibling
value for each of these. Also cover an ARRAY<VARIANT> element failing inside a
nested variant object. The current END_ARRAY stop already handles it, but its
result d
epends on convertArray's own drain.
**Why this works:** The stop condition becomes the container's captured
nesting depth instead of the first matching END token. Any contexts a child
leaves open are strictly deeper than the captured depth, so unwinding to it and
then to the container's END token restores alignment wherever the child
stopped. Depth stays meaningful even though Jackson reuses context objects.
**Scope:** Rework the drain helper's stop condition and the way the three
non-partial arms pass it the container boundary, plus deep-failure regression
tests.
**Compatibility:** Partial-results-on recovery and all success-path parsing
remain unchanged.
**Risks:** Jackson recycles child context objects, so comparing context
identity can misidentify a sibling container. Compare nesting depth rather than
context identity. Unwinding after a JsonParseException may throw again and must
stay suppressed on the original cause.
**Constraints:** Leave the enablePartialResults=true recovery paths
unchanged (declared scope). Rethrow the same original cause so FAILFAST still
reports it.
**Success:** With enablePartialResults=false, a nested struct, map, or array
whose child fails at any depth (including a VARIANT duplicate key inside a
nested variant object) leaves the parser on that container's own END token
before rethrowing. from_json('{"s":{"v":{"a":{"k":1,"k":2}},"b":7},"b":5}', 's
STRUCT<v: VARIANT, b: INT>, b INT') returns {s: null, b: 5} in PERMISSIVE with
partial results off, and FAILFAST reports VARIANT_DUPLICATE_KEY. Existing
direct-child failure results in the SPARK-40646, SPARK-44940, SPARK-59274, and
SPARK-60229 tests are unchanged.
**Verification:**
- **Regression:** With partial results off, the outer sibling keeps its own
value and FAILFAST reports the original error when a VARIANT child fails inside
a nested variant object as a struct field or a map value, or inside an array
nested in the variant element as an array element. The current drain gives a
wrong sibling value for each of these. An ARRAY<VARIANT> element failing inside
a nested variant object also keeps the outer sibling, which pins convertArray's
own drain.
- **Compatibility:** Partial-results on and off results for direct-child
nested failures in from_json and the JSON data source are unchanged.
--
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]
