viirya commented on code in PR #59448:
URL: https://github.com/apache/spark/pull/59448#discussion_r4267657580
##########
sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala:
##########
@@ -1952,6 +1976,53 @@ class CachedTableSuite extends SharedSparkSession
}
}
+ test("cache lookups by table name do not rewrite cached plans") {
+ val t = "testcat.tbl"
+ withTable(t) {
+ sql(s"CREATE TABLE $t (id int, data string) USING foo")
+ sql(s"INSERT INTO $t VALUES (1, 'a'), (2, 'b')")
+
+ val originalStrategies = spark.experimental.extraStrategies
+ try {
+ // Set the strategy before caching, since caching plans the query in a
cloned session.
+ spark.experimental.extraStrategies = CopyCountingNodeStrategy +:
originalStrategies
+
+ // Stack several nodes above the aliased table relation. Rewriting the
subtree under every
+ // node that the by-name checks visit would copy each of these nodes
once for itself and
+ // once for every node above it, which is quadratic in the plan size.
+ val numNodes = 10
+ val plan = (1 to
numNodes).foldLeft(spark.table(t).queryExecution.analyzed) {
+ (child, _) => CopyCountingNode(child)
+ }
+ cacheManager.cacheQuery(spark, plan, tableName = None, MEMORY_AND_DISK)
+ assert(cacheManager.numCachedEntries == 1)
+ CopyCountingNode.numCopies.set(0)
Review Comment:
Good point. Added the precondition check before resetting the counter.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -259,7 +259,7 @@ class CacheManager extends Logging with
AdaptiveSparkPlanHelper {
resolver: Resolver,
includeTimeTravel: Boolean): Boolean = {
- EliminateSubqueryAliases(plan) match {
+ stripRootSubqueryAliases(plan) match {
Review Comment:
Done. The resolver is now resolved once per call, and the unused `SQLConf`
overload is removed.
##########
sql/core/src/test/scala/org/apache/spark/sql/CachedTableSuite.scala:
##########
@@ -1952,6 +1976,53 @@ class CachedTableSuite extends SharedSparkSession
}
}
+ test("cache lookups by table name do not rewrite cached plans") {
Review Comment:
Added a test that caches `spark.table(t).as("x")` (two root aliases) and
checks that `lookupCachedTable` finds the relation and that non-cascading
uncache by name removes the entry. It fails if the helper strips only one alias
level. The single-alias positive cases are already covered by the SPARK-54004 /
SPARK-54022 tests.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -477,7 +489,7 @@ class CacheManager extends Logging with
AdaptiveSparkPlanHelper {
name: Seq[String],
resolver: Resolver): Seq[LogicalPlan] = {
cachedData.flatMap { cd =>
- val plan = EliminateSubqueryAliases(cd.plan)
+ val plan = stripRootSubqueryAliases(cd.plan)
Review Comment:
After this change the per-entry work here is a short walk down the root
alias chain, and there is usually at most one match, so I don't expect a
measurable difference. I'd prefer to keep this PR focused, since it will be
backported to 4.1 - 4.3.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -278,6 +278,18 @@ class CacheManager extends Logging with
AdaptiveSparkPlanHelper {
}
}
+ /**
+ * Strips the chain of [[SubqueryAlias]] nodes at the root of the given
plan. Callers that only
+ * inspect the root node use this instead of `EliminateSubqueryAliases`,
which rewrites the whole
+ * tree. That matters because [[isMatchedTableOrView]] is evaluated on every
node of every cached
Review Comment:
Updated the scaladoc.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala:
##########
@@ -278,6 +278,18 @@ class CacheManager extends Logging with
AdaptiveSparkPlanHelper {
}
}
+ /**
+ * Strips the chain of [[SubqueryAlias]] nodes at the root of the given
plan. Callers that only
+ * inspect the root node use this instead of `EliminateSubqueryAliases`,
which rewrites the whole
+ * tree. That matters because [[isMatchedTableOrView]] is evaluated on every
node of every cached
+ * plan, where a full rewrite per node makes cache lookups by name quadratic
in plan size.
+ */
+ @tailrec
+ private def stripRootSubqueryAliases(plan: LogicalPlan): LogicalPlan = plan
match {
Review Comment:
Since those callers are one-shot and not in hot loops, I'd keep the helper
private here to keep the fix small for backporting. It can move to catalyst if
a hot caller shows up later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
