LuciferYang opened a new pull request, #57968:
URL: https://github.com/apache/spark/pull/57968

   ### What changes were proposed in this pull request?
   
   This PR converts the three `_LEGACY_ERROR_TEMP_*` conditions in 
`SparkCoreErrors` that reject an array-typed RDD key (`_3008`, `_3009` and 
`_3010`) into a new `UNSUPPORTED_ARRAY_KEY` umbrella with three sub-conditions, 
continuing the cleanup under 
[SPARK-37935](https://issues.apache.org/jira/browse/SPARK-37935).
   
   | Legacy | Builder | Now | SQLSTATE |
   |---|---|---|---|
   | `_LEGACY_ERROR_TEMP_3008` | `cannotUseMapSideCombiningWithArrayKeyError` | 
`UNSUPPORTED_ARRAY_KEY.MAP_SIDE_COMBINE` | 0A000 |
   | `_3009` | `hashPartitionerCannotPartitionArrayKeyError` | 
`UNSUPPORTED_ARRAY_KEY.HASH_PARTITIONER` | 0A000 |
   | `_3010` | `reduceByKeyLocallyNotSupportArrayKeysError` | 
`UNSUPPORTED_ARRAY_KEY.REDUCE_BY_KEY_LOCALLY` | 0A000 |
   
   The umbrella message is `Array keys are not supported by:` and each 
sub-condition completes the sentence with the mechanism that did the rejecting 
(`HashPartitioner.` / `map-side combining.` / `reduceByKeyLocally().`).
   
   The split is by mechanism rather than by user-facing API because an 
API-based split does not partition the throw sites. `HASH_PARTITIONER` covers 5 
of the 7 sites, spread over `partitionBy`, `combineByKeyWithClassTag` and three 
`cogroup` overloads, and those are reached from `partitionBy`, `groupByKey`, 
`join`, the three outer joins, `cogroup` and `groupWith`. In the other 
direction, `combineByKeyWithClassTag` alone can raise either `MAP_SIDE_COMBINE` 
or `HASH_PARTITIONER` depending on its `mapSideCombine` argument. Naming by 
mechanism also keeps each message truthful: an array key is fine with a 
`RangePartitioner`, and all five `HASH_PARTITIONER` guards test 
`isInstanceOf[HashPartitioner]` rather than rejecting partitioning outright.
   
   The builders keep their Scala names, which keeps the diff off 
`PairRDDFunctions`'s 7 throw sites.
   
   ### Reachability
   
   All three are reachable directly from the RDD API on the driver, so they get 
user-facing names rather than becoming internal errors. The guards run eagerly 
in the calling method inside `withScope`, before any RDD is constructed, so no 
action is needed to trigger them and no task is involved, which keeps 
`DAGScheduler.abortStage`'s internal-error handling out of the picture.
   
   `RDD.countByValueApprox`'s own array check (`_LEGACY_ERROR_TEMP_3015`, 
reached from `countByKeyApprox`) is deliberately left out even though it is the 
same family. It is a different check: it tests the *element* class of the 
mapped key RDD rather than a pair RDD's key class, and its message names 
`countByValueApprox()` rather than the API the user called, so folding it under 
`UNSUPPORTED_ARRAY_KEY` would overstate the umbrella's scope.
   
   ### Why are the changes needed?
   
   The error-conditions 
[README](https://github.com/apache/spark/blob/master/common/utils/src/main/resources/error/README.md)
 disallows new `_LEGACY_ERROR_TEMP_*` entries and asks existing ones to be 
resolved. This clears three of them.
   
   The three old messages were also inconsistent with each other for one 
concept: two ended with a period and one did not, and each named the mechanism 
in its own phrasing (`Cannot use map-side combining ...`, `HashPartitioner 
cannot partition ...`, `reduceByKeyLocally() does not support ...`). The 
umbrella gives them one sentence frame.
   
   ### Does this PR introduce _any_ user-facing change?
   
   Yes, to error messages, with no API change.
   
   Converting any legacy condition changes the rendered string in two 
mechanical ways: `SparkThrowableHelper.formatErrorMessage` suppresses the 
`[CONDITION] ` prefix only for `_LEGACY_ERROR_`-prefixed names, and appends ` 
SQLSTATE: xxxxx` when a sqlState exists (legacy entries have none, so these 
three gain both). Beyond that the message bodies change:
   
   - `_3008`: `Cannot use map-side combining with array keys.` -> 
`[UNSUPPORTED_ARRAY_KEY.MAP_SIDE_COMBINE] Array keys are not supported by: 
map-side combining. SQLSTATE: 0A000`
   - `_3009`: `HashPartitioner cannot partition array keys.` -> 
`[UNSUPPORTED_ARRAY_KEY.HASH_PARTITIONER] Array keys are not supported by: 
HashPartitioner. SQLSTATE: 0A000`
   - `_3010`: `reduceByKeyLocally() does not support array keys` -> 
`[UNSUPPORTED_ARRAY_KEY.REDUCE_BY_KEY_LOCALLY] Array keys are not supported by: 
reduceByKeyLocally(). SQLSTATE: 0A000`
   
   The thrown type stays `SparkException` for all three. Sibling 0A000 
conditions in `SparkCoreErrors` use both `SparkException` 
(`UNSUPPORTED_ADD_FILE.*`) and `SparkUnsupportedOperationException` 
(`UNSUPPORTED_CALL.TASK_NOT_FINISHED`), so there is no convention to follow 
here, and switching would move these out of `SparkException`'s hierarchy and 
stop existing `catch` blocks from catching them.
   
   ### How was this patch tested?
   
   `PartitioningSuite`'s `"partitioning Java arrays should fail"` already 
covered all three conditions, but its helper was 
`intercept[SparkException](testFun).getMessage.contains("array")`, whose 
Boolean result was discarded, so all twelve assertions only checked that some 
`SparkException` was thrown, not that it was about array keys. The helper now 
takes the expected sub-condition and calls `checkError` with it plus SQLSTATE, 
and 11 of the 12 assertions fail against the pre-change code (both the 
condition and the SQLSTATE differ). The twelfth is `countByKeyApprox`, which 
asserts `_LEGACY_ERROR_TEMP_3015` and is unaffected by this PR.
   
   Asserting a specific sub-condition per line pins which guard fires first, in 
particular that `combineByKeyWithClassTag` checks `mapSideCombine` before it 
looks at the partitioner. An inline comment records this, since reordering 
those two guards would change a user-visible condition name and should be 
deliberate.
   
   The SQLSTATE assertion was verified to be live by temporarily setting the 
JSON value to `42000` and watching the test fail with `sqlState: expected 
'0A000' but got '42000'` before restoring it. `checkError` skips the comparison 
when `sqlState` is `None`, and `SparkThrowableSuite` only checks that a state 
is registered, so a wrong SQLSTATE would otherwise ship green.
   
   Ran `core/testOnly org.apache.spark.SparkThrowableSuite 
org.apache.spark.PartitioningSuite` (50 tests, all passing).
   
   ### Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Code (Opus 4.8)
   


-- 
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]

Reply via email to