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: [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]
