HyukjinKwon commented on code in PR #59167:
URL: https://github.com/apache/spark/pull/59167#discussion_r4162115826
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -635,9 +692,9 @@ class JacksonParser(
case err: PartialValueException if enablePartialResults =>
badRecordException = badRecordException.orElse(Some(err.cause))
values += err.partialResult
- case NonFatal(e) if enablePartialResults =>
+ case NonFatal(e) if drainErrors =>
badRecordException = badRecordException.orElse(Some(e))
- parser.skipChildren()
+ skipRemainingValue(parser, mapDepth)
Review Comment:
**Nit (P3):** This line also changes the ordinary `MAP<STRING, ...>` path
under the default `spark.sql.json.enablePartialResults=true`, which goes beyond
the CHAR/VARCHAR feature. `convertArray` rethrows a raw element failure while
the parser is still on that scalar inside the array. The old `skipChildren()`
did nothing in that case, so the remaining array tokens were read as keys of
the enclosing map. With `skipRemainingValue` they are drained correctly. That
looks like an improvement, but the PR description says STRING maps are
unchanged, and no test exercises this path: `nestedArrayLeakQuery` and
`nestedArrayMapQuery` both use `MAP<CHAR(2), ARRAY<INT>>`.
Could you add a STRING-keyed case, for example
`from_json('{"a":[1,"bad",99],"b":[3]}', 'MAP<STRING, ARRAY<INT>>')` with
partial results enabled, asserting that only `b` remains? Please also mention
the STRING-path change in the PR description.
**Verification:**
- **Regression:** A STRING-keyed JSON map with a nested array value that
fails partway through keeps only the later sibling key, and inner tokens do not
leak as outer keys, with partial results enabled.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -659,6 +716,60 @@ class JacksonParser(
}
}
+ /**
+ * CHAR/VARCHAR JSON maps apply assignment to object names, then
+ * `spark.sql.mapKeyDedupPolicy` to collisions created by that normalization.
+ * Exact repeated names stay historical last-wins.
+ */
+ private def convertConstrainedMap(
+ parser: JsonParser,
+ fieldConverter: ValueConverter,
+ keyType: DataType,
+ valueType: DataType,
+ mapDepth: Int): MapData = {
+ val lastEntries =
+ mutable.LinkedHashMap.empty[UTF8String, (UTF8String, Option[Any])]
+ var partialResultException: Option[Throwable] = None
+ var badMapException: Option[Throwable] = None
+
+ while (nextUntil(parser, JsonToken.END_OBJECT)) {
+ val rawKey = UTF8String.fromString(parser.currentName)
+ val value = try {
+ Some(fieldConverter.apply(parser))
+ } catch {
+ case err: PartialValueException if enablePartialResults =>
+ partialResultException =
partialResultException.orElse(Some(err.cause))
+ Some(err.partialResult)
+ case DuplicateMapKeyUtils(e) => throw e
+ case NonFatal(e) =>
+ badMapException = badMapException.orElse(Some(e))
+ skipRemainingValue(parser, mapDepth)
+ None
+ }
+ try {
+ val key = CharVarcharUtils.applyTextParseSemantics(rawKey, keyType)
+ lastEntries.update(rawKey, (key, value))
Review Comment:
**Non-blocking (P2):** This only applies when
`spark.sql.charVarchar.standardSemantics.enabled=true` and
`spark.sql.mapKeyDedupPolicy=LAST_WIN`.
`mutable.LinkedHashMap.update` on an existing key replaces the value but
keeps the entry in its original slot. `buildConstrainedMap` then walks
`lastEntries` in that order. Under `LAST_WIN`, the later normalized entry wins,
so the winner depends on each raw name's *first* occurrence, not its last.
Example: `from_json('{"a":1,"a ":2,"a":3}', 'MAP<CHAR(2), INT>')`
- `lastEntries` = `["a" -> ("a ", 3), "a " -> ("a ", 2)]`
- result: `{"a " -> 2}`, although the textually last entry is `"a":3`
`StaxXmlParser.convertConstrainedMap` avoids this by calling
`lastEntries.remove(rawKeyUtf8)` before `update`, so the same helper gives
different answers for JSON and XML. Doing the same here would fix it:
```scala
lastEntries.remove(rawKey)
lastEntries.update(rawKey, (key, value))
```
The `interleavedCharJsonQuery` expectation would then become `[("b ", 2),
("a ", 3)]`, which matches XML ordering. Please also add a case where an exact
name repeats after a colliding variant.
**Verification:**
- **Regression:** A JSON CHAR map whose input repeats an exact name after a
colliding variant yields the textually last value under LAST_WIN.
- **Compatibility:** Existing JSON CHAR/VARCHAR map expectations for
EXCEPTION, two-entry collisions, and exact-repeat last-wins still hold, with
entry order following the latest occurrence.
--
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]
