Skip to content

[SPARK-35281][SQL] StaticInvoke should not apply boxing if return type is primitive #32416

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
sunchao wants to merge 2 commits into from
Closed

[SPARK-35281][SQL] StaticInvoke should not apply boxing if return type is primitive #32416

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c687d0
first commit
sunchao Apr 30, 2021
cfa15fc
simplify
sunchao May 2, 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
Next Next commit
first commit
  • Loading branch information
@sunchao
sunchao committed May 2, 2021
commit 7c687d0b2db216fc87495eb1d0462abfd83b66ed
20 changes: 12 additions & 8 deletions 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 @@ -263,14 +263,18 @@ case class StaticInvoke(
${ev.isNull} = ${ev.value} == null;
"""
} else {
val boxedResult = ctx.freshName("boxedResult")
s"""
${CodeGenerator.boxedType(dataType)} $boxedResult = $callFunc;
${ev.isNull} = $boxedResult == null;
if (!${ev.isNull}) {
${ev.value} = $boxedResult;
}
"""
if (method.getReturnType.isPrimitive) {
Copy link
Member

@viirya viirya May 2, 2021

Choose a reason for hiding this comment

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

Hmm, why returnNullable is true when return type is primitive?

Copy link
Member

@viirya viirya May 2, 2021

Choose a reason for hiding this comment

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

Doesn't primitive return type go the last else block (i.e. returnNullable = false)?

Copy link
Member Author

@sunchao sunchao May 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the caller of StaticInvoke can set returnNullable to true even though the return type is primitive, so it's better to check here nevertheless. I think I can make the code simpler by reusing the else branch.

Copy link
Member

@viirya viirya May 2, 2021

Choose a reason for hiding this comment

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

Yea, if the caller sets returnNullable correctly, StaticInvoke won't box primitive return value. I agree, it is possible the caller wrongly set returnNullable to true.

s"${ev.value} = $callFunc;"
} else {
val boxedResult = ctx.freshName("boxedResult")
s"""
${CodeGenerator.boxedType(dataType)} $boxedResult = $callFunc;
${ev.isNull} = $boxedResult == null;
if (!${ev.isNull}) {
${ev.value} = $boxedResult;
}
"""
}
}
} else {
s"${ev.value} = $callFunc;"
Expand Down
14 changes: 13 additions & 1 deletion ...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import org.apache.spark.sql.catalyst.ScroogeLikeExample
import org.apache.spark.sql.catalyst.analysis.{ResolveTimeZone, SimpleAnalyzer, UnresolvedDeserializer}
import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.encoders._
import org.apache.spark.sql.catalyst.expressions.codegen.GenerateUnsafeProjection
import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, GenerateUnsafeProjection}
import org.apache.spark.sql.catalyst.expressions.objects._
import org.apache.spark.sql.catalyst.plans.logical.{LocalRelation, Project}
import org.apache.spark.sql.catalyst.util.{ArrayBasedMapData, ArrayData, DateTimeUtils, GenericArrayData, IntervalUtils}
Expand Down Expand Up @@ -641,6 +641,13 @@ class ObjectExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper {
checkObjectExprEvaluation(
Invoke(Literal(obj, clsType), "testFunc", IntegerType, Seq(Literal(1))), 0)
}

test("SPARK-35281: StaticInvoke shouldn't box primitive when result is nullable") {
val ctx = new CodegenContext
val arguments = Seq(Literal(0), Literal(1))
val genCode = StaticInvoke(TestFun.getClass, IntegerType, "foo", arguments).genCode(ctx)
assert(!genCode.code.toString.contains("boxedResult"))
}
}

class TestBean extends Serializable {
Expand All @@ -659,3 +666,8 @@ abstract class BaseClass[T] {
class ConcreteClass extends BaseClass[Int] with Serializable {
override def testFunc(param: Int): Int = param - 1
}

case object TestFun {
def foo(left: Int, right: Int): Int = left + right
}