[SPARK-34596][SQL] Use Utils.getSimpleName to avoid hitting Malformed class name in NewInstance.doGenCode

[rednaxelafx]

What changes were proposed in this pull request?

Use Utils.getSimpleName to avoid hitting Malformed class name error in NewInstance.doGenCode.

Why are the changes needed?

On older JDK versions (e.g. JDK8u), nested Scala classes may trigger java.lang.Class.getSimpleName to throw an java.lang.InternalError: Malformed class name error.
In this particular case, creating an ExpressionEncoder on such a nested Scala class would create a NewInstance expression under the hood, which will trigger the problem during codegen.

Similar to #29050, we should use Spark's Utils.getSimpleName utility function in place of Class.getSimpleName to avoid hitting the issue.

There are two other occurrences of java.lang.Class.getSimpleName in the same file, but they're safe because they're only guaranteed to be only used on Java classes, which don't have this problem, e.g.:

    // Make a copy of the data if it's unsafe-backed
    def makeCopyIfInstanceOf(clazz: Class[_ <: Any], value: String) =
      s"$value instanceof ${clazz.getSimpleName}? ${value}.copy() : $value"
    val genFunctionValue: String = lambdaFunction.dataType match {
      case StructType(_) => makeCopyIfInstanceOf(classOf[UnsafeRow], genFunction.value)
      case ArrayType(_, _) => makeCopyIfInstanceOf(classOf[UnsafeArrayData], genFunction.value)
      case MapType(_, _, _) => makeCopyIfInstanceOf(classOf[UnsafeMapData], genFunction.value)
      case _ => genFunction.value
    }

The Unsafe-* family of types are all Java types, so they're okay.

Does this PR introduce any user-facing change?

Fixes a bug that throws an error when using ExpressionEncoder on some nested Scala types, otherwise no changes.

How was this patch tested?

Added a test case to org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite. It'll fail on JDK8u before the fix, and pass after the fix.

[maropu]

I've checked that the added test fails on jdk8. LGTM if the running tests pass.

[dongjoon-hyun]

+1, LGTM (if CI passes).

[cloud-fan]

retest this please

[SparkQA]

Test build #135671 has started for PR 31709 at commit d4e92aa.

[rednaxelafx]

Looks like the AMPLab Jenkins is unstable, that there are test failures due to java.io.IOException: Backing channel 'amp-jenkins-staging-worker-02' is disconnected.

[dongjoon-hyun]

Ya, it looks like that, @rednaxelafx .

[dongjoon-hyun]

Retest this please.

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40257/

[SparkQA]

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40257/

[SparkQA]

Test build #135675 has finished for PR 31709 at commit d4e92aa.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[HyukjinKwon]

Merged to master, branch-3.1 and branch-3.0. @rednaxelafx should we backport in bracnh-2.4 too?

[rednaxelafx]

Merged to master, branch-3.1 and branch-3.0. @rednaxelafx should we backport in bracnh-2.4 too?

Since Spark 2.4 line is already EOL, I think we're safe to skip backporting to 2.4?

[HyukjinKwon]

I think it's not explicitly announced as an EOL though it will likely be EOL according to https://spark.apache.org/versioning-policy.html. Sure, so I think it's not to backport for now.

[dongjoon-hyun]

Well, we had better try to EOL. :)

I think it's not explicitly announced as an EOL

Could you backport it to branch-2.4, @HyukjinKwon ? Recently, ZSTD library is also upgraded still.

[dongjoon-hyun]

@rednaxelafx . Or, your patch is not applicable to branch-2.4?

[rednaxelafx]

@dongjoon-hyun this bug does affect Spark 2.4 branch as well (in fact to previous version too). The same fix could be applied to 2.4.

[dongjoon-hyun]

Thank you for confirmation. I'll try to cherry-pick and test then.

[dongjoon-hyun]

The fix seems to be applicable, but the test seems to fail unfortunately.

