LuciferYang opened a new pull request, #57678:
URL: https://github.com/apache/spark/pull/57678
### 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]
