[SPARK-23951][SQL] Use actual java class instead of string representation. #21026

hvanhovell · 2018-04-10T14:36:36Z

What changes were proposed in this pull request?

This PR slightly refactors the newly added ExprValue API by quite a bit. The following changes are introduced:

ExprValue now uses the actual class instead of the class name as its type. This should give some more flexibility with generating code in the future.
Renamed StatementValue to SimpleExprValue. The statement concept is broader then an expression (untyped and it cannot be on the right hand side of an assignment), and this was not really what we were using it for. I have added a top level JavaCode trait that can be used in the future to reinstate (no pun intended) a statement a-like code fragment.
Added factory methods to the JavaCode companion object to make it slightly less verbose to create JavaCode/ExprValue objects. This is also what makes the diff quite large.
Added one more factory method to ExprCode to make it easier to create code-less expressions.

How was this patch tested?

Existing tests.

hvanhovell · 2018-04-10T14:38:02Z

cc @viirya @cloud-fan @rednaxelafx

SparkQA · 2018-04-10T14:45:07Z

Test build #89128 has finished for PR 21026 at commit 821e08a.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2018-04-10T15:38:41Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala

        }

-        if (input.isNull == "false") {
+        if (input.isNull == FalseLiteral) {


This fixes an existing bug :)

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala

SparkQA · 2018-04-10T17:01:36Z

Test build #89129 has finished for PR 21026 at commit 0b194ca.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-04-10T17:27:13Z

Test build #89134 has finished for PR 21026 at commit 45d3ed8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-04-10T17:59:43Z

Test build #89136 has finished for PR 21026 at commit 2488007.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
|class SpecificUnsafeProjection extends $

SparkQA · 2018-04-10T19:11:47Z

Test build #89143 has finished for PR 21026 at commit b2c1601.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2018-04-10T20:19:14Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

      case _ => s"$value = $initCode;"
    }
-    ExprCode(code, FalseLiteral, GlobalValue(value, javaType(dataType)))
+    ExprCode.forNonNullValue(JavaCode.global(value, dataType))


rednaxelafx

Mostly looks good. I left some comments and let's see if we should change them.

rednaxelafx · 2018-04-10T21:38:18Z

.../main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala

    // `rowWriter` is declared as a class field, so we can access it directly in methods.
-    ExprCode(code, FalseLiteral, StatementValue(s"$rowWriter.getRow()", "UnsafeRow",
-      canDirectAccess = true))
+    ExprCode(code, FalseLiteral, SimpleExprValue(s"$rowWriter.getRow()", classOf[UnsafeRow]))


Add a JavaCode.expression(expr: String, javaClass: Class[_]) and use it here?

rednaxelafx · 2018-04-10T21:38:43Z

...rc/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala

    """

-    ExprCode(code, FalseLiteral, VariableValue(output, "MapData"))
+    ExprCode(code, FalseLiteral, VariableValue(output, classOf[MapData]))


Use JavaCode.variable(name, javaClass) here?

rednaxelafx · 2018-04-10T21:39:06Z

...rc/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala

    """

-    ExprCode(code, FalseLiteral, VariableValue(output, "ArrayData"))
+    ExprCode(code, FalseLiteral, VariableValue(output, classOf[ArrayData]))


Use JavaCode.variable(name, javaClass) here?

rednaxelafx · 2018-04-10T21:39:22Z

