[SPARK-30765][SQL] Refine base operator abstraction code style by Eric5553 · Pull Request #27511 · apache/spark

Eric5553 · 2020-02-09T15:29:30Z

What changes were proposed in this pull request?

When doing base operator abstraction work, we found there are still some code snippet is inconsistent with other abstraction code style. This PR addressed following two code refactor cases.

Case 1 Override keyword missed for some fields in derived classes. The compiler will not capture it if we rename some fields in the future.

Case 2 Inconsistent abstract class field definition. The updated style will simplify derived class definition, e.g. EvalPythonExec WindowExecBase

Why are the changes needed?

Improve the code style consistency and code quality

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests

Eric5553 · 2020-02-09T15:30:08Z

cc @gatorsmile @maropu

maropu · 2020-02-09T22:56:24Z

ok to test

maropu · 2020-02-09T22:56:40Z

cc: @cloud-fan @dongjoon-hyun

SparkQA · 2020-02-10T02:57:02Z

Test build #118101 has finished for PR 27511 at commit 87c2a32.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SubqueryExec(override val name: String, override val child: SparkPlan)
case class ReusedSubqueryExec(override val child: BaseSubqueryExec)
case class CollectLimitExec(override val limit: Int, child: SparkPlan) extends LimitExec
case class CollectTailExec(override val limit: Int, child: SparkPlan) extends LimitExec
case class LocalLimitExec(override val limit: Int, child: SparkPlan) extends BaseLimitExec
case class GlobalLimitExec(override val limit: Int, child: SparkPlan) extends BaseLimitExec
case class ArrowEvalPythonExec(
case class BatchEvalPythonExec(
abstract class EvalPythonExec extends UnaryExecNode
case class StreamingLocalLimitExec(override val limit: Int, child: SparkPlan)
abstract class WindowExecBase extends UnaryExecNode

HyukjinKwon · 2020-02-10T09:45:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/python/WindowInPandasExec.scala

Is override val required here?

same question. case class means everything is val so this is just adding override.

@HyukjinKwon @cloud-fan Thanks! I dropped most changes based on this comment #27368 (comment). So current refactor rules are:

If abstract class or trait defined with def, then no override needed in derived class.

Change HashJoin definition to def to avoid override in derived classes.

Remove abstract class constructer part of EvalPythonExec WindowExecBase to sync with other operator code style.

SparkQA · 2020-02-11T18:38:01Z

Test build #118240 has finished for PR 27511 at commit cc709a6.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class ReusedSubqueryExec(child: BaseSubqueryExec)
case class CollectTailExec(limit: Int, child: SparkPlan) extends LimitExec
case class LocalLimitExec(limit: Int, child: SparkPlan) extends BaseLimitExec
case class GlobalLimitExec(limit: Int, child: SparkPlan) extends BaseLimitExec

SparkQA · 2020-02-11T18:40:41Z

Test build #118245 has finished for PR 27511 at commit e8a6a4e.

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

SparkQA · 2020-02-11T18:59:19Z

Test build #118239 has finished for PR 27511 at commit 7ad8d83.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SubqueryExec(name: String, child: SparkPlan)
case class CollectLimitExec(limit: Int, child: SparkPlan) extends LimitExec
case class ArrowEvalPythonExec(udfs: Seq[PythonUDF], resultAttrs: Seq[Attribute], child: SparkPlan,
case class BatchEvalPythonExec(udfs: Seq[PythonUDF], resultAttrs: Seq[Attribute], child: SparkPlan)
case class StreamingLocalLimitExec(limit: Int, child: SparkPlan)

SparkQA · 2020-02-11T19:47:43Z

Test build #118242 has finished for PR 27511 at commit 9511b34.

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

sql/core/src/main/scala/org/apache/spark/sql/execution/window/WindowExecBase.scala

maropu · 2020-02-12T00:33:40Z

Looks fine now and I'll leave this to the others. @gatorsmile @cloud-fan @HyukjinKwon

SparkQA · 2020-02-12T07:28:29Z

Test build #118272 has finished for PR 27511 at commit d07cc1b.

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

HyukjinKwon · 2020-02-25T08:46:23Z

sql/core/src/main/scala/org/apache/spark/sql/execution/window/WindowExecBase.scala

-    partitionSpec: Seq[Expression],
-    orderSpec: Seq[SortOrder],
-    child: SparkPlan) extends UnaryExecNode {
+abstract class WindowExecBase extends UnaryExecNode {


I think we can switch it to trait now.

I see, updated in 364f5a7. Thanks!

HyukjinKwon · 2020-02-25T08:47:32Z

sql/core/src/main/scala/org/apache/spark/sql/execution/python/EvalPythonExec.scala

 */
-abstract class EvalPythonExec(udfs: Seq[PythonUDF], resultAttrs: Seq[Attribute], child: SparkPlan)
-  extends UnaryExecNode {
+abstract class EvalPythonExec extends UnaryExecNode {


This one too. seems we can switch to trait.

Updated in 364f5a7, thanks!

HyukjinKwon

Looks good to me too otherwise.

cloud-fan · 2020-02-25T11:24:32Z

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

    handledFilters: Set[Filter],
    rdd: RDD[InternalRow],
-    @transient relation: BaseRelation,
+    @transient override val relation: BaseRelation,


do we need override?

Oh, I think I forgot to update here. Per previous discussion, we should replace base definition to 'def' instead of adding these overrides.

cloud-fan · 2020-02-25T11:24:39Z

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

 */
 case class FileSourceScanExec(
-    @transient relation: HadoopFsRelation,
+    @transient override val relation: HadoopFsRelation,


SparkQA · 2020-02-25T15:10:21Z

Test build #118919 has finished for PR 27511 at commit 364f5a7.

This patch fails SparkR unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait EvalPythonExec extends UnaryExecNode
trait WindowExecBase extends UnaryExecNode

SparkQA · 2020-02-25T20:18:17Z

Test build #118925 has finished for PR 27511 at commit 839f7b0.

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

HyukjinKwon · 2020-02-27T07:40:01Z

Merged to master.

Eric5553 · 2020-02-27T07:42:34Z

Thanks a lot! @HyukjinKwon @cloud-fan @maropu

### What changes were proposed in this pull request? When doing base operator abstraction work, we found there are still some code snippet is inconsistent with other abstraction code style. This PR addressed following two code refactor cases. **Case 1** Override keyword missed for some fields in derived classes. The compiler will not capture it if we rename some fields in the future. **Case 2** Inconsistent abstract class field definition. The updated style will simplify derived class definition, e.g. `EvalPythonExec` `WindowExecBase` ### Why are the changes needed? Improve the code style consistency and code quality ### Does this PR introduce any user-facing change? No ### How was this patch tested? Existing tests Closes apache#27511 from Eric5553/BaseClassAbstraction. Authored-by: Eric Wu <492960551@qq.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

Eric5553 changed the title ~~Refine base operator abstraction code style~~ [SPARK-30765][SQL] Refine base operator abstraction code style Feb 9, 2020

HyukjinKwon reviewed Feb 10, 2020

View reviewed changes

dongjoon-hyun added the SQL label Feb 10, 2020

maropu reviewed Feb 12, 2020

View reviewed changes

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

Eric5553 added 6 commits February 12, 2020 10:36

Refine abstraction code style

8097492

adjust code based on more commented rules

d04092f

adjust code based on more commented rules

183eb41

minor fix

b94871b

refine HashJoin

cdb0e3e

address comment

d07cc1b

Eric5553 force-pushed the BaseClassAbstraction branch from e8a6a4e to d07cc1b Compare February 12, 2020 02:43

HyukjinKwon reviewed Feb 25, 2020

View reviewed changes

HyukjinKwon approved these changes Feb 25, 2020

View reviewed changes

use trait

364f5a7

cloud-fan reviewed Feb 25, 2020

View reviewed changes

address comment

839f7b0

HyukjinKwon closed this in bce8d93 Feb 27, 2020

Eric5553 deleted the BaseClassAbstraction branch March 13, 2020 06:51

Conversation

Eric5553 commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Eric5553 commented Feb 9, 2020

Uh oh!

maropu commented Feb 9, 2020

Uh oh!

maropu commented Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Feb 10, 2020

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 Feb 11, 2020

Uh oh!

SparkQA commented Feb 11, 2020

Uh oh!

SparkQA commented Feb 11, 2020

Uh oh!

SparkQA commented Feb 11, 2020

Uh oh!

Uh oh!

maropu commented Feb 12, 2020

Uh oh!

SparkQA commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eric5553 Feb 25, 2020 • 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!

Eric5553 Feb 25, 2020 • 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!

HyukjinKwon 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!

SparkQA commented Feb 25, 2020

Uh oh!

SparkQA commented Feb 25, 2020

Uh oh!

HyukjinKwon commented Feb 27, 2020

Uh oh!

Eric5553 commented Feb 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Eric5553 commented Feb 9, 2020 •

edited

Loading

maropu commented Feb 9, 2020 •

edited

Loading

Eric5553 Feb 25, 2020 •

edited

Loading

Eric5553 Feb 25, 2020 •

edited

Loading