HyukjinKwon commented on code in PR #59325:
URL: https://github.com/apache/spark/pull/59325#discussion_r4240217530
##########
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. A mismatched key turns
the row into a bad record: in `PERMISSIVE` mode it is dropped (the output is
`null`), while in `FAILFAST` mode parsing fails with
`UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY` (SQLSTATE `0A000`) as the cause of
`MALFORMED_RECORD_IN_PARSING`. Duplicate names are kept, exactly as for STRING
keys; `spark.sql.mapKeyDedupPolicy` is not applied.
Review Comment:
**Non-blocking (P2):** This entry describes the JSON datasource as behaving
like `from_json`, but datasource scans still go through the existing read-side
CHAR/VARCHAR handling, which differs in two ways:
- **Catalog tables** (e.g. `CREATE TABLE t (m MAP<CHAR(3), INT>) USING
json`): `SessionCatalog.getTableMetadata` / `LogicalRelation.apply` replace
CHAR/VARCHAR with STRING before the reader sees the schema, so
`makeMapKeyChecker` returns `None`. `ApplyCharTypePadding` then applies
`addPaddingForScan`, which under standard semantics routes map keys through
`charTypeReadSideCheck` / `varcharTypeReadSideCheck`
(`CharVarcharUtils.scala:269-281`). So `{"m":{"a":1}}` reads back with key `'a
'` (padded), and an over-long key fails the whole query with
`EXCEED_LIMIT_LENGTH` instead of becoming a bad record.
- **Duplicates on any datasource scan**, including `spark.read.schema("m
MAP<CHAR(3), INT>")`: the same read-side rewrite rebuilds the map with
`MapFromArrays`, which applies `spark.sql.mapKeyDedupPolicy`. So
`{"m":{"abc":1,"abc":2}}` hits `DUPLICATED_MAP_KEY` under the default
`EXCEPTION` policy, even though the parser kept both pairs.
The new tests cover duplicates only through `from_json`, and the datasource
cases use a user-specified schema with no duplicates, so neither difference is
exercised. I'd suggest scoping the no-pad / bad-record / keep-duplicates
statements to `from_json` (and to the parser check for user-specified reader
schemas), and noting that datasource scans still apply the read-side
CHAR/VARCHAR check and map-key dedup. The same wording is in the PR description.
See **Shared repair plan 1** in the review body.
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -3072,6 +3094,229 @@ 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") {
+ // Exact CHAR and within-limit VARCHAR names are accepted as-is: the
trailing space in
+ // "ab " is preserved for CHAR(3) and not trimmed for VARCHAR(3).
+ 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)))
+ // Binary-distinct names ("ab" vs "ab ") are both kept.
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"ab": 1, "ab ": 2}',
'MAP<VARCHAR(3), INT>'))"""),
+ Row(Seq(Row("ab", 1), Row("ab ", 2))))
+ // Duplicate names are kept, exactly as for STRING keys;
mapKeyDedupPolicy is not applied.
+ // checkAnswer alone would collapse the duplicates when it builds a
Scala Map, so pin the
+ // behavior with size and map_entries.
+ checkAnswer(
+ sql("""SELECT size(from_json('{"abc": 1, "abc": 2}', 'MAP<CHAR(3),
INT>'))"""),
+ Row(2))
+ checkAnswer(
+ sql("""SELECT map_entries(from_json('{"abc": 1, "abc": 2}',
'MAP<CHAR(3), INT>'))"""),
+ Row(Seq(Row("abc", 1), Row("abc", 2))))
+ // Length counts characters (code points), not UTF-16 units or bytes:
one non-BMP code
+ // point is exactly CHAR(1) and is kept verbatim, and the same name is
rejected by CHAR(2).
+ // scalastyle:off nonascii (the \u escape is decoded before the nonascii
check runs)
+ val nonBmp = "\uD83D\uDE00" // U+1F600, one code point stored as a
UTF-16 surrogate pair
+ // scalastyle:on nonascii
+ checkAnswer(
+ sql(s"""SELECT map_keys(from_json('{"$nonBmp": 1}', 'MAP<CHAR(1),
INT>'))[0]"""),
+ Row(nonBmp))
+ checkAnswer(sql(s"""SELECT from_json('{"$nonBmp": 1}', 'MAP<CHAR(2),
INT>')"""), Row(null))
+
+ // Length-0 boundary: CHAR(0)/VARCHAR(0) accept only the empty-string
name, and any
+ // non-empty name is rejected (the empty key is still kept as a real map
entry).
+ checkAnswer(
+ sql("""SELECT from_json('{"": 1}', 'MAP<CHAR(0), INT>')"""),
+ Row(Map("" -> 1)))
+ checkAnswer(
+ sql("""SELECT from_json('{"": 1}', 'MAP<VARCHAR(0), INT>')"""),
+ Row(Map("" -> 1)))
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<CHAR(0),
INT>')"""), Row(null))
+ checkAnswer(sql("""SELECT from_json('{"a": 1}', 'MAP<VARCHAR(0),
INT>')"""), Row(null))
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<CHAR(0), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "CHAR(0)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"a": 1}', 'MAP<VARCHAR(0), INT>', map('mode',
'FAILFAST'))""",
+ key = "a",
+ dataTypeSql = "VARCHAR(0)")
+
+ // A key longer than the 128-character echo cap is truncated with a
trailing "..." in the
+ // error so a huge JSON object name does not bloat the message.
+ val longKey = "a" * 200
+ assertUnsupportedJsonMapKey(
+ s"""SELECT from_json('{"$longKey": 1}', 'MAP<CHAR(3), INT>',
map('mode', 'FAILFAST'))""",
+ key = "a" * 128 + "...",
+ dataTypeSql = "CHAR(3)")
+
+ // Valid keys inside nested containers.
+ checkAnswer(
+ sql("""SELECT from_json('{"outer": {"xy ": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>')""".stripMargin),
+ Row(Map("outer" -> Map("xy " -> 1))))
+ checkAnswer(
+ sql("""SELECT from_json('[{"ab ": 1}]', 'ARRAY<MAP<CHAR(3),
INT>>')"""),
+ Row(Seq(Map("ab " -> 1))))
+
+ // A rejected key makes the row a bad record: null in PERMISSIVE (not
EXCEED_LIMIT_LENGTH),
+ // and in FAILFAST the UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY cause is
surfaced.
+ 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)")
+ // A rejected key inside a nested container is surfaced too (not
swallowed).
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('{"outer": {"a": 1}}',
+ | 'MAP<STRING, MAP<CHAR(3), INT>>', map('mode',
'FAILFAST'))""".stripMargin,
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+ assertUnsupportedJsonMapKey(
+ """SELECT from_json('[{"a": 1}]',
+ | 'ARRAY<MAP<CHAR(3), INT>>', map('mode',
'FAILFAST'))""".stripMargin,
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+
+ // The DataFrame functions.from_json path constructs JsonToStructs the
same way and must
+ // behave identically: accepted key kept, rejected key null in
PERMISSIVE and surfaced in
+ // FAILFAST.
+ checkAnswer(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"ab ": 1}"""), "MAP<CHAR(3), INT>",
Map.empty[String, String])),
+ Row(Map("ab " -> 1)))
+ checkAnswer(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"a": 1}"""), "MAP<CHAR(3), INT>",
Map.empty[String, String])),
+ Row(null))
+ assertUnsupportedJsonMapKeyError(
+ spark.range(1).select(
+ functions.from_json(
+ functions.lit("""{"a": 1}"""),
+ "MAP<CHAR(3), INT>",
+ Map("mode" -> "FAILFAST"))).collect(),
+ key = "a",
+ dataTypeSql = "CHAR(3)")
+
+ // SPARK-60108: a rejected key must not corrupt a sibling field of an
enclosing struct.
+ // The map becomes null but the following `tail` field is still parsed,
and an inner name
+ // ("tail") is not mistaken for the outer field. This holds in both
partial-results modes.
+ Seq(true, false).foreach { partial =>
Review Comment:
**Nit (P3):** Every rejected key in this block, and in the rest of the new
test, has a scalar value (`{"a":1}`, `{"m":{"a":1,"tail":5},"tail":1}`, ...).
For a scalar, the `parser.skipChildren()` in the rejected-key branch of
`convertMap` is a no-op, so deleting it would leave every assertion here
passing. With a container value, that call is what keeps the parser aligned.
Without it, `from_json('{"m":{"a":{"tail":5}},"tail":1}', 'm MAP<CHAR(3), INT>,
tail INT')` would end the map loop at the inner `END_OBJECT`, and the enclosing
struct would then stop at the map's `END_OBJECT`, returning `Row(null, null)`
instead of `Row(null, 1)`. That is the same sibling-corruption class this block
guards against.
Could you add object- and array-valued rejected keys followed by a sibling
to this loop, e.g. `{"m":{"a":{"x":5}},"tail":1}` and
`{"m":{"a":[1,2]},"tail":1}`, expecting `Row(null, 1)` in both partial-results
modes? Values are not converted for a rejected key, so the existing `$schema`
works unchanged.
**Verification:**
- **Regression:** Container-valued rejected keys followed by a sibling
produce Row(null, <sibling>) with the sibling intact, and the case fails if the
value skip is removed.
--
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]