$ git diff HEAD~1
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
index 3189e6841a..ba9e9ecebb 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
@@ -487,7 +487,7 @@ case class NewInstance(
     ev.isNull = resultIsNull

     val constructorCall = outer.map { gen =>
-      s"${gen.value}.new ${cls.getSimpleName}($argString)"
+      s"${gen.value}.new ${Utils.getSimpleName(cls)}($argString)"
     }.getOrElse {
       s"new $className($argString)"
     }

[info] - encode/decode for nested Scala class should work: MalformedNameExample(42) (codegen path) *** FAILED *** (34 milliseconds)
[info]   Exception thrown while decoding
[info]   Converted: [0,2a]
[info]   Schema: x#836
[info]   root
[info]   -- x: integer (nullable = false)
[info]
[info]
[info]   Encoder:
[info]   class[x[0]: int] (ExpressionEncoderSuite.scala:399)
[info]   org.scalatest.exceptions.TestFailedException:
[info]   at org.scalatest.Assertions$class.newAssertionFailedException(Assertions.scala:528)
[info]   at org.scalatest.FunSuite.newAssertionFailedException(FunSuite.scala:1560)
[info]   at org.scalatest.Assertions$class.fail(Assertions.scala:1105)
[info]   at org.scalatest.FunSuite.fail(FunSuite.scala:1560)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite$$anonfun$encodeDecodeTest$1.apply(ExpressionEncoderSuite.scala:399)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite.org$apache$spark$sql$catalyst$encoders$ExpressionEncoderSuite$$verifyNotLeakingReflectionObjects(ExpressionEncoderSuite.scala:477)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite$$anonfun$testAndVerifyNotLeakingReflectionObjects$1.apply(ExpressionEncoderSuite.scala:484)

...

[info]   Cause: java.lang.RuntimeException: Error while decoding: java.util.concurrent.ExecutionException: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 32, Column 124: failed to compile: org.codehaus.commons.compiler.CompileException: File 'generated.java', Line 32, Column 124: "org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite$MalformedClassObject$" has no member type "ExpressionEncoderSuite$MalformedClassObject$MalformedNameExample"
[info] newInstance(class org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite$MalformedClassObject$MalformedNameExample)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoder.fromRow(ExpressionEncoder.scala:305)
[info]   at org.apache.spark.sql.catalyst.encoders.ExpressionEncoderSuite$$anonfun$encodeDecodeTest$1.apply(ExpressionEncoderSuite.scala:397)

[dongjoon-hyun]

Could you please make a backporting PR to make it sure to pass CIs since Apache Spark 2.4 is not EOL officially and this patch is important?

[HyukjinKwon]

Yeah, let's backport. Looks we'll have another maintenance release that's likely EOL.

[rednaxelafx]

The test failure on Spark 2.4 may be due to a difference in either Scala version or Janino version in use vs Spark 3.0+.

[dongjoon-hyun]

Ur, @rednaxelafx and @HyukjinKwon . The test fails even in branch-3.1.

• https://github.com/apache/spark/runs/2026199208

[rednaxelafx]

So I did some investigation and found that:

• For the test case I gave in this PR (emulating a kind of Scala class nesting scenario), before this PR it'll fail to codegen because of the "Malformed class name" issue. So that's something that never worked before. In practice, it isn't that hard to hit it, if you run Spark code through a Scala REPL, because the Scala REPL would use either nested objects or nested classes to implement the notion of multiple commands. (Well, there are some REPL-specific code paths in Spark that avoids hitting this problem in some cases though...e.g. this one )
• After applying this PR, the use of Utils.getSimpleName actually returns a wrong name for generated-Java-code's compilation purposes. We do need a real "simple name", but when the Scala nested class is not generated by the Scala REPL, the one returned by Utils.getSimpleName only strips off the package name and maybe the trailing dollar-sign, but that's it. So in the case of the test case in this PR, we'd want the simple name to be MalformedNameExample, but we'd actually get back ExpressionEncoderSuite$MalformedClassObject$MalformedNameExample instead. That'll cause Janino compilation to fail.
• Actually, what's more interesting is the different rules of Java's nested classes and Scala's. Since there is no such notion of a "companion object" or built-in "singleton object" notion on the Java language level, at least not in Java 8, the Java language syntax for creating a new instance of a inner class object doesn't work well for Scala's class Foo { object Bar { class Baz {...} } } kind of nesting, i.e. even when we do use the correct "simple name" (e.g. MalformedNameExample), there would still be no valid Java syntax to create it (i.e. outerObj.new InnerClass(...) doesn't work when outerObj is a Scala object)
• Then comes what's interesting. In Spark's tests, some environment setup will cause the codegen fallback to be in no-fallback mode, meaning a failed compilation will cause test failure; in some other setup, the fallback to interpreted is allowed, so compilation failure does not cause test failure. The reason why my test case in this PR sometimes pass and sometimes fail is exactly that: the compilation actually always fail due to incorrect generated code, but in some test setup it's allowed to fallback so it doesn't fail the test, but in some other setup it's banned from falling back, thus failing the test.

