[SPARK-22856][SQL] Add wrappers for codegen output and nullability #20043

viirya · 2017-12-21T04:22:15Z

What changes were proposed in this pull request?

The codegen output of Expression, aka ExprCode, now encapsulates only strings of output value (value) and nullability (isNull). It makes difficulty for us to know what the output really is. I think it is better if we can add wrappers for the value and nullability that let us to easily know that.

How was this patch tested?

Existing tests.

viirya · 2017-12-21T06:45:48Z

cc @kiszk @cloud-fan

SparkQA · 2017-12-21T06:49:39Z

Test build #85231 has finished for PR 20043 at commit 78680cc.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExprCode(var code: String, var isNull: ExprValue, var value: ExprValue)
case class LiteralValue(var value: String) extends ExprValue
case class VariableValue(var variableName: String) extends ExprValue
case class StatementValue(var statement: String) extends ExprValue
case class GlobalValue(var value: String) extends ExprValue
case class SubExprEliminationState(isNull: ExprValue, value: ExprValue)

SparkQA · 2017-12-21T07:03:47Z

Test build #85232 has finished for PR 20043 at commit d5c986a.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExprCode(var code: String, var isNull: ExprValue, var value: ExprValue)
case class LiteralValue(val value: String) extends ExprValue
case class VariableValue(val variableName: String) extends ExprValue
case class StatementValue(val statement: String) extends ExprValue
case class GlobalValue(val value: String) extends ExprValue
case class SubExprEliminationState(isNull: ExprValue, value: ExprValue)

SparkQA · 2017-12-21T07:11:32Z

Test build #85234 has finished for PR 20043 at commit 5ace8b8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExprCode(var code: String, var isNull: ExprValue, var value: ExprValue)
case class LiteralValue(val value: String) extends ExprValue
case class VariableValue(val variableName: String) extends ExprValue
case class StatementValue(val statement: String) extends ExprValue
case class GlobalValue(val value: String) extends ExprValue
case class SubExprEliminationState(isNull: ExprValue, value: ExprValue)

cloud-fan · 2017-12-21T07:15:53Z

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