...rc/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateSafeProjection.scala

       """.stripMargin

-    ExprCode(code, FalseLiteral, VariableValue(output, "InternalRow"))
+    ExprCode(code, FalseLiteral, VariableValue(output, classOf[InternalRow]))


Use JavaCode.variable(name, javaClass) here?

rednaxelafx · 2018-04-10T22:15:10Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+ */
+trait ExprValue extends JavaCode {
+  def javaType: Class[_]
+  def isPrimitive: Boolean = javaType.isPrimitive


This PR got rid of the canDirectAccess property from @viirya 's base change. Should we add it back?
(and implement that property in concrete classes with an override def instead of an override val)

@viirya where are you planning to use this? I removed it because it did not seem to have any benefit (yet).

It's just a few lines in this file, easy to add it back if needed.

@hvanhovell it was introduced for the whole stage codegen when it is needed to know whether a variable can be directly accessed in the scope of a new method or if it should be passed as an argument. I think we should keep it/replace with a def

But it is not being used at the moment right?

not yet,as far as I know @viirya was waiting for this method to be available to go on with his work: #19813 (comment)

Sorry for replying late. Yeah, my plan is to use canDirectAccess to quickly determine whether an ExprValue can be used a method parameter. I will add it back if I need in my work later.

hvanhovell · 2018-04-10T23:26:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

+    val value = eval.isNull match {
+      case TrueLiteral => FalseLiteral
+      case FalseLiteral => TrueLiteral
+      case v => JavaCode.isNullExpression(s"!($v)")


I think we can remove the parens here. SimpleExprValue guarantees this.

SparkQA · 2018-04-11T00:58:15Z

Test build #89157 has finished for PR 21026 at commit 8ab0931.

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

SparkQA · 2018-04-11T03:03:44Z

Test build #89159 has finished for PR 21026 at commit ce39905.

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

SparkQA · 2018-04-11T03:20:51Z

Test build #89160 has finished for PR 21026 at commit e6abd22.

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

cloud-fan · 2018-04-11T12:10:53Z

LGTM, merging to master!

mgaido91 · 2018-04-11T12:11:28Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+  /**
+   * Create a local isNull variable.
+   */
+  def isNullVariable(name: String): VariableValue = variable(name, BooleanType)


actually this is a booleanVariable, rather than a isNullVariable, isn't it?

It is used to create is null variables. That it currently creates a boolean variable is actually not that important. I just want to create a narrow waist for this specific case, so we can swap it out for a more sophisticated implementation later on (something that advertises that it a null check for example).

yes, I see the goal of this, but I can't understand why it is useful. The isNull variable is a boolean as all the other boolean variables... It is the more frequent case, but I can't see any differentiation between them and other booleans. Do you have in mind a case for which this differentiation can be useful?

Feel free to change it.

not a big deal but I'm ok if we change isNullVariable(xxx) to variable(xxx, BooleanType).

mgaido91 · 2018-04-11T12:14:53Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+ */
+trait ExprValue extends JavaCode {
+  def javaType: Class[_]
+  def isPrimitive: Boolean = javaType.isPrimitive


@hvanhovell it was introduced for the whole stage codegen when it is needed to know whether a variable can be directly accessed in the scope of a new method or if it should be passed as an argument. I think we should keep it/replace with a def

mgaido91 · 2018-04-11T12:15:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+  /**
+   * Create a global isNull variable.
+   */
+  def isNullGlobal(name: String): GlobalValue = global(name, BooleanType)


as well, I think this should be a booleanGlobal...

mgaido91 · 2018-04-11T12:16:11Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+  /**
+   * Create a isNull expression fragment.
+   */
+  def isNullExpression(code: String): SimpleExprValue = {


mgaido91 · 2018-04-11T12:23:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+/**
+ * Utility functions for creating [[JavaCode]] fragments.
+ */
+object JavaCode {


despite I really like the introduction of the JavaCode trait, all these methods seems more related to ExprValue (especially the expression one, which represents a SimpleExprValue is very specific to it, and not to JavaCode which can represent also complex expressions instead): shall we move these methods to the ExprValue object?

I wanted to create a narrow waist for the creation JavaCode/ExprValue objects so we can swap out stuff later on. Feel free to move these methods (and remove the JavaCode trait) if you think that is more appropriate.

viirya · 2018-04-11T14:56:51Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/javaCode.scala

+   * -1 for other primitive types.
+   */
+  def defaultLiteral(dataType: DataType): LiteralValue = {
+    new LiteralValue(


Should we just call literal(CodeGenerator.defaultValue(dataType, typedNull = true), dataType)? In current way, defaultLiteral(BooleanType) won't return a FalseLiteral.

Use actual java class instead of string representation.

821e08a

hvanhovell added 2 commits April 10, 2018 16:56

Fix tests

0b194ca

Some minor touch-ups

45d3ed8

hvanhovell commented Apr 10, 2018

View reviewed changes

Merge remote-tracking branch 'apache/master' into SPARK-23951

2488007

# Conflicts: # sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/GenerateUnsafeProjection.scala

Fix float literals

b2c1601

hvanhovell commented Apr 10, 2018

View reviewed changes

oops

8ab0931

rednaxelafx reviewed Apr 10, 2018

View reviewed changes

More factory methods

ce39905

hvanhovell commented Apr 10, 2018

View reviewed changes

Simplify template

e6abd22

asfgit closed this in c604d65 Apr 11, 2018

mgaido91 reviewed Apr 11, 2018

View reviewed changes

viirya reviewed Apr 11, 2018

View reviewed changes

kiszk mentioned this pull request Aug 16, 2018

[SPARK-24505][SQL] Convert strings in codegen to blocks: Cast and BoundAttribute #21537

Closed

[SPARK-23951][SQL] Use actual java class instead of string representation. #21026

[SPARK-23951][SQL] Use actual java class instead of string representation. #21026

Uh oh!

Conversation

hvanhovell commented Apr 10, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

hvanhovell commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

SparkQA commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rednaxelafx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

SparkQA commented Apr 11, 2018

Uh oh!

cloud-fan commented Apr 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hvanhovell Apr 10, 2018 •

edited

Loading

cloud-fan Apr 11, 2018 •

edited

Loading

hvanhovell Apr 11, 2018 •

edited

Loading

cloud-fan Apr 11, 2018 •

edited

Loading

hvanhovell Apr 11, 2018 •

edited

Loading