[GitHub] [spark] rednaxelafx commented on a change in pull request #31733: [SPARK-34607][SQL] Add `Utils.isMemberClass` to fix a malformed class name error on jdk8u

[GitBox <gi...@apache.org>]

rednaxelafx commented on a change in pull request #31733:
URL: https://github.com/apache/spark/pull/31733#discussion_r587211720



##########
File path: core/src/main/scala/org/apache/spark/util/Utils.scala
##########
@@ -2858,6 +2858,31 @@ private[spark] object Utils extends Logging {
     Hex.encodeHexString(secretBytes)
   }
 
+  /**
+   * Returns true if and only if the underlying class is a member class.
+   *
+   * Note: jdk8u throws a "Malformed class name" error if a given class is a deeply-nested
+   * inner class (See SPARK-34607 for details). This issue has already been fixed in jdk9+, so
+   * we can remove this helper method safely if we drop the support of jdk8u.
+   */
+  def isMemberClass(cls: Class[_]): Boolean = {
+    try {
+      cls.isMemberClass
+    } catch {
+      case _: InternalError =>
+        // We emulate jdk8u `Class.isMemberClass` below:
+        //   public boolean isMemberClass() {
+        //     return getSimpleBinaryName() != null && !isLocalOrAnonymousClass();
+        //   }
+        // `getSimpleBinaryName()` returns null if a given class is a top-level class,
+        // so we replace it with `cls.getEnclosingClass != null`. The second condition checks
+        // if a given class is not a local or an anonymous class, so we replace it with
+        // `cls.getEnclosingMethod == null` because `cls.getEnclosingMethod()` return a value
+        // only in either case (JVM Spec 4.8.6).
+        cls.getEnclosingClass != null && cls.getEnclosingMethod == null

Review comment:
       Cool! I think the emulation logic here is correct, but we might be able to do a small tweak to it: let's swap the order of the two checks -- we don't have to emulate the JDK8u version, instead we just need to mimic "the best" implementation since the semantics haven't changed.
   
   Here's the JDK method in the latest OpenJDK development branch:
   https://github.com/openjdk/jdk/blob/84c93d5a18021c178d2f28b22869e910c391ead2/src/java.base/share/classes/java/lang/Class.java#L1746
   ```java
       /**
        * Returns {@code true} if and only if the underlying class
        * is a member class.
        *
        * @return {@code true} if and only if this class is a member class.
        * @since 1.5
        */
       public boolean isMemberClass() {
           return !isLocalOrAnonymousClass() && getDeclaringClass0() != null;
       }
   ```
   As you can see, the local-or-anonymous check goes first, then the enclosing class check.
   
   That's because `getEnclosingClass` itself needs to do a `getEnclosingMethod` check, so if we do the enclosing class check first we'll be doing the enclosing method check twice.




----------------------------------------------------------------
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.

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