dtenedor commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4235496472
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
*/
private def convertMap(
parser: JsonParser,
- fieldConverter: ValueConverter): MapData = {
+ fieldConverter: ValueConverter,
+ keyType: DataType): MapData = {
val keys = ArrayBuffer.empty[UTF8String]
val values = ArrayBuffer.empty[Any]
var badRecordException: Option[Throwable] = None
+ // CHAR/VARCHAR keys last-win on exact (binary) duplicate names. This maps
a key to
+ // the index of its value in `values` for O(1) overwrite; it stays null
(and the
+ // buffers keep every pair, as STRING maps always have) when last-win does
not apply.
+ // It indexes `values` (not `keys`) so it stays valid even after the
NonFatal arm
+ // appends a dangling key, leaving the two buffers unbalanced.
+ val lastWinIndex: mutable.HashMap[UTF8String, Int] =
+ if (isLengthCheckedKeyType(keyType)) mutable.HashMap.empty[UTF8String,
Int] else null
while (nextUntil(parser, JsonToken.END_OBJECT)) {
- keys += UTF8String.fromString(parser.currentName)
+ val key = convertJsonMapKey(parser.currentName, keyType)
Review Comment:
This check runs before the `try`, so a rejected key escapes `convertMap`
while the parser is still on the map's `FIELD_NAME`. When the map is a struct
field, `convertObject` catches it (`NonFatal(e) if isRoot ||
enablePartialResults`) and calls `skipChildren()`, which is a no-op on a
`FIELD_NAME`. It then reads the rest of the map's entries as if they were
struct fields, and stops at the map's `END_OBJECT` as if it were its own.
With the default `spark.sql.json.enablePartialResults=true`, schema `m
MAP<CHAR(3), INT>, tail INT`, PERMISSIVE (I replayed the token flow with a
small standalone Jackson harness that mirrors `convertObject`/`convertMap`):
- `{"m":{"a":1},"tail":1}` gives `{m: null, tail: null}`. The value-side
analog in `SPARK-59274: JSON map value overflow keeps EXCEED_LIMIT_LENGTH`
keeps `tail = 1`.
- `{"m":{"a":1,"tail":5},"tail":1}` gives `{m: null, tail: 5}`, a wrong
non-null value.
- multiLine with `enableStreamingTopLevelArray=true`:
`[{"m":{"a":1},"tail":1},{"m":{"abc":2},"tail":2}]` yields 4 rows instead of 2.
`parseIterator` treats a `PartialResultException` from a `START_OBJECT` element
as resumable, assuming `convertObject` consumed through that element's
`END_OBJECT`.
The negative tests only use root-level maps, where the parser is discarded,
so they don't hit this. Treating a bad key like a failed value fixes all three
cases in the harness: record the error, step past the value, and append the key
unpaired so the existing `keys.length != values.length` check rethrows it after
`END_OBJECT`. Roughly:
```scala
case Some(err) => // key failed the length check
badRecordException = badRecordException.orElse(Some(err))
parser.nextToken() // FIELD_NAME -> value
parser.skipChildren()
keys += key
```
I'd apply this regardless of `enablePartialResults`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -107,6 +107,9 @@ class JacksonParser(
private val enablePartialResults = SQLConf.get.jsonEnablePartialResults
+ // CHAR/VARCHAR JSON map keys are length-checked without pad or trim under
this flag.
+ private val charVarcharStandardSemantics =
SQLConf.get.charVarcharStandardSemantics
Review Comment:
`spark.sql.charVarchar.standardSemantics.enabled` has
`ConfigBindingPolicy.PERSISTED` binding so that views keep their creation-time
semantics, but this reads it through `SQLConf.get` when the parser is built,
which for `from_json` happens lazily at execution. Unless I'm missing a
mechanism that applies persisted confs at execution time, a view created with
the flag on and queried from a session with it off still declares `MAP<CHAR(3),
_>` but skips the key check. Map values stay correct because
`applyTextParseSemantics` follows the analyzed schema. In the other direction,
a view created with the flag off starts rejecting rows when queried with it on.
Could this be captured at analysis time, the way `JsonToStructs` captures
`variantAllowDuplicateKeys`, and passed into the parser? A test that creates a
view under the flag and queries it with the flag off would cover it.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -659,6 +696,33 @@ class JacksonParser(
}
}
+ /**
+ * A CHAR/VARCHAR map key under standard semantics is length-checked (not
padded or
+ * trimmed) and deduplicated last-win on exact names. This is the single
predicate both
+ * `convertMap` and `convertJsonMapKey` consult so the two cannot drift
apart.
+ */
+ private def isLengthCheckedKeyType(keyType: DataType): Boolean =
+ charVarcharStandardSemantics && (keyType match {
+ case _: CharType | _: VarcharType => true
+ case _ => false
+ })
+
+ private def convertJsonMapKey(rawName: String, keyType: DataType):
UTF8String = {
+ val key = UTF8String.fromString(rawName)
+ if (!isLengthCheckedKeyType(keyType)) {
+ key
+ } else {
+ keyType match {
+ case c: CharType =>
+ CharVarcharCodegenUtils.charTypeJsonMapKeyCheck(key, c.length)
+ case v: VarcharType =>
+ CharVarcharCodegenUtils.varcharTypeJsonMapKeyCheck(key, v.length)
+ case _ =>
+ key
Review Comment:
Nit: this branch is unreachable, since `isLengthCheckedKeyType` already
matched `CharType`/`VarcharType`. The key handling could also be chosen once in
`makeConverter` (for example, a key converter built from `mt.keyType`) instead
of matching `keyType` again for every key of every map.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
*/
private def convertMap(
parser: JsonParser,
- fieldConverter: ValueConverter): MapData = {
+ fieldConverter: ValueConverter,
+ keyType: DataType): MapData = {
val keys = ArrayBuffer.empty[UTF8String]
val values = ArrayBuffer.empty[Any]
var badRecordException: Option[Throwable] = None
+ // CHAR/VARCHAR keys last-win on exact (binary) duplicate names. This maps
a key to
+ // the index of its value in `values` for O(1) overwrite; it stays null
(and the
+ // buffers keep every pair, as STRING maps always have) when last-win does
not apply.
+ // It indexes `values` (not `keys`) so it stays valid even after the
NonFatal arm
+ // appends a dangling key, leaving the two buffers unbalanced.
+ val lastWinIndex: mutable.HashMap[UTF8String, Int] =
+ if (isLengthCheckedKeyType(keyType)) mutable.HashMap.empty[UTF8String,
Int] else null
Review Comment:
Hardening suggestion: these keys come straight from the input,
`UTF8String.hashCode` is Murmur3 with a fixed seed, and Scala 2.13's
`mutable.HashMap` chains colliding entries without tree bins. An object with
many crafted colliding names (feasible mainly with longer VARCHAR limits) makes
this quadratic per map. `java.util.HashMap` converts long collision chains into
trees for `Comparable` keys (`UTF8String` is one), and it is what
`ArrayBasedMapBuilder` uses for binary-equality strings.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/util/CharVarcharCodegenUtils.java:
##########
@@ -111,4 +113,28 @@ public static UTF8String charTypeReadSideCheck(UTF8String
inputStr, int limit) {
public static UTF8String varcharTypeReadSideCheck(UTF8String inputStr, int
limit) {
return varcharTypeWriteSideCheck(inputStr, limit);
}
+
+ /**
+ * JSON object names used as MAP<CHAR(n), _> keys cannot be padded or
trimmed.
+ * The name must already be exactly n characters.
+ */
+ public static UTF8String charTypeJsonMapKeyCheck(UTF8String inputStr, int
limit) {
+ if (inputStr.numChars() != limit) {
+ throw QueryExecutionErrors.unsupportedJsonCharVarcharMapKey(
+ inputStr, CharType.apply(limit));
Review Comment:
Rebuilding the type from the length drops any collation, so a `VARCHAR(2)
COLLATE UTF8_LCASE` key is reported as `"VARCHAR(2)"` (same for
`VarcharType.apply(limit)` below). Passing the actual key type through would
keep the message accurate. Also, since these two helpers aren't codegen entry
points, they might sit better as private helpers in `JacksonParser` or in
`CharVarcharUtils`.
##########
docs/sql-migration-guide.md:
##########
@@ -28,6 +28,7 @@ license: |
## Upgrading from Spark SQL 4.3 to 4.4
+- Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is
true, JSON object names used as `MAP<CHAR(n), _>` or `MAP<VARCHAR(n), _>` keys
in `from_json` and the JSON datasource are length-checked without padding or
trimming. A `CHAR(n)` key must already be exactly `n` characters, and a
`VARCHAR(n)` key must already be at most `n` characters. Mismatched keys fail
with `UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY` (SQLSTATE `0A000`) and follow the
JSON parse mode (`PERMISSIVE` or `FAILFAST`). Exact repeated member names
last-win; `spark.sql.mapKeyDedupPolicy` is not applied. With the flag off, CHAR
keys are still padded and VARCHAR overflow still uses `EXCEED_LIMIT_LENGTH`.
Review Comment:
`spark.sql.charVarchar.standardSemantics.enabled` is new in 4.4.0 and off by
default, so nobody upgrading from 4.3 sees a change; this may fit the
CHAR/VARCHAR docs better than the migration guide.
The last sentence also doesn't match what I see. With the default settings,
a CHAR type in a `from_json` schema fails analysis (`failIfHasCharVarchar`).
With `preserveCharVarcharTypeInfo` on, the parser keeps the raw key, and
padding only happens when results are converted to external rows (see the
comment on the flag-off test).
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3072,6 +3094,113 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-60108: JSON CHAR/VARCHAR map keys are not padded or trimmed") {
Review Comment:
The root-level cases are well covered. Cases I'd add:
- A bad key in a map followed by a sibling field, in both partial-results
modes (mirroring the SPARK-59274 `tail` test). This would catch the
parser-position issue in `convertMap`.
- Bad keys in nested containers: `MAP<STRING, MAP<CHAR(3), INT>>`,
`ARRAY<MAP<CHAR(3), INT>>`, and a map inside a nested struct. Only the valid
nested case is covered.
- multiLine with `enableStreamingTopLevelArray=true`, checking the row count
and the corrupt-record column.
- A view created with the flag on and queried with it off.
- Characters outside the BMP (for example an emoji in a `CHAR(1)` key), to
pin that length counts characters.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -9605,6 +9605,13 @@
],
"sqlState" : "0A000"
},
+ "UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY" : {
+ "message" : [
+ "JSON map keys of CHAR or VARCHAR cannot be padded or trimmed.",
+ "The key <key> is not valid for type <dataType>."
Review Comment:
For a VARCHAR key that is too long, "cannot be padded or trimmed" doesn't
tell the user what's wrong or how to fix it. Maybe state the rule and a remedy,
for example: "The JSON object name <key> is not valid for map key type
<dataType>: CHAR(n) keys must have exactly n characters and VARCHAR(n) keys at
most n characters. Use STRING map keys to accept any name."
Separately, `<key>` embeds the raw object name, which in map-shaped JSON is
often data (IDs, emails) and can be up to Jackson's default 50,000-character
name limit, while `EXCEED_LIMIT_LENGTH` deliberately omits the value. Consider
truncating it.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3072,6 +3094,113 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-60108: JSON CHAR/VARCHAR map keys are not padded or trimmed") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ checkAnswer(
+ sql("""SELECT from_json('{"ab ": 1}', 'MAP<CHAR(3), INT>')"""),
+ Row(Map("ab " -> 1)))
+ checkAnswer(
+ sql("""SELECT from_json('{"ab": 1}', 'MAP<VARCHAR(3), INT>')"""),
+ Row(Map("ab" -> 1)))
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"ab": 1, "ab ": 2}',
'MAP<VARCHAR(3), INT>'))"""),
+ Row(Seq(Row("ab", 1), Row("ab ", 2))))
+ checkAnswer(
+ sql("""SELECT size(from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>'))"""),
+ Row(1))
+ checkAnswer(
+ sql("""SELECT from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>')"""),
+ Row(Map("abc" -> 2)))
+ checkAnswer(
+ sql("""SELECT from_json('{"outer": {"xy ": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>')""".stripMargin),
+ Row(Map("outer" -> Map("xy " -> 1))))
+
+ // Too-short CHAR, CHAR overflow, and VARCHAR overflow are not
EXCEED_LIMIT_LENGTH.
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"ab ": 1}', 'MAP<VARCHAR(2),
INT>')"""), Row(null))
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "VARCHAR(3)")
+
+ withTempPath { path =>
+ Seq("""{"m":{"ab ":1}}""", """{"m":{"a":1}}""").toDS()
+ .repartition(1)
+ .write.text(path.getCanonicalPath)
+ val schema = "m MAP<CHAR(3), INT>"
+ val permissive = spark.read.schema(schema).json(path.getCanonicalPath)
+ checkAnswer(permissive, Seq(Row(Map("ab " -> 1)), Row(null)))
+ val failFast = spark.read.option("mode", "FAILFAST").schema(schema)
+ .json(path.getCanonicalPath)
+ assertUnsupportedJsonMapKeyError(failFast.collect(), key = "a",
dataTypeSql = "CHAR(3)")
+ }
+
+ withSQLConf(
+ SQLConf.MAP_KEY_DEDUP_POLICY.key ->
SQLConf.MapKeyDedupPolicy.EXCEPTION.toString) {
+ checkAnswer(
+ sql("""SELECT from_json('{"abc": 1, "abc": 2}', 'MAP<VARCHAR(3),
INT>')"""),
+ Row(Map("abc" -> 2)))
Review Comment:
`checkAnswer` compares against a Scala `Map`, and converting the result
during `collect()` collapses duplicate keys, so this passes whether or not the
parser deduplicated. Checking `size(...)` or `map_entries(...)`, as the CHAR
case above does, would pin the behavior.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3072,6 +3094,113 @@ class BasicCharVarcharTestSuite extends
SharedSparkSession {
}
}
+ test("SPARK-60108: JSON CHAR/VARCHAR map keys are not padded or trimmed") {
+ withSQLConf(SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "true") {
+ checkAnswer(
+ sql("""SELECT from_json('{"ab ": 1}', 'MAP<CHAR(3), INT>')"""),
+ Row(Map("ab " -> 1)))
+ checkAnswer(
+ sql("""SELECT from_json('{"ab": 1}', 'MAP<VARCHAR(3), INT>')"""),
+ Row(Map("ab" -> 1)))
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"ab": 1, "ab ": 2}',
'MAP<VARCHAR(3), INT>'))"""),
+ Row(Seq(Row("ab", 1), Row("ab ", 2))))
+ checkAnswer(
+ sql("""SELECT size(from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>'))"""),
+ Row(1))
+ checkAnswer(
+ sql("""SELECT from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>')"""),
+ Row(Map("abc" -> 2)))
+ checkAnswer(
+ sql("""SELECT from_json('{"outer": {"xy ": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>')""".stripMargin),
+ Row(Map("outer" -> Map("xy " -> 1))))
+
+ // Too-short CHAR, CHAR overflow, and VARCHAR overflow are not
EXCEED_LIMIT_LENGTH.
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"ab ": 1}', 'MAP<VARCHAR(2),
INT>')"""), Row(null))
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<CHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"abcd": 1}', 'MAP<VARCHAR(3), INT>', map('mode',
'FAILFAST'))""",
+ key = "abcd",
+ dataTypeSql = "VARCHAR(3)")
+
+ withTempPath { path =>
+ Seq("""{"m":{"ab ":1}}""", """{"m":{"a":1}}""").toDS()
+ .repartition(1)
+ .write.text(path.getCanonicalPath)
+ val schema = "m MAP<CHAR(3), INT>"
+ val permissive = spark.read.schema(schema).json(path.getCanonicalPath)
+ checkAnswer(permissive, Seq(Row(Map("ab " -> 1)), Row(null)))
+ val failFast = spark.read.option("mode", "FAILFAST").schema(schema)
+ .json(path.getCanonicalPath)
+ assertUnsupportedJsonMapKeyError(failFast.collect(), key = "a",
dataTypeSql = "CHAR(3)")
+ }
+
+ withSQLConf(
+ SQLConf.MAP_KEY_DEDUP_POLICY.key ->
SQLConf.MapKeyDedupPolicy.EXCEPTION.toString) {
+ checkAnswer(
+ sql("""SELECT from_json('{"abc": 1, "abc": 2}', 'MAP<VARCHAR(3),
INT>')"""),
+ Row(Map("abc" -> 2)))
+ }
+
+ // A last-win duplicate whose winning (second) value overflows nulls the
whole
+ // record and keeps EXCEED_LIMIT_LENGTH, matching the non-duplicate
case, instead
+ // of silently keeping the superseded first value. Holds for both
partial modes.
+ Seq(true, false).foreach { partial =>
+ withSQLConf(SQLConf.JSON_ENABLE_PARTIAL_RESULTS.key ->
partial.toString) {
+ checkAnswer(
+ sql("""SELECT from_json('{"abc": "x", "abc": "abcdef"}',
+ | 'MAP<CHAR(3), CHAR(2)>')""".stripMargin),
+ Row(null))
+ assertParseExceedLimit(
+ """SELECT from_json('{"abc": "x", "abc": "abcdef"}',
+ | 'MAP<CHAR(3), CHAR(2)>',
+ | map('mode', 'FAILFAST'))""".stripMargin,
+ expectedLimit = "2")
+ }
+ }
+
+ // Collation is not consulted for last-win: binary-distinct names are
both kept,
+ // unlike XML which pads keys and then applies the dedup policy.
+ withSQLConf(SQLConf.ALLOW_COLLATIONS_IN_MAP_KEYS.key -> "true") {
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"ab": 1, "AB": 2}',
+ | 'MAP<VARCHAR(2) COLLATE UTF8_LCASE, INT>'))""".stripMargin),
+ Row(Seq(Row("ab", 1), Row("AB", 2))))
+ }
+ }
+
+ // Flag off is unchanged: write-side pad/trim still apply to keys, and the
new
+ // UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY check is not used.
+ withSQLConf(
+ SQLConf.CHAR_VARCHAR_STANDARD_SEMANTICS.key -> "false",
+ SQLConf.PRESERVE_CHAR_VARCHAR_TYPE_INFO.key -> "true") {
+ checkAnswer(
+ sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>')"""),
+ Row(Map("a " -> 1)))
Review Comment:
As far as I can tell, the parser doesn't touch keys with the flag off, so
`Map("a " -> 1)` here and the `EXCEED_LIMIT_LENGTH` below come from the
CHAR/VARCHAR deserializers that run during `collect()` (`RowEncoder` uses
`CharEncoder`/`VarcharEncoder` when CHAR/VARCHAR are first-class), not from
`from_json`. Inside the engine the key is still `"a"`, and `mode=FAILFAST`
doesn't change the outcome. Asserting on an engine-side value, such as
`to_json(from_json(...))` or `length(map_keys(...)[0])`, would test what the
comment describes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala:
##########
@@ -2739,6 +2739,19 @@ private[sql] object QueryExecutionErrors extends
QueryErrorsBase with ExecutionE
)
}
+ // A SparkRuntimeException (not SparkSQLFeatureNotSupportedException)
despite the 0A000
+ // SQLSTATE: restricting CHAR/VARCHAR JSON map keys is a limitation, but the
error is
+ // raised while parsing a row and must flow through Jackson's throw/catch
parse-mode
+ // model (PERMISSIVE wraps it as a bad record, FAILFAST surfaces it).
Review Comment:
`0A000` is an unusual fit for a failure that depends on row data, and in
FAILFAST mode clients won't see it at the top level: `FailureSafeParser` wraps
the cause in `MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION` (`22023`). The
standard `22026` ("string data, length mismatch", already in
`error-states.json`) describes a CHAR(n) key that isn't exactly n characters,
and would make this an ordinary data error instead of a feature limitation.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/json/JacksonParser.scala:
##########
@@ -619,25 +622,59 @@ class JacksonParser(
/**
* Parse an object as a Map, preserving all fields.
+ *
+ * JSON object names used as CHAR/VARCHAR keys are length-checked without
rewriting:
+ * CHAR keys must already be exactly n characters, and VARCHAR keys must
already be
+ * at most n characters. Padding, trimming, and mapKeyDedupPolicy are not
applied.
+ * Repeated names that are binary-equal last-win for CHAR/VARCHAR keys
(collation is
+ * not consulted, matching how STRING keys are compared here); STRING keys
keep every
+ * pair. This differs from XML, which pads keys and then applies
mapKeyDedupPolicy.
Review Comment:
This introduces a third duplicate-key behavior:
- STRING keys keep every pair.
- XML CHAR/VARCHAR keys honor `spark.sql.mapKeyDedupPolicy` (default
`EXCEPTION`) through `ArrayBasedMapBuilder`, which compares using the key's
collation.
- JSON CHAR/VARCHAR keys (this PR) last-win on binary equality and ignore
the policy.
For example, `SPARK-59274: collated CHAR/VARCHAR map keys honor the dedup
policy` expects `from_xml` with `<a>1</a><A>2</A>` under `CHAR(2) COLLATE
UTF8_LCASE` to raise `DUPLICATED_MAP_KEY`, while the new collation test expects
the JSON equivalent to return both entries, which is a map whose keys aren't
unique under its own key type.
The length check doesn't need dedup. Keeping STRING's keep-all behavior
would be the smallest change, and reusing `ArrayBasedMapBuilder` would match
XML (and also addresses the hashing comment below). Related:
- XML pads short CHAR keys under the same flag and handles the collisions
that padding can create through the dedup policy, which is the reason given
here for not padding JSON keys. It would be good to settle on one rule across
formats, or document the difference.
- When the superseded value is the one that fails
(`{"abc":"toolong","abc":"ok"}` into `MAP<CHAR(3), VARCHAR(2)>`), the record
still becomes null. That's defensible, but worth a test and a doc note.
Nit: the scaladoc still opens with "preserving all fields", and STRING keys
aren't compared at all here, so "matching how STRING keys are compared here"
reads oddly.
--
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]
