LuciferYang opened a new pull request, #57707:
URL: https://github.com/apache/spark/pull/57707
### What changes were proposed in this pull request?
Two tests around codegen compile failures assert on the compiler's own
diagnostic wording instead of what Spark itself produces. This PR points each
assertion at the part Spark owns.
`CodeGeneratorWithInterpretedFallbackSuite`, test "codegen failures in the
CODEGEN_ONLY mode":
```scala
assert(errMsg.contains("Failed to compile:
org.codehaus.commons.compiler.CompileException:"))
```
The class name in that string is Janino's. What Spark owns on this path is
the wrapping - a source-level failure goes through
`QueryExecutionErrors.compilerError`, whose checked `CompileException` the
compile cache wraps in an `ExecutionException`, which is exactly the contract
SPARK-23711 / SPARK-25140 established - and the `"Failed to compile: "` prefix
that `failedToCompileMsg` prepends. The test now asserts the cause's type and
that prefix. Asserting the cause type states the contract directly; the class
name inside a message stated it by accident, and
`intercept[ExecutionException]` alone says nothing about the cause.
`ObjectExpressionsSuite`, test "SPARK-23593: InitializeJavaBean should
support interpreted execution":
```scala
checkExceptionInExpression[Exception](initializeWithNonexistingMethod,
"""A method named "nonexistent" is not declared in any enclosing class """
+
"nor any supertype")
```
`checkExceptionInExpression` applies one substring to both the interpreted
and the codegen path, so this string has to match both. It does, but only
because the two happen to overlap. Interpreted execution raises Spark's own
`INTERNAL_ERROR` from `QueryExecutionErrors.methodNotDeclaredError`:
```
[INTERNAL_ERROR] A method named "nonexistent" is not declared in any
enclosing class nor any supertype SQLSTATE: XX000
```
while the codegen path fails in the Java compiler:
```
org.codehaus.commons.compiler.CompileException: File 'generated.java', Line
41, Column 12: Failed to compile:
org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 41,
Column 12: A method named "nonexistent" is not declared in any enclosing class
nor any supertype, nor through a static import
```
Janino's diagnostic is a superstring of Spark's sentence, so one assertion
covers both legs. The test now asserts each path against what raises it: the
interpreted leg through `checkError`, which pins the error class, parameters
and sqlState exactly - stronger than the substring it replaces - and the
codegen leg on the `ExecutionException` wrapping plus the `"Failed to compile:
"` prefix.
The same test also had an assertion that never ran, because the `contains`
result was discarded:
```scala
intercept[Exception] {
evaluateWithoutCodegen(initializeWithWrongParamType,
InternalRow.fromSeq(Seq()))
}.getMessage.contains(
"""A method named "setX" is not declared in any enclosing class """ +
"nor any supertype")
```
It has been a bare expression since SPARK-23593 introduced it in 2018, so
only "an exception was thrown" was ever verified. That leg raises the same
`methodNotDeclaredError`, so it now uses `checkError` as well rather than two
assertion styles for one error class in one test.
### Why are the changes needed?
An assertion on a compiler's diagnostic wording tests something Spark does
not control, and it obscures what the test is actually for. In
`CodeGeneratorWithInterpretedFallbackSuite` the contract under test is the
exception type, not the message. In `ObjectExpressionsSuite` the interpreted
path has an error class, parameters and a sqlState available, so a substring
match leaves assertable contract unasserted. And the `setX` assertion has
verified nothing for eight years.
Not addressed here, found while auditing for similar sites:
`DataFrameSuite`'s ignored test "SPARK-19372: Filter can be executed w/o
generated code due to JVM code size limit" asserts `e.contains("grows beyond 64
KiB")`, but Janino's string is `Code grows beyond 64 KB` - no released version
spells it `KiB`. That assertion is already stale and only survives because the
test is `ignore`d.
A side benefit: with the compiler's wording no longer asserted, these tests
no longer need updating if the generated code is ever compiled by something
other than Janino.
### Does this PR introduce _any_ user-facing change?
No. Test-only.
### How was this patch tested?
`CodeGeneratorWithInterpretedFallbackSuite`, `ObjectExpressionsSuite` and
`ExpressionEvalHelperSuite` (37 tests) pass, and `dev/scalastyle` is clean.
Each changed assertion was mutation-tested to confirm it is live: breaking
the expected message on the interpreted leg reports `checkError found 1
mismatch(es)`; breaking it on the `setX` leg does the same; changing the
codegen leg's `intercept` type to `TimeoutException` reports `Expected
exception java.util.concurrent.TimeoutException to be thrown, but
java.util.concurrent.ExecutionException was thrown`.
I also verified the claim that one substring covered both legs by accident:
the two real messages are quoted above, taken from a probe that printed them
under `CODEGEN_ONLY` and `NO_CODEGEN`. And to check that asserting the method
name alone would be too weak, I confirmed three unrelated failures still echo
it - wrapping the expression in an `Unevaluable`, switching the interpreted
path to `methodNotFoundError`, and falling back to a bare reflection exception
- because Spark echoes whole expressions into those messages and the name is
also the input.
### 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]