At this point I think it's a bit moot to make the generated code actually work for the class Foo { object Bar { class Baz {...} } } pattern mentioned here, as there's no Java syntax available for it anyway. Maybe there are workarounds, but we'll have to investigate a bit more to find them.

The original intent of this PR was to make sure when using Dataset on this kind of Scala class nesting it can still run without user-visible failures. I think that intent is fulfilled with the fix in this PR when running in non-testing mode -- the codegen fallback to interpreter would step in and make it work, instead of getting a JDK java.lang.InternalError that Spark's codegen doesn't handle.

cc @retronym I wonder if we can get some help from some wonderful Scala folks ^_^ Thanks!

cc @maropu @viirya @kiszk @cloud-fan do @dongjoon-hyun @HyukjinKwon you have any idea what the environment setup differences are with running build/sbt catalyst/test vs build/sbt "catalyst/testOnly *ExpressionEncoderSuite"? On branch-3.1, the former shows the compilation error as test failure, whereas the latter doesn't show it but under the hood the same compilation error actually happened as well...

---

Some more background on Java's "object creation expression", aka the new expression.

Roughly speaking, there are two variants of the new expression in Java,

1. Unqualified: that's the regular new ClassName(args) form. The way to create non-nested object instances in the normal case, but can also create nested object instances if the enclosing object is this.
2. Qualified: the way to create nested objects. The syntax looks like outerObjRef.new InnerClassSimpleName(args). The outerObjRef here would be an expression that refers to the enclosing object of the inner object, and the InnerClassSimpleName would be a simple class name of the inner class. Note that both javac and Janino demands that this InnerClassSimpleName cannot have dots (.) in it, so you can't use the fully qualified class name in this place. The compiler will check if the InnerClassSimpleName is indeed a member class of the static type of the outerObjRef.

In Scala, when you declare:

object Foo {
  class Bar { }
}

then Foo's class name is Foo$, but Bar's class name isn't in the Java convention of outerClassName + '$' + innerClassSimpleName (which would have been Foo$$Bar), but instead it's just Foo$Bar.
There's quite a bit of information recorded in the Class file that at runtime, reflection is still able to recognize the enclosing/member relationships, e.g.

val fooDollarClazz = Foo.getClass
fooDollarClazz.getDeclaredClasses //=> Array[Class[_]] = Array(class Foo$Bar)

val barClazz = classOf[Foo.Bar]
barClazz.getEnclosingClass //=> Class[_] = class Foo$
barClazz.getDeclaringClass //=> Class[_] = class Foo$

When Janino tries to find a member type, unfortunately, it doesn't use runtime reflection to figure out the member type, but instead it just relies on the Java naming convention of outerClassName + '$' + innerClassSimpleName,
https://github.com/janino-compiler/janino/blob/8142ec43a3be288e214de30c9f57fe50e9047766/janino/src/main/java/org/codehaus/janino/IClass.java#L803-L876 , like this:

            String memberDescriptor = Descriptor.fromClassName(
                Descriptor.toClassName(this.getDescriptor())
                + '$'
                + name
            );

which will fail to find the Bar type in a fooObjRef.new Bar() expression.

Same deal with javac. Here's an example:
foo.scala

object Foo {
  class Bar {
  }
}

TestScalaNestedType.java

import java.lang.reflect.*;

public class TestScalaNestedType {
  public static void main(String[] args) throws Exception {
    Class<?> fooDollarClazz = Foo$.class; // Class.forName("Foo$");
    Field fooModuleField = fooDollarClazz.getDeclaredField("MODULE$");
    Foo$ fooObjRef = (Foo$) fooModuleField.get(null);
    // compiles fine up to the above

    fooObjRef.new Bar(); // fails compilation: error: cannot find symbol Bar in Foo$
  }
}

command line:

$ javac -g TestScalaNestedType.java 
TestScalaNestedType.java:10: error: cannot find symbol
    fooObjRef.new Bar();
                  ^
  symbol:   class Bar
  location: class Foo$
1 error

If we change the new expression to: fooObjRef.new Foo.Bar(), then the error becomes:

$ javac -g TestScalaNestedType.java 
TestScalaNestedType.java:10: error: '(' expected
    fooObjRef.new Foo.Bar();
                     ^
1 error

obviously the dot (.) isn't accepted there.

[cloud-fan]

Maybe we set CODEGEN_FACTORY_MODE somewhere without setting it back

[rednaxelafx]

