[SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ... #2397

ravipesala · 2014-09-15T10:39:14Z

This feature allows user to add cache table from the select query.
Example : CACHE TABLE testCacheTable AS SELECT * FROM TEST_TABLE
Spark takes this type of SQL as command and it does lazy caching just like SQLContext.cacheTable, CACHE TABLE <name> does.
It can be executed from both SQLContext and HiveContext.

Recreated the pull request after rebasing with master.And fixed all the comments raised in previous pull requests.
#2381
#2390

Author : ravipesala ravindra.pesala@huawei.com

This feature allows user to add cache table from the select query. Example : ADD CACHE TABLE <tableName> AS SELECT * FROM TEST_TABLE. Spark takes this type of SQL as command and it does eager caching. It can be executed from SQLContext and HiveContext. Signed-off-by: ravipesala <ravindra.pesala@huawei.com>

Add-Cache-table-as

…into SPARK-2594

SparkQA · 2014-09-15T10:42:08Z

Can one of the admins verify this patch?

davies · 2014-09-15T17:50:11Z

ok to test.

SparkQA · 2014-09-15T17:54:15Z

QA tests have started for PR 2397 at commit c18aa38.

This patch merges cleanly.

SparkQA · 2014-09-15T19:22:28Z

QA tests have finished for PR 2397 at commit c18aa38.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan)

liancheng · 2014-09-16T01:56:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/SqlParser.scala

+    CACHE ~ TABLE ~> ident ~ opt(AS ~ select) <~ opt(";") ^^ {
+      case tableName ~ None => 
+        CacheCommand(tableName, true)
+      case tableName ~ Some(as ~ plan) =>


We can remove as ~ if we use opt(AS ~> select) in line 186.

Thank you for your suggestion. I have updated it as per your comment.

liancheng · 2014-09-16T02:08:26Z

To me the only important issue here is the laziness semantics of CACHE TABLE AS SELECT. I tend to make it lazy because SQLContext.cacheTable, CACHE TABLE <name> are both lazy. Making all three eager also seems acceptable, but this kinda breaks downward compatibility (or at least breaks existing performance assumptions of caching functions/statements).

Otherwise LGTM except for some minor issues mentioned in the comments.

SparkQA · 2014-09-16T10:44:09Z

QA tests have started for PR 2397 at commit d6e469d.

This patch merges cleanly.

ravipesala · 2014-09-16T11:37:39Z

Making lazy cache seems to be a good idea.As SQLContext.cacheTable, CACHE TABLE <name> are both lazy so It is better to make CACHE TABLE AS SELECT .. also lazy to avoid confusion. @marmbrus can comment on this.

SparkQA · 2014-09-16T12:10:33Z

QA tests have finished for PR 2397 at commit d6e469d.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan)

marmbrus · 2014-09-16T17:47:35Z

I agree with making it lazy. Can you also update the title and the description to reflect the final syntax?

ravipesala · 2014-09-16T20:13:54Z

Changed the behavior from eager to lazy caching. And also updated the description.

SparkQA · 2014-09-16T20:14:21Z

QA tests have started for PR 2397 at commit 8059cd2.

This patch merges cleanly.

SparkQA · 2014-09-16T21:41:10Z

QA tests have finished for PR 2397 at commit 8059cd2.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan)

liancheng · 2014-09-17T07:12:30Z

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