+}
+
+// A global variable evaluation of [[ExprCode]].
+case class GlobalValue(val value: String) extends ExprValue {


for compacted global variables, we may get something like arr[1] while arr is a global variable. Is arr[1] a statement or global variable?

It is considered as global variable now, as it can be accessed globally and don't/can't/shouldn't be a parameter. Actually we don't want to take global variables as parameters.

SparkQA · 2017-12-21T08:05:02Z

Test build #85243 has finished for PR 20043 at commit 81c9b6e.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-12-21T08:05:02Z

Test build #85241 has finished for PR 20043 at commit d120750.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2017-12-21T08:10:23Z

retest this please.

SparkQA · 2017-12-21T10:42:25Z

Test build #85247 has finished for PR 20043 at commit 81c9b6e.

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

HyukjinKwon · 2017-12-21T11:53:12Z

retest this please

viirya · 2017-12-21T13:48:40Z

retest this please.

viirya · 2017-12-21T13:49:02Z

Oh, already re-testing.

viirya · 2017-12-21T13:49:12Z

Thanks @HyukjinKwon

SparkQA · 2017-12-21T14:56:39Z

Test build #85255 has finished for PR 20043 at commit 81c9b6e.

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

kiszk · 2017-12-21T15:08:49Z

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

  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
    val eval = child.genCode(ctx)
-    ExprCode(code = eval.code, isNull = "false", value = eval.isNull)
+    val value = if ("true" == s"${eval.isNull}" || "false" == s"${eval.isNull}") {


if ("true" == eval.isNull || "false" == eval.isNull) {?

or eval.isNull == Literal("true")? Or even better we can create a LiteralTrue = Literal("true") and equivalent for false and use them?

We can do eval.isNull.instanceOf[LiteralValue] as suggested by @cloud-fan below.

kiszk · 2017-12-21T15:09:00Z

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

  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
    val eval = child.genCode(ctx)
-    ExprCode(code = eval.code, isNull = "false", value = s"(!(${eval.isNull}))")
+    val value = if ("true" == s"${eval.isNull}") {


SparkQA · 2017-12-21T16:43:38Z

Test build #85264 has finished for PR 20043 at commit 81c9b6e.

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

cloud-fan · 2017-12-21T17:01:25Z

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

    // TODO: support whole stage codegen too
    if (eval.code.trim.length > 1024 && ctx.INPUT_ROW != null && ctx.currentVars == null) {
-      val setIsNull = if (eval.isNull != "false" && eval.isNull != "true") {
+      val setIsNull = if ("false" != s"${eval.isNull}" && "true" != s"${eval.isNull}") {


this can be simplified to !eval.isNull.instanceOf[LiteralValue]

cloud-fan · 2017-12-21T17:07:30Z

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

+
+
+// An abstraction that represents the evaluation result of [[ExprCode]].
+abstract class ExprValue


I think we should classify ExprValue by our needs, not by java definitions. Thinking about the needs, we wanna know: 1) if this value is accessible anywhere and we don't need to carry it via method parameters. 2) if this value needs to be carried with parameters, do we need to generate a parameter name or use this value directly?

So basically we can combine LiteralValue and GlobalValue.

IMHO I prefer this approach because in the future we might need to distinguish these two cases, thus I think is a good thing to let them be distinct.

For now LiteralValue and GlobalValue can be seen as the same effectively, as they are all accessible anywhere and we don't need to carry it via method parameters.

I don't have strong preference here.

@kiszk WDYT?

In summary, I have no strong preference.

In the future, we will want to distinguish Literal and Global for some optimizations. This is already one of optimizations for Literal.

If this PR just focuses on classifying types between arguments and non-arguments, it is fine to combine Literal and Global. Then, another PR will separate one type into Literal and Global.

If no strong preference for combining them, I'd keep it as two concepts for now, if we foresee the need to distinguish them.

@cloud-fan What do you think?

OK let's keep it.

SparkQA · 2017-12-21T23:52:34Z

Test build #85288 has finished for PR 20043 at commit 53926cc.

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

SparkQA · 2017-12-22T04:23:49Z

Test build #85291 has finished for PR 20043 at commit 4384c84.

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

mgaido91 · 2018-03-01T13:55:26Z

this LGTM, any more comments @cloud-fan @kiszk @rednaxelafx ?

SparkQA · 2018-03-01T15:01:06Z

Test build #87838 has finished for PR 20043 at commit 0841c4a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ExprType(val typeName: String)

SparkQA · 2018-03-01T15:07:23Z

Test build #87843 has finished for PR 20043 at commit e530f01.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class LiteralValue(val value: String, val javaType: String) extends ExprValue
case class GlobalValue(val value: String, val javaType: String) extends ExprValue

mgaido91 · 2018-03-01T15:15:42Z

retest this please

SparkQA · 2018-03-01T18:38:31Z

Test build #87844 has finished for PR 20043 at commit e530f01.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class LiteralValue(val value: String, val javaType: String) extends ExprValue
case class GlobalValue(val value: String, val javaType: String) extends ExprValue

hvanhovell · 2018-03-05T11:50:10Z

@viirya big fan of this change! More structure will make code gen easier & safer to implement. I think we should merge this as is, and then I think it might be good to start adding types to the values, and to make the CodeGenerator and the CodegenContext work directly with these values.

Since I merged @kiszk PR just now, can you update? I am sorry for the hassle.

viirya · 2018-03-05T12:57:11Z

@hvanhovell Thanks! I also think this structure can help us improve codegen. I will update it soon.

SparkQA · 2018-03-05T17:23:21Z

Test build #87958 has finished for PR 20043 at commit c8c70a9.

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

viirya · 2018-03-10T10:11:31Z

ping @hvanhovell @cloud-fan Any more comment for this?

kiszk · 2018-03-10T16:52:17Z

LGTM

viirya · 2018-04-04T09:07:27Z

ping @hvanhovell @cloud-fan

SparkQA · 2018-04-04T11:44:09Z

Test build #88884 has finished for PR 20043 at commit ac2e595.

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

viirya · 2018-04-04T11:56:12Z

retest this please.

SparkQA · 2018-04-04T15:42:47Z

Test build #88887 has finished for PR 20043 at commit ac2e595.

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

maropu · 2018-04-05T08:50:04Z

This pr will be merge soon? I'd like to use this in my pr: #20965

hvanhovell · 2018-04-09T12:06:39Z

retest this please

hvanhovell · 2018-04-09T12:07:26Z

I am going to merge this after this successfully passes tests

SparkQA · 2018-04-09T14:39:58Z

Test build #89056 has finished for PR 20043 at commit ac2e595.

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

viirya · 2018-04-09T14:51:37Z

retest this please.

SparkQA · 2018-04-09T18:48:13Z

Test build #89065 has finished for PR 20043 at commit ac2e595.

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

gatorsmile · 2018-04-09T18:53:18Z

Thanks! Merged to master.

viirya · 2018-04-09T22:33:17Z

Thanks! @cloud-fan @hvanhovell @kiszk @HyukjinKwon @mgaido91 @maropu @rednaxelafx @gatorsmile

cloud-fan · 2018-04-11T11:47:50Z

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

+}
+
+// A literal evaluation of [[ExprCode]].
+class LiteralValue(val value: String, val javaType: String) extends ExprValue {


why not a case class?

We currently have case objects for TrueLiteral and FalseLiteral which extends LiteralValue.

viirya force-pushed the SPARK-22856 branch 3 times, most recently from 8514cb6 to 5ace8b8 Compare December 21, 2017 04:51

Add wrappers for codegen output.

5ace8b8

cloud-fan reviewed Dec 21, 2017

View reviewed changes

viirya added 2 commits December 21, 2017 07:17

Merge remote-tracking branch 'upstream/master' into SPARK-22856

d120750

Fix equality check with string.

81c9b6e

kiszk reviewed Dec 21, 2017

View reviewed changes

cloud-fan reviewed Dec 21, 2017

View reviewed changes

Address comments.

53926cc

viirya added 2 commits December 22, 2017 00:50

Merge remote-tracking branch 'upstream/master' into SPARK-22856

69422d4

Fix merging.

4384c84

Merge remote-tracking branch 'upstream/master' into SPARK-22856

c8c70a9

Merge remote-tracking branch 'upstream/master' into SPARK-22856

ac2e595

rednaxelafx mentioned this pull request Apr 5, 2018

[SPARK-21870][SQL] Split aggregation code into small functions #19082

Closed

asfgit closed this in 7c1654e Apr 9, 2018

cloud-fan 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

viirya deleted the SPARK-22856 branch December 27, 2023 18:35



		// An abstraction that represents the evaluation result of [[ExprCode]].
		abstract class ExprValue

[SPARK-22856][SQL] Add wrappers for codegen output and nullability #20043

[SPARK-22856][SQL] Add wrappers for codegen output and nullability #20043

Uh oh!

Conversation

viirya commented Dec 21, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

viirya commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Dec 21, 2017 • 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!

SparkQA commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

viirya commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

Uh oh!

HyukjinKwon commented Dec 21, 2017

Uh oh!

viirya commented Dec 21, 2017

Uh oh!

viirya commented Dec 21, 2017

Uh oh!

viirya commented Dec 21, 2017

Uh oh!

SparkQA commented Dec 21, 2017

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 Dec 21, 2017

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!

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 Dec 21, 2017

Uh oh!

SparkQA commented Dec 22, 2017

Uh oh!

mgaido91 commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

SparkQA commented Mar 1, 2018

Uh oh!

viirya Dec 21, 2017 •

edited

Loading