[SPARK-8117] [SQL] Move codegen implementation into Expression #6660

davies · 2015-06-04T23:36:18Z

This PR move codegen implementation of expressions into Expression class itself, make it easy to manage.

It introduces two APIs in Expression:

def gen(ctx: CodeGenContext): GeneratedExpressionCode
def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): Code

gen(ctx) will call genSource(ctx, ev) to generate Java source code for the current expression. A expression needs to override genSource().

Here are the types:

type Term String
type Code String

/**
 * Java source for evaluating an [[Expression]] given a [[Row]] of input.
 */
case class GeneratedExpressionCode(var code: Code,
                               nullTerm: Term,
                               primitiveTerm: Term,
                               objectTerm: Term)
/**
 * A context for codegen, which is used to bookkeeping the expressions those are not supported
 * by codegen, then they are evaluated directly. The unsupported expression is appended at the
 * end of `references`, the position of it is kept in the code, used to access and evaluate it.
 */
class CodeGenContext {
  /**
   * Holding all the expressions those do not support codegen, will be evaluated directly.
   */
  val references: Seq[Expression] = new mutable.ArrayBuffer[Expression]()
}

SparkQA · 2015-06-04T23:42:25Z

Test build #34224 has finished for PR 6660 at commit 3ff25f8.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-05T00:06:36Z

Test build #34227 has finished for PR 6660 at commit e57959d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class EvaluatedExpression(var code: Code,
- class CodeGenContext
- abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Logging

rxin · 2015-06-05T00:06:39Z

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

