[SPARK-43376][SQL] Improve reuse subquery with table cache #41046

ulysses-you · 2023-05-04T11:15:08Z

What changes were proposed in this pull request?

AQE can not reuse subquery if it is pushed into InMemoryTableScan. There are two issues:

ReuseAdaptiveSubquery can not support reuse subquery if two subquery have the same exprId
InMemoryTableScan miss apply ReuseAdaptiveSubquery when wrap TableCacheQueryStageExec

For example:

Seq(1).toDF("c1").cache().createOrReplaceTempView("t1")
Seq(2).toDF("c2").createOrReplaceTempView("t2")
spark.sql("SELECT * FROM t1 WHERE c1 < (SELECT c2 FROM t2)")

There are two subquery#27 but have no ReusedSubquery

AdaptiveSparkPlan isFinalPlan=true
+- == Final Plan ==
   *(1) Filter (c1#14 < Subquery subquery#27, [id=#20])
   :  +- Subquery subquery#27, [id=#20]
   :     +- AdaptiveSparkPlan isFinalPlan=true
   :        +- LocalTableScan [c2#25]
   +- TableCacheQueryStage 0
      +- InMemoryTableScan [c1#14], [(c1#14 < Subquery subquery#27, [id=#20])]
            :- InMemoryRelation [c1#14], StorageLevel(disk, memory, deserialized, 1 replicas)
            :     +- LocalTableScan [c1#14]
            +- Subquery subquery#27, [id=#20]
               +- AdaptiveSparkPlan isFinalPlan=true
                  +- LocalTableScan [c2#25]

Why are the changes needed?

Improve the coverage of reuse subquery.

Note that, it is not a real perf issue because the subquery has been already reused (the same Java object). This pr just makes the plan clearer about subquery reuse.

Does this PR introduce any user-facing change?

no

How was this patch tested?

add test

cloud-fan · 2023-05-04T17:44:24Z

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/ReuseAdaptiveSubquery.scala

+        reuseMap.get(sub.plan.canonicalized).map { subquery =>
+          sub.withNewPlan(ReusedSubqueryExec(subquery))
+        }.getOrElse {
+          reuseMap.put(sub.plan.canonicalized, sub.plan)


do we have atomicity concerns?

The behavior should be same with getOrElseUpdate. I changed put to putIfAbsent to fully equivalent with getOrElseUpdate that the first put will win if there is race condition.

Is this correct? 2 concurent .get()s for 2 semantic equal plans can return Nones and then none of the plans will be reused (wrapped into ReusedSubqueryExec).

The behavior follows the getOrElseUpdate. It does not affect perf and I'm not sure how many possible can that happen, so I do not lock it.

I think getOrElseUpdate() on TrieMap is atomic, a .get() followed by a .put() is not.

I mean, getOrElseUpdate() is basically a .get() followed by a .putIfAbsent() as you do. But you should then check if putIfAbsent() was successful ~~the map contains the key you put into it~~ and depending on that do the reuse.

thank you @peter-toth , I see it now

Thanks! 99f810c looks ok to me.

ulysses-you · 2023-05-05T02:02:17Z

sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala

-        }
+        assert(subqueryIds.size == 2, "Missing or unexpected SubqueryExec in the plan")
+        assert(reusedSubqueryIds.size == 5,
+          "Missing or unexpected reused ReusedSubqueryExec in the plan")


The reuse subquery bahavior is now same with non-AQE.

cloud-fan · 2023-05-05T05:05:19Z

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/ReuseAdaptiveSubquery.scala

-        } else {
+        // `InsertAdaptiveSparkPlan` compiles subquery for each exprId, then the java object
+        // is always `eq` if two subqueries have same exprId.
+        // Check if the subquery can be reused manually instead of call `getOrElseUpdate`.


let's make the comment more explicit

// The subquery can be already reused (the same Java object) due to filter pushdown // of table cache. If it happens, we just need to wrap the current subquery with `ReusedSubqueryExec` // and no need to update te `reuseMap`.

cloud-fan · 2023-05-05T05:06:19Z

Can we mention it in the PR description that this is not a real perf issue, but just make the plan clearer about subquery reuse?

cloud-fan · 2023-05-06T03:52:47Z

thanks, merging to master!

ulysses-you · 2023-05-06T06:31:27Z

thank you @cloud-fan @peter-toth @dongjoon-hyun

### What changes were proposed in this pull request? AQE can not reuse subquery if it is pushed into `InMemoryTableScan`. There are two issues: - `ReuseAdaptiveSubquery` can not support reuse subquery if two subquery have the same exprId - `InMemoryTableScan` miss apply `ReuseAdaptiveSubquery` when wrap `TableCacheQueryStageExec` For example: ``` Seq(1).toDF("c1").cache().createOrReplaceTempView("t1") Seq(2).toDF("c2").createOrReplaceTempView("t2") spark.sql("SELECT * FROM t1 WHERE c1 < (SELECT c2 FROM t2)") ``` There are two `subquery#27` but have no `ReusedSubquery` ``` AdaptiveSparkPlan isFinalPlan=true +- == Final Plan == *(1) Filter (c1#14 < Subquery subquery#27, [id=apache#20]) : +- Subquery subquery#27, [id=apache#20] : +- AdaptiveSparkPlan isFinalPlan=true : +- LocalTableScan [c2#25] +- TableCacheQueryStage 0 +- InMemoryTableScan [c1#14], [(c1#14 < Subquery subquery#27, [id=apache#20])] :- InMemoryRelation [c1#14], StorageLevel(disk, memory, deserialized, 1 replicas) : +- LocalTableScan [c1#14] +- Subquery subquery#27, [id=apache#20] +- AdaptiveSparkPlan isFinalPlan=true +- LocalTableScan [c2#25] ``` ### Why are the changes needed? Improve the coverage of reuse subquery. Note that, it is not a real perf issue because the subquery has been already reused (the same Java object). This pr just makes the plan clearer about subquery reuse. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? add test Closes apache#41046 from ulysses-you/aqe-subquery. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2023-06-02T15:13:35Z

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/ReuseAdaptiveSubquery.scala

+        // The subquery can be already reused (the same Java object) due to filter pushdown
+        // of table cache. If it happens, we just need to wrap the current subquery with
+        // `ReusedSubqueryExec` and no need to update the `reuseMap`.
+        reuseMap.get(sub.plan.canonicalized).map { subquery =>


I just noticed that this rule is not idempotent now. If we run it twice, then all subqueries will become ReusedSubqueryExec because all subqueries are in the reuseMap.

… subquery ### What changes were proposed in this pull request? #41046 make `ReuseAdaptiveSubquery` become not idempotent. This pr reverts the change in `ReuseAdaptiveSubquery`. To solve the same instance issue when planning and reusing subquery in AQE, this pr makes `subqueryMap` hold the executed plan in subquery. Then in `PlanAdaptiveSubqueries`, each logical subquery plan can build their own instance of physical subquery. ### Why are the changes needed? To improve reuse subquery in AQE. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? Pass CI Closes #41454 from ulysses-you/SPARK-43376-2. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

… subquery ### What changes were proposed in this pull request? apache#41046 make `ReuseAdaptiveSubquery` become not idempotent. This pr reverts the change in `ReuseAdaptiveSubquery`. To solve the same instance issue when planning and reusing subquery in AQE, this pr makes `subqueryMap` hold the executed plan in subquery. Then in `PlanAdaptiveSubqueries`, each logical subquery plan can build their own instance of physical subquery. ### Why are the changes needed? To improve reuse subquery in AQE. ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? Pass CI Closes apache#41454 from ulysses-you/SPARK-43376-2. Authored-by: ulysses-you <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

Improve reuse subquery with table cache

54f3bc3

github-actions bot added the SQL label May 4, 2023

cloud-fan reviewed May 4, 2023

View reviewed changes

fix

97ddbe7

ulysses-you commented May 5, 2023

View reviewed changes

cloud-fan reviewed May 5, 2023

View reviewed changes

cloud-fan approved these changes May 5, 2023

View reviewed changes

ulysses-you added 2 commits May 5, 2023 13:34

address comment

8a41b7a

address comment

99f810c

dongjoon-hyun approved these changes May 5, 2023

View reviewed changes

cloud-fan closed this in b54a94b May 6, 2023

ulysses-you deleted the aqe-subquery branch May 6, 2023 06:31

cloud-fan reviewed Jun 2, 2023

View reviewed changes

ulysses-you mentioned this pull request Jun 5, 2023

[SPARK-43376][SQL][FOLLOWUP] lazy construct subquery to improve reuse subquery #41454

Closed

[SPARK-43376][SQL] Improve reuse subquery with table cache #41046

[SPARK-43376][SQL] Improve reuse subquery with table cache #41046

Uh oh!

Conversation

ulysses-you commented May 4, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth May 5, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

peter-toth May 5, 2023 • 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!

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!

cloud-fan commented May 5, 2023

Uh oh!

cloud-fan commented May 6, 2023

Uh oh!

ulysses-you commented May 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ulysses-you commented May 4, 2023 •

edited

Loading

peter-toth May 5, 2023 •

edited

Loading

peter-toth May 5, 2023 •

edited

Loading