+  extends LeafNode with Command {
+
+  override protected[sql] lazy val sideEffectResult = {
+    sqlContext.catalog.registerTable(None, tableName,  sqlContext.executePlan(plan).analyzed)


(Probably my final comment on this PR :) )

As described in PR #2382, we shouldn't store analyzed logical plan when registering tables any more (see here).

To prevent duplicated code, I'd suggest to import SQLContext._ so that we can leverage the implicit conversion from LogicalPlan to SchemaRDD, and then simply do this:

sqlContext.executePlan(plan).logical.registerTempTable(tableName)

Thank you for your comment. It is a good idea to import sqlContext._. But we can simplify as below code if we import it. Please comment on it.

import sqlContext._ plan.registerTempTable(tableName) cacheTable(tableName)

Ah, yes you're right, we can use plan directly. And instead of importing sqlContext._, I'd import SQLContext._ in the import section at the begin of this file:

import org.apache.spark.sql.SQLContext._

It seems we cannot use the import org.apache.spark.sql.SQLContext._ at the beginning of file to use implicit. Because there is no object defined for SQLContext and implicits are only part of class SQLContext. We can only use the import on instance like import sqlContext._ Please correct me if I am wrong.

Sorry again, you're right, I mistook sqlContext._ for SparkContext._...

Updated the code. Please review

liancheng · 2014-09-17T07:14:04Z

LGTM except for the analyzed logical plan issue as mentioned in my last comment. Thanks for working on this!

SparkQA · 2014-09-18T06:59:24Z

QA tests have started for PR 2397 at commit a5f0beb.

This patch merges cleanly.

ravipesala · 2014-09-18T06:59:28Z

Updated as per comments. Please review.

SparkQA · 2014-09-18T07:47:56Z

QA tests have finished for PR 2397 at commit a5f0beb.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CacheTableAsSelectCommand(tableName: String, plan: LogicalPlan) extends Command
- case class CacheTableAsSelectCommand(tableName: String, logicalPlan: LogicalPlan)

liancheng · 2014-09-18T20:33:28Z

@ravipesala Thanks for working on this! @marmbrus I think this is ready to go :)

marmbrus · 2014-09-19T22:32:25Z

Thanks! I've merged this to master.

ravipesala added 17 commits September 11, 2014 15:53

Updated parser to support add cache table command

4e858d8

Updated parser to support add cache table command

13c8e27

Added comment

7459ce3

Changed style

6758f80

Updated parser to support add cache table command

b5276b2

Updated parser to support add cache table command

dc33895

Added comment

aaf5b59

Changed style

724b9db

Updated the code as per the comments by Admin in pull request.

e3265d0

Merge remote-tracking branch 'ravipesala/Add-Cache-table-as' into

bc0bffc

Add-Cache-table-as

Updated as per the comments by Admin

d8b37b2

Changed the style

8c9993c

Updated as per Admin comments

fb1759b

Changed style

394d5ca

Merge remote-tracking branch 'remotes/ravipesala/Add-Cache-table-as' …

c18aa38

…into SPARK-2594

This was referenced Sep 15, 2014

[SPARK-2594][SQL] Add CACHE TABLE <name> AS SELECT ... #2381

Closed

[SPARK-2594][SQL] Add CACHE TABLE <name> AS SELECT ... (Updated as per review comments) #2390

Closed

liancheng reviewed Sep 16, 2014
View reviewed changes

Code review comments by Admin are handled.

d6e469d

ravipesala changed the title ~~[SPARK-2594][SQL] Add CACHE TABLE <name> AS SELECT ...~~ [SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ... Sep 16, 2014

ravipesala changed the title ~~[SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ...~~ [SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ... Sep 16, 2014

Changed the behaviour from eager caching to lazy caching.

8059cd2

liancheng reviewed Sep 17, 2014
View reviewed changes

Simplified the code as per Admin comment.

a5f0beb

asfgit closed this in 5522151 Sep 19, 2014

[SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ... #2397

[SPARK-2594][SQL] Support CACHE TABLE <name> AS SELECT ... #2397

Uh oh!

Conversation

ravipesala commented Sep 15, 2014

Uh oh!

SparkQA commented Sep 15, 2014

Uh oh!

davies commented Sep 15, 2014

Uh oh!

SparkQA commented Sep 15, 2014

Uh oh!

SparkQA commented Sep 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liancheng commented Sep 16, 2014

Uh oh!

SparkQA commented Sep 16, 2014

Uh oh!

ravipesala commented Sep 16, 2014

Uh oh!

SparkQA commented Sep 16, 2014

Uh oh!

marmbrus commented Sep 16, 2014

Uh oh!

ravipesala commented Sep 16, 2014

Uh oh!

SparkQA commented Sep 16, 2014

Uh oh!

SparkQA commented Sep 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!

liancheng commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 18, 2014

Uh oh!

ravipesala commented Sep 18, 2014

Uh oh!

SparkQA commented Sep 18, 2014

Uh oh!

liancheng commented Sep 18, 2014

Uh oh!

marmbrus commented Sep 19, 2014

Uh oh!

Uh oh!