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/08/04 16:15:03 UTC
[PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
LuciferYang opened a new pull request, #57768:
URL: https://github.com/apache/spark/pull/57768
### What changes were proposed in this pull request?
Follow-up to #56430, addressing review comments from cloud-fan.
1. The JDK backend rewrote a reference to a class Java cannot name - an anonymous or
local class, or a class nested inside one - into its nearest nameable supertype
unconditionally. That is only sound when the supertype is itself referenceable and
offers every member the generated code could access, and nothing enforced either
condition. Such a unit is now routed to Janino instead, joining the two existing
deterministic-routing cases (REPL contexts and Scala package-object classes).
`canNarrowSafely` decides this. A member matches by its exact erased signature, with
an allowance for bridges: an override of a generic method erases narrower than the
supertype declaration it implements (`compare(String, String)` against
`Comparator.compare(Object, Object)`) and carries a bridge with the supertype's
signature, so narrowing keeps dispatching to the override. An overload has no bridge
and is rejected - it must be, because `Invoke` codegen wraps every call in an explicit
cast, which would hide the resulting type mismatch from javac and silently bind the
call to the supertype's method. The replacement type and all its enclosing classes
must also be public: same-package is not sufficient, because the generated class is
loaded by `InMemoryClassLoader` and its runtime package differs from the same-named
package on the application loader.
`nameableSupertype` now climbs while `getCanonicalName` is null rather than while the
class is anonymous or local, which additionally covers a named class nested inside
one of those - neither anonymous nor local itself, yet equally unnameable.
2. Reflection in this path can raise a `LinkageError` when a class loads but a type in
its signature does not (a partial or shaded jar). `NonFatal` does not cover that and
an escaping `Error` would bypass the codegen fallbacks, so `canNarrowSafely` and
`sourceNameOf` catch it and degrade to "cannot narrow" / the binary name.
3. Comment and documentation fixes: two wordings in the `CodeCompiler.active` scaladoc,
and the `spark.sql.codegen.compiler` config doc, which enumerated only two
always-Janino cases.
### Why are the changes needed?
Fixes the unchecked assumption. Without it, an anonymous type whose accessed member is
absent from the supertype either fails to compile or - for a same-arity overload -
compiles clean and returns the supertype's answer.
### Does this PR introduce _any_ user-facing change?
No. The default backend is Janino, and for `spark.sql.codegen.compiler=jdk` this only
moves units that javac would have mishandled onto the working path.
### How was this patch tested?
Eight cases in `CodeCompilerSuite` covering the `$`-digit gate, narrowable and
unnarrowable shapes, an unnameable supertype, a class nested in a local class, the
routing decision under both backend settings, an end-to-end compile asserting the JDK
backend rejects the unit that narrowing breaks, and its counterpart asserting a
bridge-preserved call stays on the JDK backend. Each production check was mutated in
turn and the corresponding test fails without it.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude 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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5189635355
Thank you @LuciferYang! Adding @cloud-fan for further review
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5241554916
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/b3c0402a9c8da97c655a52c791cc59d6f8ea9789
- merged into branch-4.x https://github.com/apache/spark/commit/261918370d3729322af0a755e1086ad57f0c067d
- merged into branch-4.3 https://github.com/apache/spark/commit/3e39b4ac372c4ee41f97f93419448aee51256497
*Posted by `merge_spark_pr.py`*
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5241573434
Thank you @cloud-fan @dongjoon-hyun @uros-b
I've merged this PR first. I'll address any new comments and make further fixes accordingly.
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5187925666
cc @cloud-fan
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5235559211
@cloud-fan Sorry, I forgot to ping you and @dongjoon-hyun yesterday. Could you take another look at it if you have time?
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5235538995
is it ready for review?
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #57768:
URL: https://github.com/apache/spark/pull/57768#discussion_r3719511546
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompiler.scala:
##########
@@ -812,6 +869,134 @@ object JdkCodeCompiler extends CodeCompiler with Logging {
if (c == null) classOf[Object] else c
}
+ /**
+ * True when `body` references a class that [[nameableSupertype]] cannot narrow soundly,
+ * i.e. the class carries a public member that the replacement type does not offer, or
+ * the replacement type is itself one javac cannot reference. Such a unit must go to
+ * Janino: rewriting the reference would either drop the member or emit a type name javac
+ * rejects.
+ *
+ * Called from [[CodeCompiler.active]] on every compile, so it is gated behind a scan for
+ * `$` followed by a digit. Every class the Java language forbids naming carries that in
+ * its binary name (`Outer$1`, `Outer$1Local`, Scala's `Outer$$anon$1`, and their nested
+ * members `Outer$1$Inner`), while the other `$` forms the rewrite handles - regular
+ * nesting (`Map$Entry`), Scala modules (`Foo$`, `Model$Load$Leaf`), package objects
+ * (`pkg$Inner`), specialized (`Function1$mcII$sp`) and operator-named (`$colon$colon`)
+ * classes - do not. Lambdas (`Outer$$Lambda$14/0x...`) do carry it and are neither
+ * anonymous nor local, but they are inert here: the tokenizer stops at `/`, leaving a
+ * name no loader can resolve. Janino cannot name them either, so a lambda reference
+ * never reaches generated source in the first place.
+ *
+ * The scan adds one linear pass over the body ahead of the compile-cache lookup, behind
+ * the intrinsified `$`-digit gate that ordinary generated code fails immediately.
+ *
+ * The scan reads the raw body, so a `$`-digit sequence inside a string literal or a
+ * comment can trigger the resolution attempt. That is harmless: an unloadable token is
+ * ignored, and a loadable one only ever picks Janino, which accepts a superset of what
+ * javac does.
+ */
+ private[codegen] def referencesUnnarrowableClass(body: String): Boolean = {
+ if (!containsDollarDigit(body)) return false
+ val classLoader = Utils.getContextOrSparkClassLoader
+ val checked = mutable.HashSet.empty[String]
+ var i = 0
+ val n = body.length
+ while (i < n) {
+ if (isNameStart(body.charAt(i))) {
+ val start = i
+ i += 1
+ while (i < n && isNamePart(body.charAt(i))) i += 1
+ val token = body.substring(start, i)
+ if (containsDollarDigit(token) && checked.add(token) &&
+ loadLongestPrefix(token, classLoader).exists {
+ case (cls, _) => !canNarrowSafely(cls)
+ }) {
+ return true
+ }
+ } else {
+ i += 1
+ }
+ }
+ false
+ }
+
+ /** True iff `s` holds a `$` immediately followed by an ASCII digit. */
+ private[codegen] def containsDollarDigit(s: String): Boolean = {
+ var i = s.indexOf('$')
+ while (i >= 0 && i < s.length - 1) {
+ val next = s.charAt(i + 1)
+ if (next >= '0' && next <= '9') return true
+ i = s.indexOf('$', i + 1)
+ }
+ false
+ }
+
+ /**
+ * True when a reference to `cls` can be replaced by [[nameableSupertype]] without losing
+ * access to any member. A class that is already nameable needs no narrowing and always
+ * qualifies.
+ *
+ * Otherwise two things must hold. First, the replacement type must be one the generated
+ * unit can reference: it and every enclosing class must be public. Same-package is NOT
+ * sufficient even though javac would accept it - the generated class is defined into
+ * `org.apache.spark.sql.catalyst.expressions` but loaded by [[InMemoryClassLoader]], so
+ * its runtime package differs from the same-named package on the app loader and a
+ * package-private access would fail with `IllegalAccessError` at execution time instead
+ * of at compile time. Second, every public member of the concrete class - including
+ * inherited ones, since the generated code may access any of them - must be reachable on
+ * the replacement type.
+ *
+ * A member is matched by its exact erased signature, with one allowance for bridges: an
+ * override of a generic method has a narrower erasure than the supertype declaration it
+ * implements (`compare(String, String)` against `Comparator.compare(Object, Object)`),
+ * and the compiler emits a bridge carrying the supertype's signature. Such a method is
+ * safe to narrow because `invokevirtual` on the supertype signature still dispatches to
+ * the override. An overload has no bridge, so it is rejected - and it must be, because
+ * narrowing binds the call to the supertype's method instead: `Invoke` codegen always
+ * wraps the call in an explicit cast, which would hide the type mismatch from javac and
+ * silently produce the wrong result rather than fail to compile.
+ *
+ * Reflection over the concrete class can raise a `LinkageError` when a member signature
+ * names a class the loader cannot find (a partial or shaded jar). `NonFatal` does not
+ * cover that, and an escaping `Error` would bypass the codegen fallbacks, so it is
+ * caught here and reported as "cannot narrow" - Janino compiles what javac cannot.
+ */
+ private def canNarrowSafely(cls: Class[_]): Boolean = {
+ val target = nameableSupertype(cls)
+ if (cls eq target) return true
+ if (!isPubliclyNameable(target)) return false
+ try {
+ val reachable: Seq[Class[_]] = Seq(target, classOf[Object])
+ val targetSignatures = reachable.flatMap(_.getMethods).map(erasedSignature).toSet
+ val targetFields = reachable.flatMap(_.getFields).map(_.getName).toSet
+ val methods = cls.getMethods
+ val bridgedTo = methods.iterator
+ .filter(m => m.isBridge && targetSignatures.contains(erasedSignature(m)))
+ .map(m => (m.getName, m.getParameterCount))
+ .toSet
+ methods.forall { m =>
+ targetSignatures.contains(erasedSignature(m)) ||
+ bridgedTo.contains((m.getName, m.getParameterCount))
Review Comment:
This key is too broad: one legitimate bridge makes every overload with the same name and arity pass this check. For example, an anonymous `Comparator[String]` that also defines `compare(Int, Int)` stays on JDK, but after narrowing an integer call binds to `Comparator.compare(Object, Object)` and reaches the String-casting bridge instead of the overload. Please match each concrete method to its bridge using erased parameter types, and add a fixture combining a bridge with an unrelated same-arity overload.
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5199219898
The `LinkageError` hardening looks incomplete to me. The follow-up note above says both call sites catch it now, but the catches guard only the calls made directly at those sites — the reflective calls reached *through* `nameableSupertype` are still outside both of them:
- In `canNarrowSafely`, `val target = nameableSupertype(cls)` and `isPubliclyNameable(target)` run **before** the `try`. `nameableSupertype`'s loop condition calls `c.getCanonicalName`, and `isPubliclyNameable` walks `getEnclosingClass` — both can throw `NoClassDefFoundError` when an enclosing class is not loadable (the same partial/shaded-jar scenario this PR defends against). An `Error` escaping here propagates through `referencesUnnarrowableClass` -> `active()` -> `CodeGenerator.compile` and bypasses the `NonFatal`-based codegen fallbacks.
- In `rewriteQualifiedName`, `sourceNameOf(nameableSupertype(cls))` leaves the `nameableSupertype` call unprotected; `sourceNameOf` guards only its own `cls.getCanonicalName`. Before this refactor the whole sequence (`Class.forName` + `nameableSupertype` + `getCanonicalName`) sat inside `resolveSourceName`'s catch, so this path has narrower protection than it had on master.
Suggestion: move the first three lines of `canNarrowSafely` into its `try`, and wrap the narrow-and-name step in `rewriteQualifiedName` with the same catch semantics (degrade to the binary name / "cannot narrow").
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #57768: [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound
URL: https://github.com/apache/spark/pull/57768
--
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
Re: [PR] [SPARK-57370][SQL][FOLLOWUP] Route codegen referencing an unnameable class to Janino when narrowing is unsound [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57768:
URL: https://github.com/apache/spark/pull/57768#issuecomment-5189915613
Thanks for the review @cloud-fan. Mapping your three comments to what changed:
**`CodeCompiler.scala:146`** — fixed, now `merely slower at compiling`.
**`CodeCompiler.scala:149`** — fixed, reworded to `all involving classes the JDK compiler cannot name`. The count also went from two to three, since the routing gained an arm.
**`CodeCompiler.scala:780`** — took the second option you offered (route to Janino). The first one is not reachable from where the name is produced: the unnameable name arrives through `addReferenceObj`'s `className` parameter from `Literal.doGenCode`, and its source is `ObjectType`. `Literal.fromObject(obj)` has no declared type in its signature to preserve, so threading one down would mean changing `DeserializerBuildHelper` / `SerializerBuildHelper` and the encoder path rather than a local fix. So `active` gained a third deterministic-routing arm and `canNarrowSafely` decides it.
While implementing it I found the check has to be stricter than "the accessed members are available" in three ways. Flagging them separately since they go past what you asked for.
**1. Member matching is exact-signature plus a bridge allowance, not name+arity.** Name+arity conflates overriding with overloading. An override of a generic method carries a bridge with the supertype's erased signature, so narrowing still dispatches to the override; an overload has no bridge and javac binds statically to the supertype's method. That does not surface as a compile error, because `Invoke.doGenCode` always wraps the call in an explicit cast. On `new CBase() { public String conv(String) }` over `CBase.conv(Object)`, javac compiles with zero diagnostics and the result changes from `ANON-x` to `BASE-x`: a silent wrong answer rather than a failure. The tightening is free — over the 980 anonymous/local classes in scala-library, scala-reflect and scala-compiler, the two rules have identical trigger sets (52 / 48 / 147).
**2. The replacement type must itself be referenceable.** Members lining up is not enough if javac cannot spell the type. In scala-library, 36 of 299 anonymous/local classes narrow to a non-public target, e.g. `mutable.LinkedHashSet$$anon$1` to the `private abstract` `LinkedHashSet$LinkedHashSetIterator`; scala-reflect is 21 of 154. The check now requires the target and all its enclosing classes to be public. Same-package cannot be exempted: with a package-private target in `org.apache.spark.sql.catalyst.expressions` javac accepts the cast, but it fails at runtime with `IllegalAccessError`, because the generated class is loaded by `InMemoryClassLoader` and its runtime package differs from the same-named package on the application loader.
**3. A named class nested inside an anonymous or local class is neither.** `Outer$1$Inner` reports `isAnonymousClass == false`, `isLocalClass == false`, `isMemberClass == true` and `getCanonicalName == null`, so the old climb returned it unchanged and emitted its binary name, which javac rejects. `nameableSupertype` now climbs while `getCanonicalName` is null. That predicate is a superset at no observed cost: all 299 null-canonical classes in scala-library are exactly the anonymous/local ones, and the single extra one in scala-reflect (`Positions$worker$1$solidChildrenCollector$`) is a member of a local class, so equally unnameable. Object-nested case classes, operator-named classes, `Map$Entry` and specialized names all have non-null canonical names, so the existing rewrite tests are unaffected.
Two smaller items also outside your comments. The refactor had moved `nameableSupertype` and `getCanonicalName` out of the `try` that used to catch `LinkageError`, so a `NoClassDefFoundError` from a partial or shaded jar could escape past the codegen fallbacks — `NonFatal` does not cover it, and `WholeStageCodegenEvaluatorFactory.eval` has no try at all. Both call sites catch it now. And `spark.sql.codegen.compiler`'s doc still enumerated two always-Janino cases; it lists three.
--
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