-  protected val genericMutableRowType = classOf[GenericMutableRow].getName
-
-  private val curId = new java.util.concurrent.atomic.AtomicInteger()
+case class EvaluatedExpression(var code: Code,


GeneratedExpressionCode?

SparkQA · 2015-06-05T00:15:11Z

Test build #34229 has finished for PR 6660 at commit 8c6d82d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-06-05T00:29:21Z

Test build #34231 has finished for PR 6660 at commit c5fb514.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code,
- class CodeGenContext
- abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Logging

SparkQA · 2015-06-05T01:01:13Z

Test build #34232 has finished for PR 6660 at commit 12ff88a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code,
- class CodeGenContext
- abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Logging

rxin · 2015-06-05T01:53:13Z

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

+   */
+  def equalFunc(dataType: DataType): ((Term, Term) => Code) = dataType match {
+    case BinaryType => { case (eval1, eval2) => s"java.util.Arrays.equals($eval1, $eval2)" }
+    case dt if isNativeType(dt) => { case (eval1, eval2) => s"$eval1 == $eval2" }


does this work? native doesn't mean it works in java?

Only primitive types is native type here, from bool to long, double.

Yea but the definition of native type (4 lines down) is : " List of data types that have special accessors and setters in [[Row]]." Which means in the future we might break this.

I'd make it explicit with primitive type here.

SparkQA · 2015-06-05T04:28:57Z

Test build #34247 has finished for PR 6660 at commit 2344bc0.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code,
- class CodeGenContext
- abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Logging

rxin · 2015-06-05T06:10:08Z

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

@@ -86,6 +87,8 @@ case class Abs(child: Expression) extends UnaryArithmetic {
 abstract class BinaryArithmetic extends BinaryExpression {
  self: Product =>

+  def decimalMethod: String = ""


can u add inline comment explaining what this is for?

[SPARK-8117] [SQL] Move codegen implementation into Expression

rxin · 2015-06-05T08:12:27Z

One idea I have is to use numeric type for the null term. In that case, for a lot of expressions we no longer need conditional branches.

To compute null term: && becomes &, || becomes |, xor becomes ^.

rxin · 2015-06-05T08:12:45Z

BTW - I sent a pull request against your branch with some review comments.

Code gen code review.

SparkQA · 2015-06-05T20:07:02Z

Test build #34294 has finished for PR 6660 at commit b5d3617.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code,
- class CodeGenContext
- abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Logging

SparkQA · 2015-06-05T20:10:44Z

Test build #34306 has finished for PR 6660 at commit 02262c9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var nullTerm: Term, primitiveTerm: Term)
- class CodeGenContext

SparkQA · 2015-06-05T22:42:25Z

Test build #34308 has finished for PR 6660 at commit 86fac2c.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var nullTerm: Term, primitiveTerm: Term)
- class CodeGenContext

JoshRosen · 2015-06-05T22:52:21Z

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

+   * @param ctx a [[CodeGenContext]]
+   * @return [[GeneratedExpressionCode]]
+   */
+  def gen(ctx: CodeGenContext): GeneratedExpressionCode = {


I think that this can be final.

There is some cases that we need to override it.

JoshRosen · 2015-06-05T22:56:02Z

High-level comment: we should have a checklist / guide at the top of codegen/package.scala which gives some general guidelines on how to write codegen code (e.g. code formatting, uniqueness of variable / term names, etc.)

JoshRosen · 2015-06-05T23:00:07Z

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

+ * by codegen, then they are evaluated directly. The unsupported expression is appended at the
+ * end of `references`, the position of it is kept in the code, used to access and evaluate it.
+ */
+class CodeGenContext {


Minor nit, but can this class go into its own file?

We can do this later (reduce the number of changed lines, better for review)

SparkQA · 2015-06-05T23:24:00Z

Test build #34313 has finished for PR 6660 at commit e03edaa.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var nullTerm: Term, var primitiveTerm: Term)
- class CodeGenContext

SparkQA · 2015-06-05T23:55:10Z

Test build #34320 has finished for PR 6660 at commit bad6828.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var nullTerm: Term, var primitiveTerm: Term)
- class CodeGenContext

SparkQA · 2015-06-06T07:49:14Z

Test build #34357 has finished for PR 6660 at commit f42c732.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var isNull: Term, var primitive: Term)
- class CodeGenContext
- case class Pow(left: Expression, right: Expression)
- case class Rint(child: Expression) extends UnaryMathExpression(math.rint, "ROUND")
- case class ToDegrees(child: Expression) extends UnaryMathExpression(math.toDegrees, "DEGREES")
- case class ToRadians(child: Expression) extends UnaryMathExpression(math.toRadians, "RADIANS")

SparkQA · 2015-06-06T07:53:16Z

Test build #34358 has finished for PR 6660 at commit 9adaeaf.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class GeneratedExpressionCode(var code: Code, var isNull: Term, var primitive: Term)
- class CodeGenContext
- case class Pow(left: Expression, right: Expression)
- case class Rint(child: Expression) extends UnaryMathExpression(math.rint, "ROUND")
- case class ToDegrees(child: Expression) extends UnaryMathExpression(math.toDegrees, "DEGREES")
- case class ToRadians(child: Expression) extends UnaryMathExpression(math.toRadians, "RADIANS")

This PR move codegen implementation of expressions into Expression class itself, make it easy to manage. It introduces two APIs in Expression: ``` def gen(ctx: CodeGenContext): GeneratedExpressionCode def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): Code ``` gen(ctx) will call genSource(ctx, ev) to generate Java source code for the current expression. A expression needs to override genSource(). Here are the types: ``` type Term String type Code String /** * Java source for evaluating an [[Expression]] given a [[Row]] of input. */ case class GeneratedExpressionCode(var code: Code, nullTerm: Term, primitiveTerm: Term, objectTerm: Term) /** * A context for codegen, which is used to bookkeeping the expressions those are not supported * by codegen, then they are evaluated directly. The unsupported expression is appended at the * end of `references`, the position of it is kept in the code, used to access and evaluate it. */ class CodeGenContext { /** * Holding all the expressions those do not support codegen, will be evaluated directly. */ val references: Seq[Expression] = new mutable.ArrayBuffer[Expression]() } ``` This is basically apache#6660, but fixed style violation and compilation failure. Author: Davies Liu <[email protected]> Author: Reynold Xin <[email protected]> Closes apache#6690 from rxin/codegen and squashes the following commits: e1368c2 [Reynold Xin] Fixed tests. 73db80e [Reynold Xin] Fixed compilation failure. 19d6435 [Reynold Xin] Fixed style violation. 9adaeaf [Davies Liu] address comments f42c732 [Davies Liu] improve coverage and tests bad6828 [Davies Liu] address comments e03edaa [Davies Liu] consts fold 86fac2c [Davies Liu] fix style 02262c9 [Davies Liu] address comments b5d3617 [Davies Liu] Merge pull request #5 from rxin/codegen 48c454f [Reynold Xin] Some code gen update. 2344bc0 [Davies Liu] fix test 12ff88a [Davies Liu] fix build c5fb514 [Davies Liu] rename 8c6d82d [Davies Liu] update docs b145047 [Davies Liu] fix style e57959d [Davies Liu] add type alias 3ff25f8 [Davies Liu] refactor 593d617 [Davies Liu] pushing codegen into Expression

srowen · 2015-06-08T08:32:45Z

Now that 5e7b6b6 has merged, can this be closed?

This PR move codegen implementation of expressions into Expression class itself, make it easy to manage. It introduces two APIs in Expression: ``` def gen(ctx: CodeGenContext): GeneratedExpressionCode def genCode(ctx: CodeGenContext, ev: GeneratedExpressionCode): Code ``` gen(ctx) will call genSource(ctx, ev) to generate Java source code for the current expression. A expression needs to override genSource(). Here are the types: ``` type Term String type Code String /** * Java source for evaluating an [[Expression]] given a [[Row]] of input. */ case class GeneratedExpressionCode(var code: Code, nullTerm: Term, primitiveTerm: Term, objectTerm: Term) /** * A context for codegen, which is used to bookkeeping the expressions those are not supported * by codegen, then they are evaluated directly. The unsupported expression is appended at the * end of `references`, the position of it is kept in the code, used to access and evaluate it. */ class CodeGenContext { /** * Holding all the expressions those do not support codegen, will be evaluated directly. */ val references: Seq[Expression] = new mutable.ArrayBuffer[Expression]() } ``` This is basically apache#6660, but fixed style violation and compilation failure. Author: Davies Liu <[email protected]> Author: Reynold Xin <[email protected]> Closes apache#6690 from rxin/codegen and squashes the following commits: e1368c2 [Reynold Xin] Fixed tests. 73db80e [Reynold Xin] Fixed compilation failure. 19d6435 [Reynold Xin] Fixed style violation. 9adaeaf [Davies Liu] address comments f42c732 [Davies Liu] improve coverage and tests bad6828 [Davies Liu] address comments e03edaa [Davies Liu] consts fold 86fac2c [Davies Liu] fix style 02262c9 [Davies Liu] address comments b5d3617 [Davies Liu] Merge pull request apache#5 from rxin/codegen 48c454f [Reynold Xin] Some code gen update. 2344bc0 [Davies Liu] fix test 12ff88a [Davies Liu] fix build c5fb514 [Davies Liu] rename 8c6d82d [Davies Liu] update docs b145047 [Davies Liu] fix style e57959d [Davies Liu] add type alias 3ff25f8 [Davies Liu] refactor 593d617 [Davies Liu] pushing codegen into Expression

Davies Liu added 2 commits June 4, 2015 16:04

pushing codegen into Expression

593d617

refactor

3ff25f8

davies changed the title ~~[SPARK-8117] [SQL] Move codegen implementation into Expression~~ [WIP] [SPARK-8117] [SQL] Move codegen implementation into Expression Jun 4, 2015

Davies Liu added 2 commits June 4, 2015 17:00

add type alias

e57959d

fix style

b145047

rxin reviewed Jun 5, 2015
View reviewed changes

update docs

8c6d82d

rename

c5fb514

fix build

12ff88a

rxin reviewed Jun 5, 2015
View reviewed changes

fix test

2344bc0

davies changed the title ~~[WIP] [SPARK-8117] [SQL] Move codegen implementation into Expression~~ [SPARK-8117] [SQL] Move codegen implementation into Expression Jun 5, 2015

rxin reviewed Jun 5, 2015
View reviewed changes

rxin added a commit to rxin/spark that referenced this pull request Jun 5, 2015

Merge pull request apache#6660 from davies/codegen

34cf50a

[SPARK-8117] [SQL] Move codegen implementation into Expression

Some code gen update.

48c454f

Davies Liu added 2 commits June 5, 2015 11:16

Merge pull request #5 from rxin/codegen

b5d3617

Code gen code review.

address comments

02262c9

Davies Liu added 3 commits June 5, 2015 13:42

fix style

86fac2c

consts fold

e03edaa

address comments

bad6828

JoshRosen reviewed Jun 5, 2015
View reviewed changes

Davies Liu added 2 commits June 6, 2015 00:23

improve coverage and tests

f42c732

address comments

9adaeaf

rxin mentioned this pull request Jun 7, 2015

[SPARK-8117] [SQL] Push codegen implementation into each Expression #6690

Closed

davies closed this Jun 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-8117] [SQL] Move codegen implementation into Expression #6660

[SPARK-8117] [SQL] Move codegen implementation into Expression #6660

davies commented Jun 4, 2015

SparkQA commented Jun 4, 2015

SparkQA commented Jun 5, 2015

rxin Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

rxin Jun 5, 2015

davies Jun 5, 2015

rxin Jun 5, 2015

SparkQA commented Jun 5, 2015

rxin Jun 5, 2015

rxin commented Jun 5, 2015

rxin commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

JoshRosen Jun 5, 2015

davies Jun 6, 2015

JoshRosen commented Jun 5, 2015

JoshRosen Jun 5, 2015

davies Jun 6, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 6, 2015

SparkQA commented Jun 6, 2015

srowen commented Jun 8, 2015

[SPARK-8117] [SQL] Move codegen implementation into Expression #6660

[SPARK-8117] [SQL] Move codegen implementation into Expression #6660

Conversation

davies commented Jun 4, 2015

SparkQA commented Jun 4, 2015

SparkQA commented Jun 5, 2015

Choose a reason for hiding this comment

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 5, 2015

Choose a reason for hiding this comment

rxin commented Jun 5, 2015

rxin commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshRosen commented Jun 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jun 5, 2015

SparkQA commented Jun 5, 2015

SparkQA commented Jun 6, 2015

SparkQA commented Jun 6, 2015

srowen commented Jun 8, 2015