LuciferYang opened a new pull request, #57664:
URL: https://github.com/apache/spark/pull/57664
### What changes were proposed in this pull request?
`Sequence.genSequenceLengthCode` generates Java source that builds the
message
parameters for `_LEGACY_ERROR_TEMP_3243`:
```java
java.util.Map<String, String> params = new java.util.HashMap<String,
String>();
params.put("start", $start);
params.put("stop", $stop);
params.put("step", $step);
throw new
org.apache.spark.SparkIllegalArgumentException("_LEGACY_ERROR_TEMP_3243",
params);
```
`$start` and `$stop` are numeric expressions, and `$step` is numeric or a
`CalendarInterval` reference depending on the sequence implementation.
Janino,
which compiles the generated code, erases the type arguments and binds these
calls to `put(Object, Object)`, so the raw values go in unconverted. This PR
converts them with `String.valueOf(...)`.
### Why are the changes needed?
The parameter map reaches `SparkIllegalArgumentException` and is returned by
`SparkThrowable.getMessageParameters()`, whose declared type is
`java.util.Map<String, String>`. Callers that read a value as a `String` -
the
declared type - fail with a `ClassCastException`. Confirmed consumers on this
path:
- `SparkThrowableHelper.scala:165` does `value.replaceAll("#\\d+", "#x")`,
used
when `spark.sql.error.messageFormat` is `MINIMAL`/`STANDARD` (spark-sql
CLI and
Thrift Server via `HiveThriftServerErrors.scala:43-44`).
- Spark Connect's `ErrorUtils.scala:241` renders the map through json4s
`map2jvalue`, which inserts a `checkcast String` per value, and
`ErrorUtils.scala:167-169` puts it into a proto `map<string, string>`.
- `CheckErrorHelper.checkError` itself: while building its diagnostic dump it
iterates the actual parameters as `String`, so it throws
`ClassCastException: class java.lang.Integer cannot be cast to class
java.lang.String` at `CheckErrorHelper.scala:186`.
The rendered message text was never affected, because parameter substitution
calls `toString`. That is why this went unnoticed: only the structured
parameter
map was wrong, and this error path had no test coverage.
The map-based parameters were introduced by SPARK-46991 (`f5b0de07eff`,
replacing
`IllegalArgumentException` with `SparkIllegalArgumentException` in
catalyst), so
this is present from 4.0.0 onward. Verified on `master`, `branch-4.x`,
`branch-4.2`, `branch-4.1` and `branch-4.0`. `branch-3.5` is not affected -
it
still builds the message by string concatenation.
Out of scope, noted for the record: for the same condition the interpreted
path
(`Sequence.getSequenceLength`) throws a plain `IllegalArgumentException` via
`require` with no error class, so codegen and interpreted execution are not
symmetric here. Reconciling that would change a user-visible exception type
and
belongs in its own change.
### Does this PR introduce _any_ user-facing change?
Yes, a bug fix. `getMessageParameters()` on this error now returns `String`
values as its signature declares, instead of `Byte`/`Short`/`Integer`/`Long`/
`CalendarInterval`. The rendered message text is unchanged.
### How was this patch tested?
New test in `CollectionExpressionsSuite`, asserting the parameter map rather
than
the message text, covering both code paths: the integral implementation
(`$step`
is a Java primitive) and `TemporalSequenceImpl` (`$step` is a
`CalendarInterval`).
Confirmed it fails without the fix and passes with it. The test is
codegen-only
because the interpreted path has no error class, which is also why
`checkErrorInExpression` cannot be used - it additionally runs NO_CODEGEN.
Beyond the committed test I probed the full static-type matrix under Janino -
`byte`/`short`/`int`/`long` for the integral path, plus `CalendarInterval`,
`Period`, `Duration` and `TimestampNTZ` steps - and confirmed that all nine
cases
hold non-`String` values before the fix, `java.lang.String` after, and that
the
rendered message is byte-for-byte identical in every case. `byte` and
`short` have
no dedicated `String.valueOf` overload; Janino widens them to
`String.valueOf(int)` the same way javac does.
`CollectionExpressionsSuite` (62 tests) and `dev/scalastyle` pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code Opus 5
--
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]
