[SPARK-4861][SQL] Refactory command in spark sql #3712

scwf · 2014-12-16T15:38:09Z

Remove Command and use RunnableCommand instead.

SparkQA · 2014-12-16T15:42:34Z

Test build #24500 has started for PR 3712 at commit 125f542.

This patch merges cleanly.

SparkQA · 2014-12-16T16:21:24Z

Test build #24500 has finished for PR 3712 at commit 125f542.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class SetCommand(kv: Option[(String, Option[String])], _output: Seq[Attribute])(
- case class UncacheTableCommand(tableName: String) extends RunnableCommand
- case class DescribeCommand(child: SparkPlan, _output: Seq[Attribute])(
- case class AnalyzeTable(tableName: String)(hiveContext: HiveContext) extends RunnableCommand
- case class DropTable(
- case class AddJar(path: String)(hiveContext: HiveContext) extends RunnableCommand
- case class AddFile(path: String)(hiveContext: HiveContext) extends RunnableCommand

AmplabJenkins · 2014-12-16T16:21:27Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24500/
Test FAILed.

marmbrus · 2014-12-16T20:17:30Z

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

Can't we get rid of strategy entirely and just have this single line added to BasicOperators. That was kind of my goal with this refactoring.

I will try, maybe we can add a rule to translate logical.XXXCommand to RunnableCommand in analyzer.

I don think it needs to be in the Analyzer. I think just this single line in the BasicOperators Strategy should be sufficient.

But for set command, catalyst first parse it as logical.SetCommand, when to make it as execution.SetCommand (RunnableCommand)?

We can not avoid this step to translate logical.XXXCommand to RunnableCommand, so keep CommandStrategy here.

Okay, I see the problem now. These commands still live inside of catalyst and thus cannot be RunnableCommands. I would argue that that should be changed. My whole goal here was to eliminate boilerplate from the planner and this still seems to have a bunch of it. That said, this could be done in a follow up PR.

yes, i think we can parse command in sql/core instead of parsing in catalyst in the follow up PR

SparkQA · 2014-12-17T11:37:29Z

Test build #24544 has started for PR 3712 at commit 1d4d018.

This patch merges cleanly.

SparkQA · 2014-12-17T11:38:20Z

Test build #24544 has finished for PR 3712 at commit 1d4d018.

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

AmplabJenkins · 2014-12-17T11:38:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24544/
Test FAILed.

SparkQA · 2014-12-17T13:12:30Z

Test build #24546 has started for PR 3712 at commit 3925492.

This patch merges cleanly.

SparkQA · 2014-12-17T14:07:28Z

Test build #24547 has started for PR 3712 at commit 4033bed.

This patch merges cleanly.

SparkQA · 2014-12-17T14:18:45Z

Test build #24546 has finished for PR 3712 at commit 3925492.

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

AmplabJenkins · 2014-12-17T14:18:48Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24546/
Test PASSed.

SparkQA · 2014-12-17T15:17:01Z

Test build #24547 has finished for PR 3712 at commit 4033bed.

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

AmplabJenkins · 2014-12-17T15:17:04Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24547/
Test PASSed.

marmbrus · 2014-12-17T23:27:08Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/CreateTableAsSelect.scala

Its a little odd to have a var named sc that is not a SparkContext, also I think we should try and eliminate this since we get the SQLContext in the run method. Can we just move metastoreRelation into the run method?

marmbrus · 2014-12-17T23:28:09Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/DescribeHiveTableCommand.scala

Can we just move this to the place in the unit tests where we need it?

marmbrus · 2014-12-17T23:32:09Z

Okay, I think this is getting pretty close. Thanks for working on this!

SparkQA · 2014-12-18T03:22:27Z

Test build #24567 has started for PR 3712 at commit 0e03be8.

This patch merges cleanly.

SparkQA · 2014-12-18T04:01:30Z

Test build #24567 has finished for PR 3712 at commit 0e03be8.

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

AmplabJenkins · 2014-12-18T04:01:33Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24567/
Test FAILed.

SparkQA · 2014-12-18T09:07:27Z

Test build #24584 has started for PR 3712 at commit 51a82f2.

This patch merges cleanly.

SparkQA · 2014-12-18T10:17:09Z

Test build #24584 has finished for PR 3712 at commit 51a82f2.

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

AmplabJenkins · 2014-12-18T10:17:13Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24584/
Test PASSed.

scwf · 2014-12-19T03:07:05Z

Updated. @marmbrus is this ok to go ?

marmbrus · 2014-12-19T04:26:14Z

Thanks! Merged to master.

Follow up for #3712. This PR finally remove ```CommandStrategy``` and make all commands follow ```RunnableCommand``` so they can go with ```case r: RunnableCommand => ExecutedCommand(r) :: Nil```. One exception is the ```DescribeCommand``` of hive, which is a special case and need to distinguish hive table and temporary table, so still keep ```HiveCommandStrategy``` here. Author: scwf <wangfei1@huawei.com> Closes #3948 from scwf/followup-SPARK-4861 and squashes the following commits: 6b48e64 [scwf] minor style fix 2c62e9d [scwf] fix for hive module 5a7a819 [scwf] Refactory command in spark sql

factory command in spark sql

125f542

marmbrus reviewed Dec 16, 2014
View reviewed changes

scwf force-pushed the cmd branch from 1d4d018 to 3925492 Compare December 17, 2014 13:11

scwf added 2 commits December 17, 2014 21:24

address comments

5d20010

remove CreateTableAsSelect in hivestrategy

4033bed

scwf force-pushed the cmd branch from 3925492 to 4033bed Compare December 17, 2014 14:05

marmbrus reviewed Dec 17, 2014
View reviewed changes

address comments

0e03be8

fix test failure

51a82f2

asfgit closed this in c3d91da Dec 19, 2014

scwf deleted the cmd branch December 20, 2014 13:42

scwf mentioned this pull request Jan 8, 2015

[SPARK-4861][SQL] Refactory command in spark sql #3948

Closed

[SPARK-4861][SQL] Refactory command in spark sql #3712

[SPARK-4861][SQL] Refactory command in spark sql #3712

Uh oh!

Conversation

scwf commented Dec 16, 2014

Uh oh!

SparkQA commented Dec 16, 2014

Uh oh!

SparkQA commented Dec 16, 2014

Uh oh!

AmplabJenkins commented Dec 16, 2014

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 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

AmplabJenkins commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

AmplabJenkins commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 17, 2014

Uh oh!

AmplabJenkins commented Dec 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marmbrus commented Dec 17, 2014

Uh oh!

SparkQA commented Dec 18, 2014

Uh oh!

SparkQA commented Dec 18, 2014

Uh oh!

AmplabJenkins commented Dec 18, 2014

Uh oh!

SparkQA commented Dec 18, 2014

Uh oh!

SparkQA commented Dec 18, 2014

Uh oh!

AmplabJenkins commented Dec 18, 2014

Uh oh!

scwf commented Dec 19, 2014

Uh oh!

marmbrus commented Dec 19, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants