Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maropu wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f13d56
[SPARK-34596][SQL] Use Utils.getSimpleName to avoid hitting Malformed…
rednaxelafx Mar 3, 2021
ab77ddb
Minor fix
maropu Mar 19, 2021
5152fff
Add comment
maropu Mar 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,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)"
}
Expand Down
30 changes: 30 additions & 0 deletions ...talyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,36 @@ class ExpressionEncoderSuite extends CodegenInterpretedPlanTest with AnalysisTes

encodeDecodeTest(Array(Option(InnerClass(1))), "array of optional inner class")

// NOTE: branch-2.4 does not have the interpreted implementation of SafeProjection, so
// it does not fall back into the interpreted mode if the compilation fails.
// Therefore, the test in this PR just checks that the compilation error happens
// instead of checking that the interpreted mode works well.
private def checkCompilationError[T : ExpressionEncoder](
input: T,
testName: String): Unit = {
testAndVerifyNotLeakingReflectionObjects(s"compilation error: $testName: $input") {
val encoder = implicitly[ExpressionEncoder[T]]
val row = encoder.toRow(input)
val boundEncoder = encoder.resolveAndBind()
val errMsg = intercept[RuntimeException] {
boundEncoder.fromRow(row)
}.getCause.getMessage
assert(errMsg.contains("failed to compile: "))
Copy link
Member

@viirya viirya Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the compilation error?

[info]   Cause: 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"

Copy link
Member

@viirya viirya Mar 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But by moving MalformedClassObject out of ExpressionEncoderSuite, the generated code can be compiled.

I'm not sure what this test wants to test actually. Am I confused or do I misunderstand it?

Copy link
Member Author

@maropu maropu Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the compilation error?

Yea, that's the expected error. Actually, the master has same error when useFallback = false:

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala

Line 219 in d32bb4e

useFallback = true) 

// master
[info]   Cause: 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]   at org.apache.spark.sql.errors.QueryExecutionErrors$.compilerError(QueryExecutionErrors.scala:328)

But by moving MalformedClassObject out of ExpressionEncoderSuite, the generated code can be compiled.

IIUC the condition where the error happens is that MalformedClassObject is placed in ExpressionEncoderSuite as Kris said in the previous PR: #31709 (comment)

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)

cc: @rednaxelafx

Copy link
Member

@viirya viirya Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks for the reference.

}
}

// holder class to trigger Class.getSimpleName issue
object MalformedClassObject extends Serializable {
case class MalformedNameExample(x: Int)
}

{
OuterScopes.addOuterScope(MalformedClassObject)
checkCompilationError(
MalformedClassObject.MalformedNameExample(42),
"nested Scala class")
}

productTest(PrimitiveData(1, 1, 1, 1, 1, 1, true))

productTest(
Expand Down