[SPARK-60108][SQL] Length-check JSON CHAR/VARCHAR map keys without pad or trim

[srielau]

What changes were proposed in this pull request?

When spark.sql.charVarchar.standardSemantics.enabled is true, from_json length-checks JSON object names used as MAP<CHAR(n), _> / MAP<VARCHAR(n), _> keys without rewriting them.

• CHAR(n) keys must already be exactly n characters (no pad).
• VARCHAR(n) keys must already be at most n characters (no trim).
• Duplicate names are kept, exactly as for STRING keys. spark.sql.mapKeyDedupPolicy is not applied.
• Length is counted in characters (code points), so collation and the byte encoding do not affect it.
• Length failures follow the JSON parse mode: in PERMISSIVE mode the enclosing map is set to null (sibling fields are preserved; the whole result is null only when the map is the top-level type); in FAILFAST mode parsing fails.

The same parser-level check applies when reading JSON files with a user-specified CHAR/VARCHAR reader schema. Reads whose CHAR/VARCHAR keys are handled read-side instead (for example catalog tables) are unchanged: there keys are padded, spark.sql.mapKeyDedupPolicy is applied, and an over-long key raises EXCEED_LIMIT_LENGTH.

Mismatched keys raise a new UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY condition (SQLSTATE 0A000) instead of EXCEED_LIMIT_LENGTH, because pad/trim of map keys is not supported. In FAILFAST mode this is surfaced as the cause of MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION (SQLSTATE 22023). JSON map values still use EXCEED_LIMIT_LENGTH. CSV, XML, and TRANSFORM are unchanged.

Why are the changes needed?

SPARK-59274 already applies CHAR pad and VARCHAR overflow to JSON values. Map keys were still taken as the raw object names. Applying write-side pad/trim to keys would rewrite names and could invent collisions, so this change only validates length.

Does this PR introduce any user-facing change?

Yes, under spark.sql.charVarchar.standardSemantics.enabled=true:

-- PERMISSIVE (default): the map is set to null (the whole result here, since the map is the
-- top-level type).
SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>');
-- NULL

-- FAILFAST: the new condition is the cause of MALFORMED_RECORD_IN_PARSING.
SELECT from_json('{"a": 1}', 'MAP<CHAR(3), INT>', map('mode', 'FAILFAST'));
-- [MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION] ... SQLSTATE: 22023
-- Caused by: [UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY] The JSON object name 'a' is not a valid
-- key for the map key type "CHAR(3)". A CHAR(n) key must be exactly n characters and a
-- VARCHAR(n) key at most n characters; keys are never padded or trimmed. Use a STRING map key
-- to accept any name. SQLSTATE: 0A000

Exact-width CHAR keys and in-limit VARCHAR keys are kept as-is, including trailing spaces. With the flag off, keys remain the raw object names.

How was this patch tested?

Added SPARK-60108 coverage in BasicCharVarcharTestSuite: exact-width CHAR with spaces, CHAR/VARCHAR overflow, binary-distinct "ab" vs "ab ", kept duplicates (checked via size/map_entries), a non-BMP key, PERMISSIVE vs FAILFAST, a bad key followed by a surviving sibling, nested MAP/ARRAY of maps, the multiLine top-level-array row count, collation shown in the error (and not consulted for the length check), a view created flag-on and queried flag-off, and the flag-off round trip through to_json(from_json(...)). Also re-ran the SPARK-59274 JSON map value overflow tests and SparkThrowableSuite error-condition formatting.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Cursor

[dtenedor]

Thanks for working on this. Requiring CHAR keys to be exactly n characters is a nice way to avoid PAD SPACE equality questions for map keys.

I left inline comments. The main ones:

1. A rejected key throws while the parser is still inside the map, which corrupts sibling fields in PERMISSIVE mode and adds spurious rows with enableStreamingTopLevelArray (see convertMap).
2. The flag is read via SQLConf.get when the parser is built, but it has PERSISTED binding.
3. Duplicate-key handling now differs from both STRING keys and the XML path.

The rest covers the SQLSTATE and message, test gaps, and the migration-guide entry.

One note on the PR description: in FAILFAST mode the top-level error is MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION (SQLSTATE 22023), with UNSUPPORTED_JSON_CHAR_VARCHAR_MAP_KEY only as its cause, so the example output there isn't what users will see.

[HyukjinKwon]

Review summary

The parser change looks right to me. The rejected-key branch in convertMap steps past the value before rethrowing, so enclosing structs stay aligned. The key checker is built once per map converter behind an order-safe lazy val. from_json captures the PERSISTED standard-semantics flag at analysis time, so views keep their creation-time behavior. The main points from the earlier review (parser position after a rejected key, flag binding for views, and duplicate handling in from_json) are all addressed in this head.

What remains is documentation accuracy and one test gap:

• The new migration-guide entry says the JSON datasource validates keys without pad or trim and keeps duplicates. Datasource scans still go through the existing read-side CHAR/VARCHAR handling: catalog tables pad keys and fail with EXCEED_LIMIT_LENGTH, and every scan applies mapKeyDedupPolicy when the map is rebuilt.
• The same entry says PERMISSIVE "drops" the record. The PR's own tests show the row and its sibling fields are kept, and only the map is nulled.
• The sibling-integrity tests use only scalar values for rejected keys, so the skipChildren() that handles object or array values has no regression coverage.

One edit to the migration-guide bullet, plus the matching PR-description text, fixes the first two.

Findings

3 total: 0 P0, 0 P1, 1 P2, 2 P3.

Non-blocking (P2)

• Migration guide overstates JSON datasource CHAR/VARCHAR map-key semantics — docs/sql-migration-guide.md:31 — see inline.

Nit (P3)

