You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2026/07/31 02:57:54 UTC

[PR] [SPARK-58440][SQL] Convert Sequence codegen error parameters to String [spark]

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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org