[SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. #13638

ueshin · 2016-06-13T07:16:17Z

What changes were proposed in this pull request?

DataFrame with plan overriding sameResult but not using canonicalized plan to compare can't cacheTable.

The example is like:

    val localRelation = Seq(1, 2, 3).toDF()
    localRelation.createOrReplaceTempView("localRelation")

    spark.catalog.cacheTable("localRelation")
    assert(
      localRelation.queryExecution.withCachedData.collect {
        case i: InMemoryRelation => i
      }.size == 1)

and this will fail as:

ArrayBuffer() had size 0 instead of expected size 1

The reason is that when do spark.catalog.cacheTable("localRelation"), CacheManager tries to cache for the plan wrapped by SubqueryAlias but when planning for the DataFrame localRelation, CacheManager tries to find cached table for the not-wrapped plan because the plan for DataFrame localRelation is not wrapped.
Some plans like LocalRelation, LogicalRDD, etc. override sameResult method, but not use canonicalized plan to compare so the CacheManager can't detect the plans are the same.

This pr modifies them to use canonicalized plan when override sameResult method.

How was this patch tested?

Added a test to check if DataFrame with plan overriding sameResult but not using canonicalized plan to compare can cacheTable.

…not using canonicalized plan to compare can cacheTable.

SparkQA · 2016-06-13T08:49:45Z

Test build #60389 has finished for PR 13638 at commit 52641f4.

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

cloud-fan · 2016-06-13T20:09:27Z

this looks reasonable to me, cc @marmbrus is there any corner cases?

marmbrus · 2016-06-13T21:55:29Z

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

    cachedData.foreach {
-      case data if data.plan.collect { case p if p.sameResult(plan) => p }.nonEmpty =>
+      case data if data.plan.collect { case p if p.sameResult(canonicalized) => p }.nonEmpty =>


I think this is redundant, sameResult already compares the canonicalized plan.

I don't think so.
For example, if the cached plan is LocalRelation (which is canonicalized) and the plan argument is SubqueryAlias(LocalRelation) (which is not canonicalized), it will fail to find the same-result plan.

the implementation of sameResult:

def sameResult(plan: PlanType): Boolean = { val left = this.canonicalized val right = plan.canonicalized ......

Seems we do canonicalize plans?

Some plans like LocalRelation, LogicalRDD, etc. override the sameResult and they don't use canonicalized plan to compare.

For example, LocalRelation overrides like as follows:

override def sameResult(plan: LogicalPlan): Boolean = plan match { case LocalRelation(otherOutput, otherData) => otherOutput.map(_.dataType) == output.map(_.dataType) && otherData == data case _ => false }

If the plan is SubqueryAlias(LocalRelation), the method returns false.

Should we just fix those implementations?

Hmm, yes, we should for now, but we can't force to use canonicalized plan and it is difficult to check all implementations so I thought CacheManager should canonicalize plans.

I guess I would prefer to fix in in sameResult if possible so that all uses of that function will benefit.

I see, I'll fix them, too.

marmbrus · 2016-06-13T21:55:55Z

Seems reasonable. Is this a regression from 1.6?

cloud-fan · 2016-06-13T23:39:55Z

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

@@ -87,7 +87,7 @@ private[sql] class CacheManager extends Logging {
      query: Dataset[_],
      tableName: Option[String] = None,
      storageLevel: StorageLevel = MEMORY_AND_DISK): Unit = writeLock {
-    val planToCache = query.queryExecution.analyzed
+    val planToCache = query.queryExecution.analyzed.canonicalized


do we still need these changes? LogicalPlan.canonicalized is a lazy val and we don't have performance penalty even we use un-canonicalized plan as key.

It seems we don't need them for now.
I'll revert the changes.

This reverts commit 52641f4.

ueshin · 2016-06-14T01:15:03Z

Updated pr title, description and a test name.

SparkQA · 2016-06-14T01:29:42Z

Test build #60451 has finished for PR 13638 at commit 11dc433.

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

ueshin · 2016-06-14T02:18:31Z

@marmbrus It seems this is not a regression from 1.6.
The test modified for 1.6 also fails with the same error.

SparkQA · 2016-06-14T02:53:39Z

Test build #60458 has finished for PR 13638 at commit 3d27607.

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

SparkQA · 2016-06-14T03:14:39Z

Test build #60459 has finished for PR 13638 at commit 0c9ec86.

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

cloud-fan · 2016-06-14T06:02:40Z

LGTM, @marmbrus should we backport it to 1.6?

marmbrus · 2016-06-14T17:51:41Z

Yeah, sounds reasonable. Merging to master, 2.0 and 1.6.

…verride sameResult. ## What changes were proposed in this pull request? `DataFrame` with plan overriding `sameResult` but not using canonicalized plan to compare can't cacheTable. The example is like: ``` val localRelation = Seq(1, 2, 3).toDF() localRelation.createOrReplaceTempView("localRelation") spark.catalog.cacheTable("localRelation") assert( localRelation.queryExecution.withCachedData.collect { case i: InMemoryRelation => i }.size == 1) ``` and this will fail as: ``` ArrayBuffer() had size 0 instead of expected size 1 ``` The reason is that when do `spark.catalog.cacheTable("localRelation")`, `CacheManager` tries to cache for the plan wrapped by `SubqueryAlias` but when planning for the DataFrame `localRelation`, `CacheManager` tries to find cached table for the not-wrapped plan because the plan for DataFrame `localRelation` is not wrapped. Some plans like `LocalRelation`, `LogicalRDD`, etc. override `sameResult` method, but not use canonicalized plan to compare so the `CacheManager` can't detect the plans are the same. This pr modifies them to use canonicalized plan when override `sameResult` method. ## How was this patch tested? Added a test to check if DataFrame with plan overriding sameResult but not using canonicalized plan to compare can cacheTable. Author: Takuya UESHIN <ueshin@happy-camper.st> Closes #13638 from ueshin/issues/SPARK-15915. (cherry picked from commit c5b7355) Signed-off-by: Michael Armbrust <michael@databricks.com>

marmbrus · 2016-06-14T17:53:33Z

Hmmm, does not apply cleanly to 1.6. @ueshin if you have time it might be nice to backport.

ueshin · 2016-06-14T18:38:31Z

@marmbrus Thank you for merging this.
Sure, I'll send a backport pr for branch-1.6.

ueshin · 2016-06-14T19:05:28Z

@marmbrus I sent a backport pr #13668.
But is branch-1.6 build broken now?
Looks working on #13630.

…n when override sameResult. ## What changes were proposed in this pull request? This pr is a backport of #13638 for `branch-1.6`. ## How was this patch tested? Added the same test as #13638 modified for `branch-1.6`. Author: Takuya UESHIN <ueshin@happy-camper.st> Closes #13668 from ueshin/issues/SPARK-15915_1.6.

…n when override sameResult. ## What changes were proposed in this pull request? This pr is a backport of apache#13638 for `branch-1.6`. ## How was this patch tested? Added the same test as apache#13638 modified for `branch-1.6`. Author: Takuya UESHIN <ueshin@happy-camper.st> Closes apache#13668 from ueshin/issues/SPARK-15915_1.6. (cherry picked from commit cffc080)

ueshin added 2 commits June 13, 2016 16:09

Add a test to check if DataFrame with plan overriding sameResult but …

9eb4c25

…not using canonicalized plan to compare can cacheTable.

Modify CacheManager to use canonicalized plan for planToCache.

52641f4

marmbrus reviewed Jun 13, 2016
View reviewed changes

Use canonicalized plan to compare.

11dc433

cloud-fan reviewed Jun 13, 2016
View reviewed changes

ueshin added 2 commits June 14, 2016 10:05

Revert "Modify CacheManager to use canonicalized plan for planToCache."

3d27607

This reverts commit 52641f4.

Update a test name.

0c9ec86

ueshin changed the title ~~[SPARK-15915][SQL] CacheManager should use canonicalized plan for planToCache.~~ [SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. Jun 14, 2016

asfgit closed this in c5b7355 Jun 14, 2016

ueshin mentioned this pull request Jun 14, 2016

[SPARK-15915][SQL] Logical plans should use subqueries eliminated plan when override sameResult. #13668

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. #13638

[SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. #13638

ueshin commented Jun 13, 2016 •

edited

SparkQA commented Jun 13, 2016

cloud-fan commented Jun 13, 2016

marmbrus Jun 13, 2016

ueshin Jun 13, 2016

cloud-fan Jun 13, 2016

ueshin Jun 13, 2016 •

edited

marmbrus Jun 13, 2016

ueshin Jun 13, 2016

marmbrus Jun 13, 2016

ueshin Jun 13, 2016

marmbrus commented Jun 13, 2016

cloud-fan Jun 13, 2016

ueshin Jun 14, 2016

ueshin commented Jun 14, 2016

SparkQA commented Jun 14, 2016

ueshin commented Jun 14, 2016

SparkQA commented Jun 14, 2016

SparkQA commented Jun 14, 2016

cloud-fan commented Jun 14, 2016 •

edited

marmbrus commented Jun 14, 2016

marmbrus commented Jun 14, 2016

ueshin commented Jun 14, 2016

ueshin commented Jun 14, 2016

[SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. #13638

[SPARK-15915][SQL] Logical plans should use canonicalized plan when override sameResult. #13638

Conversation

ueshin commented Jun 13, 2016 • edited

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Jun 13, 2016

cloud-fan commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin Jun 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmbrus commented Jun 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueshin commented Jun 14, 2016

SparkQA commented Jun 14, 2016

ueshin commented Jun 14, 2016

SparkQA commented Jun 14, 2016

SparkQA commented Jun 14, 2016

cloud-fan commented Jun 14, 2016 • edited

marmbrus commented Jun 14, 2016

marmbrus commented Jun 14, 2016

ueshin commented Jun 14, 2016

ueshin commented Jun 14, 2016

ueshin commented Jun 13, 2016 •

edited

ueshin Jun 13, 2016 •

edited

cloud-fan commented Jun 14, 2016 •

edited