Maybe we set CODEGEN_FACTORY_MODE somewhere without setting it back

That is my guess as well, thus my earlier guess of the test being somewhat context/order sensitive

[dongjoon-hyun]

Ya. I agree with you the following.

I think that intent is fulfilled with the fix in this PR when running in non-testing mode -- the codegen fallback to interpreter would step in and make it work, instead of getting a JDK java.lang.InternalError that Spark's codegen doesn't handle.

And, in that case, we should wrap the unit test case with CODEGEN_FACTORY_MODE setting explicitly because it works only in the fallback status. So, are you going to proceed in revise the test case in that way, @rednaxelafx ?

[viirya]

👌 I reverted it from branch-3.1.

Do we? I still see the commit in branch-3.1.

[dongjoon-hyun]

@viirya . It was merged and reverted once . And, when @maropu 's patch arrives, I merged back with @maropu 's patch together. The AS-IS status is broken due to CODEGEN_FACTORY_MODE issue.

[viirya]

@dongjoon-hyun I see. Thanks for clarifying the situation.

[dongjoon-hyun]

Sorry for the inconvenience. I'm looking at this.

[dongjoon-hyun]

Hi, All.
I created a WIP PR to put the newly added test cases into FALLBACK mode always.

• [SPARK-34596][SPARK-34607][SQL][FOLLOWUP][TESTS][3.1] Add PlanTest. testFallback and use it #31764

According to @rednaxelafx 's comment, it should pass conceptually. However, for now, it is failing still. So, I made a WIP to share it first and get your opinions.

[kiszk]

I would like to share the finding that the following commands can reproduce the error. I hope that it can help you.

% build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn  *ExpressionEncoderSuite"

or

% build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOff  *ExpressionEncoderSuite"

[kiszk]

I think that this is the almost minimum set, which can be easily specified, to reproduce the problem.

ANSI mode: cast string to timestamp with parse error and encode/decode for nested Scala class should work may have implicit dependency. I think that SPARK-27671: cast from nested null type in struct is not related to this reproduction.

% build/sbt "catalyst/testOnly  *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z nested -z parse "
...
[info] AnsiCastSuiteWithAnsiModeOn:
[info] - SPARK-27671: cast from nested null type in struct (1 second, 407 milliseconds)
[info] - ANSI mode: cast string to timestamp with parse error (1 second, 309 milliseconds)
[info] ExpressionEncoderSuite:
[info] - encode/decode for nested Scala class should work: MalformedNameExample(42) (codegen path) *** FAILED *** (203 milliseconds)
[info]   Exception thrown while decoding
[info]   Converted: [0,2a]
[info]   Schema: x#18318
[info]   root
[info]   -- x: integer (nullable = false)
[info]   
...

[kiszk]

In the above command, when the following line is replaced with {, we cannot see any failure

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

Line 972 in e3ee8ea

SQLConf.withExistingConf(activeConf) {

[dongjoon-hyun]

Thank you, @kiszk !

[dongjoon-hyun]

I updated my PR according to @kiszk 's comment.

[kiszk]

After adding the attached code, build/sbt "catalyst/testOnly *AnsiCastSuiteWithAnsiModeOn *ExpressionEncoderSuite -- -z nested" can reproduce the same error.

--- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
+++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala
@@ -960,6 +960,19 @@ abstract class AnsiCastSuiteBase extends CastSuiteBase {
       "invalid input syntax for type numeric")
   }
 
+  test("DUMMY nested") {
+    val activeConf = conf
+    new ParVector(ALL_TIMEZONES.toVector).foreach { zid =>
+      SQLConf.withExistingConf(activeConf) {
+        withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key ->
+          org.apache.spark.sql.catalyst.expressions.CodegenObjectFactoryMode.CODEGEN_ONLY
+            .toString) {
+          val i = 1
+        }
+      }
+    }
+  }
+

[dongjoon-hyun]

@kiszk The problematic part is val activeConf = conf and SQLConf.withExistingConf(activeConf), isn't it?

[kiszk]

I think so in branch 3.1. Your change also works in the above case.

But, I am curious why the same code works at the master branch cc @rednaxelafx

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuite.scala

Line 1026 in 2f30cde

val activeConf = conf

[dongjoon-hyun]

Got it, @kiszk ~

[dongjoon-hyun]

I created a PR to master branch, too. And, I'll track the discussion and adjust according to the decision.

• [SPARK-34696][SQL][TESTS] Fix CodegenInterpretedPlanTest to generate correct test cases #31766