Repository navigation
Conversation
|
Follow-up for the SQL review of this PR:
|
… or trim When standard CHAR/VARCHAR semantics are enabled, XML element and attribute names used as MAP keys are length-checked in place: CHAR keys must already be exactly n characters, and VARCHAR keys at most n. Failures use UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY and follow PERMISSIVE/FAILFAST. convertMap drains the current element so ARRAY<MAP<...>> key mismatches do not desync the parser.
HyukjinKwon
left a comment
There was a problem hiding this comment.
Review summary
The flag-on key checks are implemented consistently across element names, prefixed attributes, and the valueTag, and the tests cover PERMISSIVE/FAILFAST, ignoreCorruptFiles, nested maps, ARRAY<MAP>, and sibling recovery well. The main issue is outside the declared scope: the new direct MapType arm in convertObject changes results for ordinary MAP<STRING, _> struct fields with the flag off (empty, attribute-only, and text-only elements), and nothing documents or tests that. The widened key-type match also lets non-string key types reach convertMap through the unvalidated format("xml").load() path, where they now fail with a ClassCastException instead of a malformed record. Two text issues remain: the convertMap scaladoc omits the flag condition, and the migration-guide entry misstates the flag-off precondition and the 4.3 baseline. Collated keys keeping both ab and AB under exact-name last-win is consistent with the PR's stated design and is not raised.
Review-level concerns
- Shared problem: The XML map dispatch rewrite reaches beyond the declared CHAR/VARCHAR key change. The new convertObject MapType arm changes MAP<STRING, _> results for empty, attribute-only, and text-only elements with the flag off. The catch-all
MapType(kt, ...)lets non-string key types produce mistyped maps that fail with ClassCastException. Narrow the dispatch in StaxXmlParser together: drop the direct struct-field arm and restrict the remaining map arm to StringType-family keys. That keeps the new key-length checks and returns everything else to merge-target behavior. Both defects are P2/P3, so this is non-blocking.
Findings
4 total: 0 P0, 0 P1, 1 P2, 3 P3.
Non-blocking (P2)
-
New struct-field MapType arm changes ordinary MAP<STRING, _> results with the flag off —
General
The newcase mt: MapType => convertMap(...)arm inconvertObject(StaxXmlParser.scala ~650) routes every map-typed struct field aroundconvertField's peek-based dispatch. That includes plainMAP<STRING, _>fields withspark.sql.charVarchar.standardSemantics.enabledoff. Under default configuration, forschema("m MAP<STRING, STRING>"):<ROW><m/></ROW>used to givem = NULL(convertField's(EndElement, _)arm) and now gives an empty map.<ROW><m a="1"/></ROW>used to give NULL and now gives{_a -> 1}.<ROW><m>xy</m></ROW>used to be a malformed record (convertToraises_LEGACY_ERROR_TEMP_3246) and now gives{_VALUE -> xy}.
The same element inside
ARRAY<MAP<...>>or as a nested map value still goes throughconvertField, so the result now depends on where the map sits. The PR description and migration note scope the change to CHAR/VARCHAR keys under the flag, and no test pins these STRING-key results. Unless this change is intended and documented, struct-field maps should keep going throughconvertField. Its StartElement path already reachesconvertMap, and with it the new key checks, for non-empty elements.Recommended change: Remove the direct MapType arm from convertObject so struct-field maps use convertField's existing dispatch again. Update the CharVarcharTestSuite cases that depended on that arm: attribute-only
<m ab="1"></m>and text-only<m>xy</m>should assert the restored results or use element-content inputs that still exercise the prefixed-attribute and valueTag key checks. Add XML map tests pinning the merge-target results for empty, attribute-only, and text-only MAP<STRING, _> struct fields.Why this works: convertField's peek decides empty and text-only elements before convertMap runs, so MAP<STRING, _> results return to merge-target behavior. For non-empty elements, convertComplicatedType still calls convertMap with the element's attributes, so the CHAR/VARCHAR key checks and bounded-drain recovery are unchanged.
Scope: Restore the merge-target struct-field map dispatch in the XML parser and realign the affected CHAR/VARCHAR tests, adding STRING-key regression coverage.
Compatibility: UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY checks and sibling recovery for non-empty CHAR/VARCHAR map elements.
Risks: Tests that pinned the attribute-only and text-only results of the direct arm must be updated, not deleted, so the prefixed-attribute and valueTag key checks stay covered.
Constraints: Keep the bounded-drain sibling recovery for key and value failures in non-empty map elements.
Success: With default configuration, a MAP<STRING, _> struct field whose element is empty or whitespace-only is null, an attribute-only element is null, and a text-only element is a malformed record, as in the merge target. The same map element gives the same result as a struct field, an ARRAY element, and a nested map value. CHAR/VARCHAR key length checks under the flag still apply to every non-empty map element, including prefixed attributes and the valueTag in mixed content, with sibling recovery.
Verification:
- Behavior: from_xml and XML datasource reads of MAP<STRING, _> fields with empty, attribute-only, and text-only elements return the merge-target null or malformed-record results under default configuration.
- Compatibility: Struct-field, ARRAY element, and nested-map-value placements of the same map element produce the same result.
- Regression: CHAR/VARCHAR key-check coverage (PERMISSIVE/FAILFAST, nested maps, ARRAY, tail recovery, prefixed attributes, valueTag in mixed content) still passes.
Nit (P3)
-
Widened map dispatch lets non-string key types build mistyped maps —
General
convertComplicatedType's map arm is nowcase MapType(kt, vt, _)(StaxXmlParser.scala ~388), and the newconvertObjectarm accepts anyMapType. For a non-CHAR/VARCHAR key type,convertXmlMapKeyfalls back toCharVarcharUtils.applyTextParseSemantics, which returns theUTF8Stringunchanged.ExprUtils.checkXmlSchema(INVALID_XML_MAP_KEY_TYPE) runs only forfrom_xmlandDataFrameReader/DataStreamReader.xml(). Meanwhilespark.read.format("xml").schema("m MAP<INT, STRING>").load(path)andCREATE TABLE ... USING xmlskip it, andXmlFileFormat.supportDataTypeaccepts atomic key types. On those paths,<ROW><m><a>x</a></m></ROW>used to hit aMatchErrorthat became a malformed record (NULL in PERMISSIVE). It now buildsMapDatawithUTF8Stringkeys underIntegerType, which fails in the scan projection (getInt) with aClassCastExceptionin every parse mode. Restricting the map arm to string-family key types (for exampleMapType(_: StringType, vt, _), which CHAR/VARCHAR and collated strings satisfy) would keep the old failure mode.Verification:
- Regression: Reading
m MAP<INT, STRING>through format("xml").load() returns null in PERMISSIVE and fails as a malformed record in FAILFAST, with no ClassCastException. - Behavior: Existing CHAR/VARCHAR and collated XML map-key tests still pass.
- Regression: Reading
-
convertMap scaladoc states no-pad key semantics without the flag condition —
General
TheconvertMapscaladoc ("XML names used as CHAR/VARCHAR keys are length-checked without rewriting ... Padding, trimming, and mapKeyDedupPolicy are not applied", plus theMAP<CHAR(4), INT>example) reads as unconditional.convertXmlMapKeyapplies those semantics only whencharVarcharStandardSemanticsis true. Otherwise keys go throughCharVarcharUtils.applyTextParseSemantics, which pads CHAR keys and raisesEXCEED_LIMIT_LENGTH, as the flag-off test's expected key'a 'shows. Qualifying the paragraph with the flag condition would keep it accurate.Verification:
- Inspection: The scaladoc's stated key semantics match convertXmlMapKey for both flag values and agree with the flag-off test's padded-key expectation.
-
Migration-guide entry misstates the flag-off precondition, the 4.3 baseline, and the checked keys —
General
A few things in the newdocs/sql-migration-guide.mdbullet could mislead upgrading users:- "With the flag off, CHAR keys are still padded and VARCHAR overflow still uses
EXCEED_LIMIT_LENGTH" holds only whenspark.sql.preserveCharVarcharTypeInfois true. The flag-off test sets it, and the adjacent bullet states that precondition. With both flags at their defaults, CHAR/VARCHAR schema types never reach the parser. - "still" doesn't match 4.3. In
branch-4.3the only XML map arm iscase MapType(StringType, vt, _), andStringType.equalscompares the length constraint, soMAP<CHAR(n)/VARCHAR(n), _>XML maps were malformed records there. Padding is new in 4.4. - The checked keys include the
attributePrefix+ local name (default_ab) and thevalueTag(default_VALUE). Under the flag, any CHAR(n != 6) / VARCHAR(n < 6) map whose element has text content therefore fails, which users would want to know. - "XML CHAR/VARCHAR values still use write-side pad" describes a parse-time check with write-path wording, while the bullet just above calls the same check "Read-side length checks".
Verification:
- Inspection: Each statement in the bullet matches convertXmlMapKey/convertMap behavior, the flag-off test's preconditions, and upstream/branch-4.3's map dispatch.
- "With the flag off, CHAR keys are still padded and VARCHAR overflow still uses
PR description suggestions
- The body's "With the flag off, CHAR keys are still padded and VARCHAR overflow still uses
EXCEED_LIMIT_LENGTH" applies only whenspark.sql.preserveCharVarcharTypeInfois true. Consider stating that precondition, as the adjacent 4.4 migration bullet does.
Generated by Omnigent on Databricks.
|
Thanks for the summary. No rebase is needed for review: the current head (02b478e) is already a single commit on top of master (215a146). Items 1 and 2 don't seem to match that head, though. |
Drop the direct convertObject MapType arm so empty, attribute-only, and
text-only MAP<STRING> elements stay null or malformed. Restrict convertMap
to StringType-family keys so format("xml") MAP<INT> does not ClassCast.
|
@HyukjinKwon Thanks for catching the stale follow-up. That comment described an intermediate revision. After the squash,
|
HyukjinKwon
left a comment
There was a problem hiding this comment.
Review summary
This round reviews 8e1c55b. The previous review at 02b478e raised four findings. Three are fully addressed. Struct-field maps go through convertField again, so MAP<STRING, _> results match the merge target. The map arm admits only string-family keys, so MAP<INT, _> read through format("xml").load() is a malformed record again, with a test pinning it. The convertMap scaladoc now states the flag condition. The fourth, migration-guide accuracy, is mostly addressed: the bullet now names the checked keys and the preserveCharVarcharTypeInfo precondition, and its wording is fixed.
Two non-blocking P3 nits remain. First, the 4.3 sentence added to the migration bullet doesn't match branch-4.3 for from_xml and reader schemas, and the bullet and the scaladoc omit that flag-off over-length CHAR keys raise EXCEED_LIMIT_LENGTH. That sentence follows the earlier review's own description of 4.3, which looked only at parser dispatch and was incomplete. Second, the DUPLICATED_MAP_KEY routing in StaxXmlParser, the DuplicateMapKeyUtils object, and two test helpers no longer have a producer. That was already true at 02b478e, and the earlier review missed it.
There is also one question about whether the new key checks should follow the flag's persisted view value rather than the caller's session.
Findings
2 total: 0 P0, 0 P1, 0 P2, 2 P3.
Nit (P3)
-
Migration note misstates the 4.3 baseline and omits flag-off CHAR key overflow —
docs/sql-migration-guide.md:32— see inline. -
Dead DUPLICATED_MAP_KEY routing and test helpers left after removing XML dedup —
General
WithconvertConstrainedMapandDuplicateMapKeyUtils.buildConstrainedMapremoved, nothing on the XML parse path can raiseDUPLICATED_MAP_KEYanymore;convertMapbuildsArrayBasedMapData(kvPairs.toMap). The SPARK-59274 plumbing that let that error escape parse-mode handling is still there:DuplicateMapKeyUtils, now onlycause/unapply, and its import inStaxXmlParser;- five special cases in
StaxXmlParser: thecase DuplicateMapKeyUtils(e) => throw earms indoParseColumn(line 238),assignAttributes(560), andconvertObject(699); theDuplicateMapKeyUtils.causebranch indoParseColumnOptimized'sPartialResultExceptionarm (335); and the root-cause arm in itsThrowablehandler (344); - the private
assertDuplicateMapKey/assertDuplicateMapKeyErrorhelpers inCharVarcharTestSuite(line 1069), which no test calls anymore.
None of this can run, but it tells readers that XML parsing can still raise
DUPLICATED_MAP_KEYand that the error must bypass PERMISSIVE andignoreCorruptFiles. That contradicts the updatedspark.sql.mapKeyDedupPolicydoc. Any future producer of that error would inherit the bypass without anyone having decided it should. Deleting the object, the five arms, and the two helpers changes no behavior; after the deletion, thePartialResultExceptionarm simply always throwsBadRecordException.Verification:
- Compile: The catalyst main sources and the sql core test sources compile with DuplicateMapKeyUtils and the helpers removed.
- Regression: Existing XML CHAR/VARCHAR map-key and value tests keep passing under PERMISSIVE, FAILFAST, and ignoreCorruptFiles, including sibling recovery after a key-check failure.
- Inspection: No remaining reference to DuplicateMapKeyUtils or DUPLICATED_MAP_KEY special-casing exists in the XML parser.
Re-review status
Prior AI findings: 4 addressed, 0 still present; additional unresolved findings in this review: 2.
New attribution: 1 newly introduced, 1 late catch, 0 previously raised, 0 unattributed.
Remaining prior AI findings
No prior AI findings remain.
Existing discussions
- existing discussion — Each claim checked against 8e1c55b. (1) convertObject sends map-typed struct fields through convertField again, so empty and attribute-only elements hit the EndElement arm (NULL) and text-only elements hit convertTo (malformed); new tests pin all three plus an ARRAY empty element. (2) convertComplicatedType matches only MapType(kt: StringType, ...), and a new format("xml").load() test asserts MAP<INT, STRING> is a malformed record with no ClassCastException. (3) The prefixed-attribute and valueTag tests use mixed content. (4) The convertMap scaladoc qualifies the no-pad semantics with the flag, and the migration bullet names attributePrefix/valueTag and the preserveCharVarcharTypeInfo precondition. The '4.3 malformed-record baseline' the author added, at the earlier review's prompting, is inaccurate. In branch-4.3, from_xml and DataFrameReader schemas rejected CHAR/VARCHAR at analysis (UNSUPPORTED_CHAR_OR_VARCHAR_AS_STRING) or mapped them to STRING under spark.sql.legacy.charVarcharAsString. The earlier review described only the parser dispatch. That correction, plus the omitted flag-off CHAR overflow error, is the related migration-guide finding.
PR description suggestions
- The body repeats the migration bullet's "In Spark 4.3,
MAP<CHAR(n), _>/MAP<VARCHAR(n), _>XML maps were malformed records" and describes flag-off CHAR keys only as padded. Consider updating both to match the corrected note. In 4.3,from_xmland reader schemas rejected CHAR/VARCHAR, or treated them as STRING underspark.sql.legacy.charVarcharAsString. With the flag off, over-length CHAR keys also raiseEXCEED_LIMIT_LENGTH.
Decision challenges
Should XML CHAR/VARCHAR key checks follow the view's persisted flag value?
StaxXmlParser now reads spark.sql.charVarchar.standardSemantics.enabled from SQLConf.get when the parser is constructed (line 89). For from_xml that happens when the evaluator is built at execution; the XML file readers build a parser per task.
What I verified: the flag is declared with ConfigBindingPolicy.PERSISTED, and its comment says a view created under standard semantics must keep CHAR/VARCHAR behavior regardless of the caller's session. effectiveSQLConf is consulted only during view resolution (ViewResolution, ViewResolver, SessionCatalog), and neither XmlToStructs nor XmlOptions captures the flag. In the merge target, CHAR/VARCHAR XML map keys always went through applyTextParseSemantics, so key handling did not depend on the flag. Take a view created with the flag on over from_xml(x, 'm MAP<CHAR(2), INT>'). It keeps CHAR(2) in its schema, but a caller with the flag off appears to get the flag-off key path: a is padded to a instead of rejected, and over-length keys raise EXCEED_LIMIT_LENGTH instead of UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY.
What I couldn't confirm: I didn't run a cross-session view query. ToStringBase also reads this flag at runtime for CAST, so runtime binding may be intended for the feature. Is it intended here? If not, the flag could be captured at analysis, for example on XmlToStructs and the XML read options the way timeZoneId is, and passed to the parser.
Generated by Omnigent on Databricks.
| ## Upgrading from Spark SQL 4.3 to 4.4 | ||
|
|
||
| - Since Spark 4.4, when `spark.sql.preserveCharVarcharTypeInfo` is true and `spark.sql.charVarchar.standardSemantics.enabled` is false, ORC reads that apply a CHAR/VARCHAR schema over STRING storage return the stored values without ORC truncation, matching Parquet. Previously the ORC reader requested `char(n)`/`varchar(n)` and truncated STRING-stored values to `n`. Read-side length checks (`EXCEED_LIMIT_LENGTH`) apply only when `spark.sql.charVarchar.standardSemantics.enabled` is true. | ||
| - Since Spark 4.4, when `spark.sql.charVarchar.standardSemantics.enabled` is true, XML names used as `MAP<CHAR(n), _>` or `MAP<VARCHAR(n), _>` keys in `from_xml` and the XML datasource are length-checked without padding or trimming. The checked names are element names, `attributePrefix` plus attribute names (default prefix `_`), and the `valueTag` (default `_VALUE`) when mixed text is present. 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_XML_CHAR_VARCHAR_MAP_KEY` (SQLSTATE `0A000`) and follow the XML parse mode (`PERMISSIVE` or `FAILFAST`). Exact repeated names last-win; `spark.sql.mapKeyDedupPolicy` is not applied. XML CHAR/VARCHAR values use the same pad and `EXCEED_LIMIT_LENGTH` checks as other parsed text. Empty, attribute-only, and text-only map elements stay SQL NULL or a malformed record, matching 4.3; they do not become an empty map or a `valueTag` entry. In Spark 4.3, `MAP<CHAR(n), _>` and `MAP<VARCHAR(n), _>` XML maps were malformed records because only unbounded STRING keys were accepted. Padding of CHAR keys and VARCHAR `EXCEED_LIMIT_LENGTH` apply when the standard-semantics flag is false and `spark.sql.preserveCharVarcharTypeInfo` is true, so first-class CHAR/VARCHAR types still reach the parser. |
There was a problem hiding this comment.
Nit (P3): Two statements in this bullet don't match the code.
- "In Spark 4.3,
MAP<CHAR(n), _>andMAP<VARCHAR(n), _>XML maps were malformed records": in branch-4.3,from_xml(viaExprUtils.evalTypeExpr) andDataFrameReader.schemaboth callfailIfHasCharVarchar. It throwsUNSUPPORTED_CHAR_OR_VARCHAR_AS_STRINGunlessspark.sql.legacy.charVarcharAsStringis true, in which case it replaces CHAR/VARCHAR with STRING and the map parses as a plain STRING map. So for the entry points this bullet names, 4.3 gave an analysis error (or a STRING map), not a malformed record. A malformed record only happened where a CHAR/VARCHAR key type actually reached the parser. My earlier review described 4.3 from the parser dispatch alone, which is where this sentence came from. - "Padding of CHAR keys and VARCHAR
EXCEED_LIMIT_LENGTHapply when the standard-semantics flag is false ...": with the flag off, an over-length CHAR key also raisesEXCEED_LIMIT_LENGTH.charTypeWriteSideCheckpasses it totrimTrailingSpaces, and XML names have no trailing spaces, so<abcd>underCHAR(3)fails, for example. TheconvertMapscaladoc ("With the flag off, CHAR keys are padded and VARCHAR overflow uses EXCEED_LIMIT_LENGTH") has the same gap.
Describing the 4.3 behavior per entry point, and saying in both places that over-length CHAR or VARCHAR keys raise EXCEED_LIMIT_LENGTH with the flag off, would make this accurate.
Verification:
- Inspection: Each statement in the bullet and scaladoc matches upstream/branch-4.3's failIfHasCharVarchar handling and the head's convertXmlMapKey/charTypeWriteSideCheck behavior for both flag values.
- Behavior: The flag-off test coverage asserts EXCEED_LIMIT_LENGTH for an over-length CHAR key, alongside the existing padding and VARCHAR overflow checks.
There was a problem hiding this comment.
Fixed. The 4.3 sentence now describes failIfHasCharVarchar: UNSUPPORTED_CHAR_OR_VARCHAR_AS_STRING, or STRING under spark.sql.legacy.charVarcharAsString. Flag-off over-length CHAR or VARCHAR keys raise EXCEED_LIMIT_LENGTH in both the bullet and the convertMap scaladoc. There is a flag-off CHAR(3) / <abcd> test next to the existing VARCHAR overflow check.
38a81e3 also deletes DuplicateMapKeyUtils and the five StaxXmlParser arms plus the unused test helpers. PartialResultException always becomes BadRecordException.
|
Thanks, I checked 8e1c55b. Struct-field maps go through |
|
@HyukjinKwon Thanks. The 4.3 baseline and flag-off CHAR overflow are corrected in the migration bullet and I also removed the dead SPARK-59274 On the view-flag question: I intend to keep session binding. |
… or trim ### What changes were proposed in this pull request? When `spark.sql.charVarchar.standardSemantics.enabled` is true, `from_xml` and the XML datasource length-check XML names used as `MAP<CHAR(n), _>` / `MAP<VARCHAR(n), _>` keys without rewriting them. This matches SPARK-60108 for JSON. The checked names are element names, `attributePrefix` plus attribute names (default `_`), and the `valueTag` (default `_VALUE`) when mixed text is present. - `CHAR(n)` keys must already be exactly `n` characters (no pad). - `VARCHAR(n)` keys must already be at most `n` characters (no trim). - Exact repeated names last-win. `spark.sql.mapKeyDedupPolicy` is not applied. - Length failures follow the XML parse mode (`PERMISSIVE` / `FAILFAST`). Mismatched keys raise a new `UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY` condition (SQLSTATE `0A000`) instead of `EXCEED_LIMIT_LENGTH`, because pad/trim of map keys is not supported. XML CHAR/VARCHAR values still use the same pad and `EXCEED_LIMIT_LENGTH` checks as other parsed text. JSON, CSV, and TRANSFORM are unchanged. Struct-field maps still go through `convertField`. Empty, attribute-only, and text-only `MAP<STRING, _>` elements stay SQL NULL or a malformed record, as on master. `convertMap` is used for non-empty string-family keys (including CHAR/VARCHAR) and owns element bounding so a key-check failure still drains `ARRAY<MAP<...>>` siblings. This PR drops `convertConstrainedMap` and `DuplicateMapKeyUtils`. SPARK-59722-style assign-after-parse is not used. JIRA: https://issues.apache.org/jira/browse/SPARK-60103 ### Why are the changes needed? SPARK-59274 padded and trimmed CHAR/VARCHAR XML map keys during the walk, then applied `mapKeyDedupPolicy` to invented collisions. SPARK-59722 would have done the same via write-side assignment after a STRING parse; that approach was abandoned for JSON in favor of SPARK-60108. XML names are the key identity, so they should be length-checked in place rather than rewritten. ### Does this PR introduce _any_ user-facing change? Yes, under `spark.sql.charVarchar.standardSemantics.enabled=true`: ```sql SELECT from_xml('<ROW><m><a>1</a></m></ROW>', 'm MAP<CHAR(2), INT>', map('mode', 'FAILFAST')); -- [UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEY] XML map keys of CHAR or VARCHAR cannot be padded or trimmed. -- The key 'a' is not valid for type "CHAR(2)". SQLSTATE: 0A000 ``` Exact-width CHAR keys and in-limit VARCHAR keys are kept as-is. Repeated names last-win. When the standard-semantics flag is false and `spark.sql.preserveCharVarcharTypeInfo` is true, short CHAR keys are padded and over-length CHAR or VARCHAR keys raise `EXCEED_LIMIT_LENGTH`. Documented in `docs/sql-migration-guide.md` (Spark SQL 4.3 to 4.4). In Spark 4.3, `from_xml` and XML reader schemas rejected CHAR/VARCHAR with `UNSUPPORTED_CHAR_OR_VARCHAR_AS_STRING`, or replaced them with STRING when `spark.sql.legacy.charVarcharAsString` was true. ### How was this patch tested? Added SPARK-60103 coverage in `BasicCharVarcharTestSuite` (too-short CHAR, exact-width CHAR, CHAR/VARCHAR overflow, last-wins duplicates, prefixed attributes and `valueTag` in mixed content, PERMISSIVE vs FAILFAST, nested maps, `ARRAY<MAP>`, collated keys, `from_xml` and the XML datasource, empty/attribute-only/text-only `MAP<STRING>`, `format("xml")` `MAP<INT>`, flag-off pad/overflow including over-length CHAR). Also re-ran SPARK-59274 XML value, sibling, and `rowTag` tests. Ran: ``` JAVA_HOME=/usr/lib/jvm/java-17-openjdk-amd64 build/sbt -Dsbt.override.build.repos=true \ 'sql/testOnly org.apache.spark.sql.BasicCharVarcharTestSuite -- -z SPARK-60103 -z "SPARK-59274: from_json/csv/xml" -z "SPARK-59274: ordinary STRING" -z "SPARK-59274: collated" -z "SPARK-59274: XML rowTag" -z "SPARK-59274: mixed XML" -z "SPARK-59274: nested XML"' ``` ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Cursor Grok 4.6 Closes #59316 from srielau/SPARK-60103. Authored-by: Serge Rielau <serge@rielau.com> Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com> (cherry picked from commit ccc8f69) Signed-off-by: Hyukjin Kwon <hyukjin.kwon@databricks.com>
What changes were proposed in this pull request?
When
spark.sql.charVarchar.standardSemantics.enabledis true,from_xmland the XML datasource length-check XML names used asMAP<CHAR(n), _>/MAP<VARCHAR(n), _>keys without rewriting them. This matches SPARK-60108 for JSON.The checked names are element names,
attributePrefixplus attribute names (default_), and thevalueTag(default_VALUE) when mixed text is present.CHAR(n)keys must already be exactlyncharacters (no pad).VARCHAR(n)keys must already be at mostncharacters (no trim).spark.sql.mapKeyDedupPolicyis not applied.PERMISSIVE/FAILFAST).Mismatched keys raise a new
UNSUPPORTED_XML_CHAR_VARCHAR_MAP_KEYcondition (SQLSTATE0A000) instead ofEXCEED_LIMIT_LENGTH, because pad/trim of map keys is not supported. XML CHAR/VARCHAR values still use the same pad andEXCEED_LIMIT_LENGTHchecks as other parsed text. JSON, CSV, and TRANSFORM are unchanged.Struct-field maps still go through
convertField. Empty, attribute-only, and text-onlyMAP<STRING, _>elements stay SQL NULL or a malformed record, as on master.convertMapis used for non-empty string-family keys (including CHAR/VARCHAR) and owns element bounding so a key-check failure still drainsARRAY<MAP<...>>siblings.This PR drops
convertConstrainedMapandDuplicateMapKeyUtils. SPARK-59722-style assign-after-parse is not used.JIRA: https://issues.apache.org/jira/browse/SPARK-60103
Why are the changes needed?
SPARK-59274 padded and trimmed CHAR/VARCHAR XML map keys during the walk, then applied
mapKeyDedupPolicyto invented collisions. SPARK-59722 would have done the same via write-side assignment after a STRING parse; that approach was abandoned for JSON in favor of SPARK-60108. XML names are the key identity, so they should be length-checked in place rather than rewritten.Does this PR introduce any user-facing change?
Yes, under
spark.sql.charVarchar.standardSemantics.enabled=true:Exact-width CHAR keys and in-limit VARCHAR keys are kept as-is. Repeated names last-win. When the standard-semantics flag is false and
spark.sql.preserveCharVarcharTypeInfois true, short CHAR keys are padded and over-length CHAR or VARCHAR keys raiseEXCEED_LIMIT_LENGTH.Documented in
docs/sql-migration-guide.md(Spark SQL 4.3 to 4.4). In Spark 4.3,from_xmland XML reader schemas rejected CHAR/VARCHAR withUNSUPPORTED_CHAR_OR_VARCHAR_AS_STRING, or replaced them with STRING whenspark.sql.legacy.charVarcharAsStringwas true.How was this patch tested?
Added SPARK-60103 coverage in
BasicCharVarcharTestSuite(too-short CHAR, exact-width CHAR, CHAR/VARCHAR overflow, last-wins duplicates, prefixed attributes andvalueTagin mixed content, PERMISSIVE vs FAILFAST, nested maps,ARRAY<MAP>, collated keys,from_xmland the XML datasource, empty/attribute-only/text-onlyMAP<STRING>,format("xml")MAP<INT>, flag-off pad/overflow including over-length CHAR). Also re-ran SPARK-59274 XML value, sibling, androwTagtests.Ran:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Grok 4.6