[SPARK-8935][SQL] Implement code generation for all casts #7365

yjshen · 2015-07-13T07:02:35Z

JIRA: https://issues.apache.org/jira/browse/SPARK-8935

yjshen · 2015-07-13T07:13:31Z

Not ready to be reviewed, just want to trigger test first.

davies · 2015-07-13T07:15:25Z

Jenkins, OK to test

davies · 2015-07-13T22:02:41Z

@YijieSHEN Just want to mention that you can easily test it locally by:

$ buid/sbt
sbt> catalyst/test
sbt> sql/test

SparkQA · 2015-07-13T22:13:51Z

Test build #1061 has finished for PR 7365 at commit 6c00e5a.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

rxin · 2015-07-13T23:00:44Z

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

-      case (BinaryType, StringType) =>
-        defineCodeGen (ctx, ev, c =>
-          s"${ctx.stringType}.fromBytes($c)")
+  private[this] def castGen(from: DataType, to: DataType, ctx: CodeGenContext,


why not remove this and put it into genCode directly?

yjshen · 2015-07-14T05:18:44Z

@davies , thanks for the tip, it's just what I need to check any unconscious situations. :)

@davies @rxin, the pr is almost done for the current implementation, I introduce CodeHolder in the latest commit to support complex types cast, can you have a quick glimpse of it? Not sure if I'm using too much unnecessary abstraction again.

yjshen · 2015-07-14T05:24:52Z

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

@@ -421,48 +421,515 @@ case class Cast(child: Expression, dataType: DataType) extends UnaryExpression w

  protected override def nullSafeEval(input: Any): Any = cast(input)

+  private[this] class CodeHolder private() {


It imitate the behaviour of defineCodeGen and nullSafeCodeGen, hold the calculation for either genCode if it's a primitive type cast or to be later used in ComplexType's member evaluation

seems a bit too much here to be honest

yjshen · 2015-07-15T08:59:00Z

CodeHolder is removed in the latest commit.

rxin · 2015-07-15T18:02:48Z

@cloud-fan Take a look at this also if you have time.

yjshen · 2015-07-15T23:57:27Z

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

@@ -371,7 +371,7 @@ case class Cast(child: Expression, dataType: DataType) extends UnaryExpression w

  private[this] def castArray(from: ArrayType, to: ArrayType): Any => Any = {
    val elementCast = cast(from.elementType, to.elementType)
-    buildCast[Seq[Any]](_, _.map(v => if (v == null) null else elementCast(v)))
+    buildCast[Seq[Any]](_, seq => seq.map(v => if (v == null) null else elementCast(v)))


ah, change this by accident, will revert in following commit.

yjshen · 2015-07-18T09:12:38Z

@cloud-fan , thanks for reviewing, will resolve the comment soon.

yjshen · 2015-07-19T02:55:08Z

@rxin , Since #7488 is merged, can we trigger the test now?

rxin · 2015-07-19T03:26:26Z

Jenkins, ok to test.

rxin · 2015-07-19T03:26:32Z

I'm pretty sure Jenkins hates you.

rxin · 2015-07-19T04:51:07Z

Jenkins, ok to test.

rxin · 2015-07-19T04:51:11Z

Jenkins, test this please.

SparkQA · 2015-07-19T06:19:16Z

Test build #37752 has finished for PR 7365 at commit 5de0a95.

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

SparkQA · 2015-07-19T15:41:07Z

Test build #37773 has finished for PR 7365 at commit 1683876.

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

SparkQA · 2015-07-19T19:55:53Z

Test build #37778 has finished for PR 7365 at commit d01eada.

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

SparkQA · 2015-07-20T02:53:09Z

Test build #37784 has finished for PR 7365 at commit 80378a5.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class ConcatWs(children: Seq[Expression])

cloud-fan · 2015-07-20T03:04:21Z

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

+      ctx: CodeGenContext): CastFunction = to match {
+
+    case _ if from == NullType => (c, evPrim, evNull) => s"$evNull = true;"
+    case _ if to == from => (c, evPrim, evNull) => s"$evPrim = $c;"


Optimizer will remove unnecessary casts, so I think we don't need this case.

Ah, let me check why optimizer didn't work for this

I get this, checkEvaluation in testSuite doesn't trigger optimiser at all, does it? So I think I should remove unnecessary cast introduced in DateExpressionSuite, what do you think?

According to the fact that we also do cast in interpreted version when from == to, let's do this in codegen version too.

I guess its reasonable to remove unnecessary cast introduced in DateExpressionSuite and remove the interpreted version as well?

It's ok to keep them. I don't really have a strong preference on this one.

cast expression will be inserted during physical operator transformation,for example Average, in which case its possible we add a cast to self expression.

SparkQA · 2015-07-20T06:19:41Z

Test build #37800 has finished for PR 7365 at commit fd7eba4.

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

yjshen · 2015-07-21T16:40:58Z

More comments on this?

davies · 2015-07-21T17:05:22Z

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

+  }
+
+  private[this] def castCode(ctx: CodeGenContext, childPrim: String, childNull: String,
+    resultPrim: String, resultNull: String, resultType: DataType, cast: CastFunction): String = {


Nit: we can use result as GeneratedExpressionCode for resultPrim, resultNull, child for childPrim and childNull. resultType is this.dataType.

Right now, it has two many parameters, hard to read.

This function is short, I'd like to inline it into genCode(), it will be easier to understand.

castCode is intendedly take all these parameters because we have to do cast for ComplexType as well, so we need call castCode recursively in these casts.
I think I should add a comment in code here.

It's used by castStructCode, never mind the above comments.

SparkQA · 2015-07-22T02:02:36Z

Test build #38013 has finished for PR 7365 at commit eaece18.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

yjshen · 2015-07-22T02:06:10Z

It's a run-tests.py error:

File "./dev/run-tests.py", line 66, in __main__.identify_changed_files_from_git_commits
Failed example:
    [x.name for x in determine_modules_for_files(             identify_changed_files_from_git_commits("fc0a1475ef", target_ref="5da21f07"))]
Exception raised:

yjshen · 2015-07-22T02:26:37Z

@davies , could you review this again? thanks!

davies · 2015-07-22T05:51:51Z

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

+          $result.update($i, null);
+        } else {
+          $fromType $fromFieldPrim =
+            ${unboxPrimitive(ctx, from.fields(i).dataType, s"$tmpRow.apply($i)")};


Use should use the special getter to access them, because UnsafeRow does not support generic getter for primitive types, see ctx.getColumn, then we don't need to unboxPrimitive()

Thanks, I get this, I will use ctx.getColumn and ctx.setColumn in new commit.

SparkQA · 2015-07-22T07:53:37Z

Test build #1153 has finished for PR 7365 at commit eaece18.

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

SparkQA · 2015-07-22T10:01:10Z

Test build #38067 has finished for PR 7365 at commit ef6e8b5.

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

yjshen · 2015-07-23T01:27:05Z

build error caused by :Error: Invalid or corrupt jarfile build/sbt-launch-0.13.7.jar
please trigger the test again?

SparkQA · 2015-07-23T05:45:43Z

Test build #1183 has finished for PR 7365 at commit ef6e8b5.

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

davies · 2015-07-23T06:43:41Z

LGTM

davies · 2015-07-23T06:44:12Z

Merging into master, thanks!

cloud-fan · 2015-07-28T05:03:35Z

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

+      s"""
+        final int $size = $c.size();
+        final $arraySeqClass<Object> $result = new $arraySeqClass<Object>($size);
+        for (int $j = 0; $j < $size; $j ++) {


is there a reason that we create fresh name for these temp variables?

Suppose we are casting a structType that has two arrayType field, all the structType's filed will be expand one by one, therefore, we need fresh name for each of the arrayField cast to avoid redeclaring variables in the same scope.

rxin reviewed Jul 13, 2015
View reviewed changes

yjshen reviewed Jul 14, 2015
View reviewed changes

yjshen reviewed Jul 15, 2015
View reviewed changes

yjshen force-pushed the cast_codegen branch 2 times, most recently from 2edc21c to 5de0a95 Compare July 18, 2015 14:19

yjshen changed the title ~~[SPARK-8935][WIP] Implement code generation for all casts~~ [SPARK-8935][SQL] Implement code generation for all casts Jul 18, 2015

yjshen force-pushed the cast_codegen branch from 1683876 to d01eada Compare July 19, 2015 18:27

yjshen added 2 commits July 20, 2015 08:11

[WIP] finished primitive part

4edfd76

moved all the cast code into class body

c286f13

yjshen added 2 commits July 20, 2015 08:11

Bug fix: NullType & primitive object unboxing

611d66e

the missing self cast

80378a5

yjshen force-pushed the cast_codegen branch from d01eada to 80378a5 Compare July 20, 2015 01:10

cloud-fan reviewed Jul 20, 2015
View reviewed changes

resolve comments

fd7eba4

davies reviewed Jul 21, 2015
View reviewed changes

remove null case in cast code gen

eaece18

davies reviewed Jul 22, 2015
View reviewed changes

getColumn and setColumn in struct cast, autounboxing in array and map

ef6e8b5

asfgit closed this in 6d0d8b4 Jul 23, 2015

yjshen deleted the cast_codegen branch July 28, 2015 04:04

cloud-fan reviewed Jul 28, 2015
View reviewed changes

		@@ -421,48 +421,515 @@ case class Cast(child: Expression, dataType: DataType) extends UnaryExpression w

		protected override def nullSafeEval(input: Any): Any = cast(input)

		private[this] class CodeHolder private() {

[SPARK-8935][SQL] Implement code generation for all casts #7365

[SPARK-8935][SQL] Implement code generation for all casts #7365

Conversation

yjshen commented Jul 13, 2015

yjshen commented Jul 13, 2015

davies commented Jul 13, 2015

davies commented Jul 13, 2015

SparkQA commented Jul 13, 2015

Choose a reason for hiding this comment

yjshen commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjshen commented Jul 15, 2015

rxin commented Jul 15, 2015

Choose a reason for hiding this comment

yjshen commented Jul 18, 2015

yjshen commented Jul 19, 2015

rxin commented Jul 19, 2015

rxin commented Jul 19, 2015

rxin commented Jul 19, 2015

rxin commented Jul 19, 2015

SparkQA commented Jul 19, 2015

SparkQA commented Jul 19, 2015

SparkQA commented Jul 19, 2015

SparkQA commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 20, 2015

yjshen commented Jul 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2015

yjshen commented Jul 22, 2015

yjshen commented Jul 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2015

SparkQA commented Jul 22, 2015

yjshen commented Jul 23, 2015

SparkQA commented Jul 23, 2015

davies commented Jul 23, 2015

davies commented Jul 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment