[SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec #23127

juliuszsompolski · 2018-11-23T20:02:58Z

What changes were proposed in this pull request?

Implement codegen for LocalTableScanExec and ExistingRDDExec. Refactor to share code between LocalTableScanExec, ExistingRDDExec, InputAdapter and RowDataSourceScanExec.

The difference in doProduce between these four was that ExistingRDDExec and RowDataSourceScanExec triggered adding an UnsafeProjection, while InputAdapter and LocalTableScanExec did not.

In the new trait InputRDDCodegen I added a flag createUnsafeProjection which the operators set accordingly.

Note: LocalTableScanExec explicitly creates its input as UnsafeRows, so it was obvious why it doesn't need an UnsafeProjection. But if an InputAdapter may take input that is InternalRows but not UnsafeRows, then I think it doesn't need an unsafe projection just because any other operator that is its parent would do that. That assumes that that any parent operator would always result in some UnsafeProjection being eventually added, and hence the output of the WholeStageCodegen unit would be UnsafeRows. If these assumptions hold, I think createUnsafeProjection could be set to (parent == null).

Note: Do not codegen LocalTableScanExec when it's the only operator. LocalTableScanExec has optimized driver-only executeCollect and executeTake code paths that are used to return Command results without starting Spark Jobs. They can no longer be used if the LocalTableScanExec gets optimized.

How was this patch tested?

Covered and used in existing tests.

juliuszsompolski · 2018-11-23T20:05:00Z

cc @hvanhovell @rednaxelafx

SparkQA · 2018-11-23T20:47:26Z

Test build #99218 has finished for PR 23127 at commit 23c2d91.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait InputRDDCodegen extends CodegenSupport
case class InputAdapter(child: SparkPlan) extends UnaryExecNode with InputRDDCodegen

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala

hvanhovell · 2018-11-23T20:53:54Z

Looks good. One more higher level question that can also be addressed in a follow-up.

cloud-fan · 2018-11-26T04:22:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala

+  /**
+   * Helper default should stop check code.
+   */
+  def shouldStopCheckCode: String = if (needStopCheck) {


we can use it in more places. This can be done in folllowup.

cloud-fan · 2018-11-26T04:23:09Z

LGTM

cloud-fan · 2018-11-26T04:23:19Z

retest this please

SparkQA · 2018-11-26T05:05:56Z

Test build #99258 has finished for PR 23127 at commit 23c2d91.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait InputRDDCodegen extends CodegenSupport
case class InputAdapter(child: SparkPlan) extends UnaryExecNode with InputRDDCodegen

SparkQA · 2018-11-26T05:11:18Z

Test build #99259 has finished for PR 23127 at commit 23c2d91.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait InputRDDCodegen extends CodegenSupport
case class InputAdapter(child: SparkPlan) extends UnaryExecNode with InputRDDCodegen

sql/core/src/main/scala/org/apache/spark/sql/execution/ExistingRDD.scala

juliuszsompolski · 2018-11-26T15:06:37Z

@cloud-fan @rednaxelafx
Actually, the input to a codegen stage can be an internal row so I can't make the inputRDD be RDD[UnsafeRow], but the output needs to be UnsafeRow.
Doing it like InputAdapter did actually make it just pass-through output the internal row.
For InputAdapter, there always is some parent operator to consume it, and create an unsafe projection in whatever it does, and then the output UnsafeRows.
But for an RDDScanExec or RowDataSourceScanExec could be alone in a WholeStageCodegenExec, and then just doing ${consume(ctx, null, row)} made it pass-through output the InternalRow from input.

WDYT about how I patched it up?

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala

SparkQA · 2018-11-26T17:33:09Z

Test build #99275 has finished for PR 23127 at commit 170073e.

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

SparkQA · 2018-11-26T22:13:01Z

Test build #99286 has finished for PR 23127 at commit 5d94c42.

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

cloud-fan · 2018-11-27T03:38:45Z

there are still 2 golden file test failures because of the plan change...

juliuszsompolski · 2018-11-27T10:35:36Z

@cloud-fan Thanks. Actually, I had to revert earlier updates because the plan no longer changes for LocalTableScanExec that is alone in a wholestagecodegen.

juliuszsompolski · 2018-11-27T10:46:43Z

Talked with @hvanhovell offline and set LocalTableScanExec and InputAdapter to not create an unsafe projection, and RDDScanExec and RowDataSourceScanExec to always do so, which replicates previous behaviour.

SparkQA · 2018-11-27T13:16:47Z

Test build #99320 has finished for PR 23127 at commit c345e2f.

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

SparkQA · 2018-11-27T13:42:06Z

Test build #99321 has finished for PR 23127 at commit bdb71d7.

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

juliuszsompolski · 2018-11-27T14:00:54Z

jenkins retest this please

hvanhovell

LGTM - @cloud-fan can you take another look?

SparkQA · 2018-11-27T16:46:53Z

Test build #99329 has finished for PR 23127 at commit bdb71d7.

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

MaxGekk · 2018-11-27T16:59:19Z

jenkins retest this please

@juliuszsompolski It won't help, you need to fix python tests it seems

Failed example:
    df.explain()
Differences (ndiff with -expected +actual):
      == Physical Plan ==
    - Scan ExistingRDD[age#0,name#1]
    + *(1) Scan ExistingRDD[age#0,name#1]
    ? +++++

SparkQA · 2018-11-27T23:32:47Z

Test build #99339 has finished for PR 23127 at commit 3668f97.

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

cloud-fan · 2018-11-28T05:37:22Z

thanks, merging to master!

## What changes were proposed in this pull request? Implement codegen for `LocalTableScanExec` and `ExistingRDDExec`. Refactor to share code between `LocalTableScanExec`, `ExistingRDDExec`, `InputAdapter` and `RowDataSourceScanExec`. The difference in `doProduce` between these four was that `ExistingRDDExec` and `RowDataSourceScanExec` triggered adding an `UnsafeProjection`, while `InputAdapter` and `LocalTableScanExec` did not. In the new trait `InputRDDCodegen` I added a flag `createUnsafeProjection` which the operators set accordingly. Note: `LocalTableScanExec` explicitly creates its input as `UnsafeRows`, so it was obvious why it doesn't need an `UnsafeProjection`. But if an `InputAdapter` may take input that is `InternalRows` but not `UnsafeRows`, then I think it doesn't need an unsafe projection just because any other operator that is its parent would do that. That assumes that that any parent operator would always result in some `UnsafeProjection` being eventually added, and hence the output of the `WholeStageCodegen` unit would be `UnsafeRows`. If these assumptions hold, I think `createUnsafeProjection` could be set to `(parent == null)`. Note: Do not codegen `LocalTableScanExec` when it's the only operator. `LocalTableScanExec` has optimized driver-only `executeCollect` and `executeTake` code paths that are used to return `Command` results without starting Spark Jobs. They can no longer be used if the `LocalTableScanExec` gets optimized. ## How was this patch tested? Covered and used in existing tests. Closes apache#23127 from juliuszsompolski/SPARK-26159. Authored-by: Juliusz Sompolski <julek@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

localtablescanexec codegen

23c2d91

juliuszsompolski changed the title ~~[SPARK-26159] Codegen for LocalTableScanExec and ExistingRDDExec~~ [SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec Nov 23, 2018

hvanhovell reviewed Nov 23, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala Show resolved Hide resolved

cloud-fan reviewed Nov 26, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/ExistingRDD.scala Show resolved Hide resolved

juliuszsompolski added 2 commits November 26, 2018 13:08

add RowDataSourceScanExec

ea93f10

fix

170073e

cloud-fan reviewed Nov 26, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala Show resolved Hide resolved

cloud-fan reviewed Nov 26, 2018

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala Outdated Show resolved Hide resolved

juliuszsompolski added 3 commits November 26, 2018 20:28

updateNumOutputRowsMetrics

2868b11

update tests

fce0512

don't codegen LocalTableScanExec when it's all alone

5d94c42

revert test changes

c345e2f

always do UnsafeProjection in RDDScanExec and RowDataSourceScanExec

bdb71d7

cloud-fan approved these changes Nov 27, 2018

View reviewed changes

hvanhovell approved these changes Nov 27, 2018

View reviewed changes

fix python test

3668f97

asfgit closed this in 8c68718 Nov 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec #23127

[SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec #23127

juliuszsompolski commented Nov 23, 2018 •

edited

Loading

juliuszsompolski commented Nov 23, 2018

SparkQA commented Nov 23, 2018

hvanhovell commented Nov 23, 2018

cloud-fan Nov 26, 2018 •

edited

Loading

cloud-fan commented Nov 26, 2018

cloud-fan commented Nov 26, 2018

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

juliuszsompolski commented Nov 26, 2018 •

edited

Loading

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

cloud-fan commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

SparkQA commented Nov 27, 2018

SparkQA commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

hvanhovell left a comment

SparkQA commented Nov 27, 2018

MaxGekk commented Nov 27, 2018

SparkQA commented Nov 27, 2018

cloud-fan commented Nov 28, 2018

[SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec #23127

[SPARK-26159] Codegen for LocalTableScanExec and RDDScanExec #23127

Conversation

juliuszsompolski commented Nov 23, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

juliuszsompolski commented Nov 23, 2018

SparkQA commented Nov 23, 2018

hvanhovell commented Nov 23, 2018

cloud-fan Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

cloud-fan commented Nov 26, 2018

cloud-fan commented Nov 26, 2018

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

juliuszsompolski commented Nov 26, 2018 • edited Loading

SparkQA commented Nov 26, 2018

SparkQA commented Nov 26, 2018

cloud-fan commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

SparkQA commented Nov 27, 2018

SparkQA commented Nov 27, 2018

juliuszsompolski commented Nov 27, 2018

hvanhovell left a comment

Choose a reason for hiding this comment

SparkQA commented Nov 27, 2018

MaxGekk commented Nov 27, 2018

SparkQA commented Nov 27, 2018

cloud-fan commented Nov 28, 2018

juliuszsompolski commented Nov 23, 2018 •

edited

Loading

cloud-fan Nov 26, 2018 •

edited

Loading

juliuszsompolski commented Nov 26, 2018 •

edited

Loading