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