• Migration guide says PERMISSIVE drops the record for a rejected map key — docs/sql-migration-guide.md:31
  Separately from the datasource scoping, the PERMISSIVE description in the same migration-guide bullet (docs/sql-migration-guide.md:31) doesn't match the PR's own tests: "... in PERMISSIVE mode it is dropped (the output is null)". "Dropped" is DROPMALFORMED terminology, and the output is entirely null only when the CHAR/VARCHAR map is the from_json root. In every other case the record is kept and only the failing map is nulled:

  • from_json('{"m":{"a":1},"tail":1}', 'm MAP<CHAR(3), INT>, tail INT') returns Row(null, 1) in both partial-results modes.
  • The multiLine datasource test expects Row(null, document): the row is kept, m is null, and _corrupt_record is populated.

  Wording like "in PERMISSIVE mode the map is set to null (the whole result is null when the map is the root of from_json)" would match the tests. The PR description's "the bad record is dropped (the row/value is null)" has the same problem.

  See Shared repair plan 1 in the review body.

• No test covers a rejected map key whose value is an object or array — sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:3221 — see inline.

Shared repair plans

Shared repair plan 1

Covered findings:

• Migration guide overstates JSON datasource CHAR/VARCHAR map-key semantics — docs/sql-migration-guide.md:31
• Migration guide says PERMISSIVE drops the record for a rejected map key — docs/sql-migration-guide.md:31

Verification:

• Inspection: Every behavioral statement in the revised entry matches the PR's own test expectations for from_json, nested struct siblings, and multiLine datasource rows, and matches the read-side CHAR/VARCHAR handling of catalog-table and datasource scans.

Existing discussions

• existing discussion — Fixed in the current source. convertMap now runs the key check inside the per-entry loop and, on rejection, records the cause, steps onto the value, and skips its children, so the loop consumes the map's END_OBJECT and the unbalanced-buffer rethrow surfaces the real cause. The new tests cover a sibling field in both partial modes and the multiLine streaming-array row count. One gap remains: every rejected key in those tests has a scalar value, so the skipChildren() step that handles object/array values is unexercised.
• existing discussion — The review's main items are addressed in the current source for from_json. A rejected key is now handled inside the per-entry loop: the parser steps onto the value, skips its children, and leaves the key unpaired, so sibling fields and streaming row counts are preserved. The flag is captured at analysis in JsonToStructs. Duplicate keys now follow keep-all on the parser path. The PR description now shows MALFORMED_RECORD_IN_PARSING.WITHOUT_SUGGESTION (22023) with the new condition as its cause. The duplicate-key concern (item 3) still applies on the datasource path, however: the pre-existing read-side CHAR/VARCHAR check rebuilds scanned maps with MapFromArrays, applying mapKeyDedupPolicy, and catalog tables still pad keys. That gap requires fresh delivery.

Verification

• convertMap's rejected-key branch steps from FIELD_NAME onto the value and skips its children before the loop continues, then rethrows the recorded cause instead of building an unbalanced ArrayBasedMapData.
• JsonToStructs captures spark.sql.charVarchar.standardSemantics.enabled at analysis time and passes it to JacksonParser as Some(...) through JsonToStructsEvaluator, and the new view test asserts the key check survives a flag-off session.

PR description suggestions

• The PR description says from_json and the JSON datasource length-check CHAR/VARCHAR map keys without pad or trim and keep duplicates with spark.sql.mapKeyDedupPolicy not applied. JSON datasource scans still go through the read-side CHAR/VARCHAR handling: catalog JSON tables pad keys and raise EXCEED_LIMIT_LENGTH, and every scan rebuilds the map with MapFromArrays, which applies mapKeyDedupPolicy. Consider scoping these statements to from_json and to the parser check for user-specified reader schemas.
• The description says that in PERMISSIVE mode "the bad record is dropped (the row/value is null)", and the SQL example comment says "the bad record is dropped". Only a root-level map produces a null result. Nested maps null just the map and keep sibling fields, and JSON datasource rows are kept with _corrupt_record populated, as the added tests assert. Consider rewording to avoid "dropped", which is DROPMALFORMED terminology.

Generated by Omnigent on Databricks.

[HyukjinKwon]

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.

[srielau]

Good catch, and confirmed against the read-side code. CharVarcharUtils.processStringForCharVarchar rebuilds a MapType with MapFromArrays (applying mapKeyDedupPolicy) and pads CHAR keys via charTypeWriteSideCheck / raises EXCEED_LIMIT_LENGTH, so the "no pad/trim, keep-all, mapKeyDedupPolicy not applied" statements only hold for the parser path (from_json, and reads with a user-specified CHAR/VARCHAR reader schema) - not for reads whose keys are handled read-side (e.g. catalog tables).

I rescoped the migration-guide bullet to from_json, noted that the same parser check runs for user-specified JSON reader schemas, and spelled out that read-side-handled reads are unchanged (keys padded, mapKeyDedupPolicy applied, over-long key -> EXCEED_LIMIT_LENGTH).

I also fixed the PERMISSIVE wording in the same bullet: it no longer says the record is "dropped". It now says the enclosing map is set to null while sibling fields are preserved, and the whole result is null only when the map is the top-level type (matching the nested-struct and multiLine datasource tests). The PR description got both fixes too.

[HyukjinKwon]

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, ) with the sibling intact, and the case fails if the value skip is removed.

[srielau]

Added. There is now a sub-block (run in both partial-results modes) with rejected keys whose values are an object (MAP<CHAR(3), STRUCT<x: INT>>) and an array (MAP<CHAR(3), ARRAY<INT>>): it asserts the map is nulled while the sibling tail field survives, and that FAILFAST still surfaces the key. That exercises the parser.nextToken() + parser.skipChildren() step over a nested value, which the previous scalar-only cases did not cover.