[SPARK-27747][SQL] add a logical plan link in the physical plan #24626

cloud-fan · 2019-05-16T14:43:46Z

What changes were proposed in this pull request?

It's pretty useful if we can convert a physical plan back to a logical plan, e.g., in #24389

This PR introduces a new feature to TreeNode, which allows TreeNode to carry some extra information via a mutable map, and keep the information when it's copied.

The planner leverages this feature to put the logical plan into the physical plan.

How was this patch tested?

a test suite that runs all TPCDS queries and checks that some common physical plans contain the corresponding logical plans.

cloud-fan · 2019-05-16T14:44:50Z

cc @pengbo this is a more general approach than #24389

also cc @maryannxue @bogdanrdc @gatorsmile

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala

@@ -620,4 +620,8 @@ class TreeNodeSuite extends SparkFunSuite with SQLHelper {
      assert(planString.startsWith("Truncated plan of"))
    }
  }
+
+  test("tags will be carried over after copy") {
+


SparkQA · 2019-05-16T16:54:29Z

Test build #105456 has finished for PR 24626 at commit d57ecc1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class TreeNodeTagName(name: String)

gatorsmile · 2019-05-16T17:47:08Z

sql/core/src/test/scala/org/apache/spark/sql/execution/LogicalPlanTagInSparkPlanSuite.scala

+
+      // The exchange related nodes are created after the planning, they don't have corresponding
+      // logical plan.
+      case _: ShuffleExchangeExec | _: BroadcastExchangeExec | _: ReusedExchangeExec =>


Add ReusedSubqueryExec?

added below

viirya · 2019-05-17T01:25:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

@@ -280,9 +297,15 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
        rule.applyOrElse(this, identity[BaseType])


Here after applying the rule, we need to carry over the tags, if needed, too.

Since carrying the tags in both if and else branches, so maybe:

val newNode = if (this fastEquals afterRuleOnChildren) { CurrentOrigin.withOrigin(origin) { rule.applyOrElse(this, identity[BaseType]) } } else { CurrentOrigin.withOrigin(origin) { rule.applyOrElse(afterRuleOnChildren, identity[BaseType]) } } // carrying over the tags to newNode...

viirya · 2019-05-17T01:30:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

+  /**
+   * A mutable map for holding auxiliary information of this tree node. It will be carried over
+   * when this node is copied via `makeCopy`. If a user copies the tree node via other ways like the
+   * `copy` method, it's his responsibility to carry over the tags.


Maybe add something like The tags will be kept after transforming, if transformed to the same type, otherwise, will be dropped.

viirya · 2019-05-17T01:36:25Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/QueryPlan.scala

@@ -271,6 +271,8 @@ abstract class QueryPlan[PlanType <: QueryPlan[PlanType]] extends TreeNode[PlanT
 }

 object QueryPlan extends PredicateHelper {
+  val LOGICAL_PLAN_TAG_NAME = TreeNodeTagName("logical_plan")


Can we add some comment? For example, After query plans go through SQL planner, the planner will attach the optimized logical plan to the generated physical plan under this tag name.

viirya · 2019-05-17T06:20:56Z

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

+        case ReturnAnswer(rootPlan) => rootPlan
+        case _ => plan
+      }
+      p.tags += QueryPlan.LOGICAL_PLAN_TAG_NAME -> logicalPlan


Only the most top physical plan has the link to logical plan, right?

This pattern is called stackable traits in Scala. When super.plan is called, its recursive plan call points to this overridden version.

You can also see from the tests that, not only the root node has the logical plan link.

SparkQA · 2019-05-17T07:05:02Z

Test build #105482 has finished for PR 24626 at commit 9f377d7.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2019-05-17T09:38:14Z

retest this please.

SparkQA · 2019-05-17T12:40:31Z

Test build #105485 has finished for PR 24626 at commit 9f377d7.

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

SparkQA · 2019-05-17T17:55:44Z

Test build #105497 has finished for PR 24626 at commit dafad5c.

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

pengbo · 2019-05-18T08:14:31Z

After changing to always carry over the tags in transform, it seems that the unit test has to be adjusted as well. I have created a PR cloud-fan#13 if that helps.
Can you please inform the motivation of that change?
Thanks

maryannxue · 2019-05-20T05:43:30Z

LGTM, pending Jenkins.

SparkQA · 2019-05-20T07:05:01Z

Test build #105548 has finished for PR 24626 at commit b033f55.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-05-20T07:05:02Z

Test build #105549 has finished for PR 24626 at commit b380f1d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

pengbo · 2019-05-20T14:26:14Z

retest this please.

cloud-fan · 2019-05-20T14:28:14Z

retest this please

SparkQA · 2019-05-20T17:39:31Z

Test build #105572 has finished for PR 24626 at commit b380f1d.

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

gatorsmile · 2019-05-20T20:38:02Z

LGTM

Thanks! Merged to master.

It's pretty useful if we can convert a physical plan back to a logical plan, e.g., in apache#24389 This PR introduces a new feature to `TreeNode`, which allows `TreeNode` to carry some extra information via a mutable map, and keep the information when it's copied. The planner leverages this feature to put the logical plan into the physical plan. a test suite that runs all TPCDS queries and checks that some common physical plans contain the corresponding logical plans. Closes apache#24626 from cloud-fan/link. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Peng Bo <bo.peng1019@gmail.com> Signed-off-by: gatorsmile <gatorsmile@gmail.com>

add a logical plan link in the physical plan

d57ecc1

gatorsmile reviewed May 16, 2019

View reviewed changes

viirya reviewed May 17, 2019

View reviewed changes

address comments

9f377d7

viirya reviewed May 17, 2019

View reviewed changes

pengbo mentioned this pull request May 17, 2019

[SPARK-27482][SQL][WEBUI] Show BroadcastHashJoinExec numOutputRows statistics info on SparkSQL UI page #24389

Closed

always carry over the tags in transform

dafad5c

pengbo mentioned this pull request May 18, 2019

Fix UT failure for https://github.com/apache/spark/pull/24626 cloud-fan/spark#13

Merged

pengbo and others added 2 commits May 20, 2019 13:33

fix ut pb (#13)

b033f55

update

b380f1d

cloud-fan force-pushed the link branch from 842b63d to b380f1d Compare May 20, 2019 05:40

gatorsmile closed this in 0e6601a May 20, 2019

pengbo mentioned this pull request May 21, 2019

[SPARK-27482][SQL][WEBUI] Show estimated BroadcastHashJoinExec numOutputRows statistics info on SparkSQL UI page #24666

Closed

		@@ -280,9 +297,15 @@ abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product {
		rule.applyOrElse(this, identity[BaseType])

[SPARK-27747][SQL] add a logical plan link in the physical plan #24626

[SPARK-27747][SQL] add a logical plan link in the physical plan #24626

Uh oh!

Conversation

cloud-fan commented May 16, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented May 16, 2019

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

SparkQA commented May 16, 2019

Uh oh!

gatorsmile May 16, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 17, 2019

Choose a reason for hiding this comment

Uh oh!

viirya May 17, 2019

Choose a reason for hiding this comment

Uh oh!

viirya May 17, 2019

Choose a reason for hiding this comment

Uh oh!

viirya May 17, 2019

Choose a reason for hiding this comment

Uh oh!

viirya May 17, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan May 17, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 17, 2019

Uh oh!

viirya commented May 17, 2019

Uh oh!

SparkQA commented May 17, 2019

Uh oh!

SparkQA commented May 17, 2019

Uh oh!

pengbo commented May 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryannxue commented May 20, 2019

Uh oh!

SparkQA commented May 20, 2019

Uh oh!

SparkQA commented May 20, 2019

Uh oh!

pengbo commented May 20, 2019

Uh oh!

cloud-fan commented May 20, 2019

Uh oh!

SparkQA commented May 20, 2019

Uh oh!

gatorsmile commented May 20, 2019

Uh oh!

Uh oh!

pengbo commented May 18, 2019 •

edited

Loading