[GitHub] [spark] robreeves commented on a diff in pull request #40812: [SPARK-43157][SQL] Clone InMemoryRelation cached plan to prevent cloned plan from referencing same objects

["robreeves (via GitHub)" <gi...@apache.org>]

robreeves commented on code in PR #40812:
URL: https://github.com/apache/spark/pull/40812#discussion_r1180671671


##########
sql/core/src/main/scala/org/apache/spark/sql/execution/columnar/InMemoryRelation.scala:
##########
@@ -389,7 +389,7 @@ case class InMemoryRelation(
 
   @volatile var statsOfPlanToCache: Statistics = null
 
-  override def innerChildren: Seq[SparkPlan] = Seq(cachedPlan)
+  override val innerChildren: Seq[SparkPlan] = Seq(cachedPlan.clone())

Review Comment:
   Since plans are considered immutable and usually cloned, thread safety is not an issue. `cachedPlan` was not cloned so it was easy to get in a situation where multiple threads access the same plan objects. This solves that narrow issue. After this the `cachedPlan` is no more susceptible to concurrency issues than any other object in the plan. 
   
   I still think there is a wider thread safety issue with the mutable state in `TreeNode.tags`, but I need to investigate it more to propose a holistic solution. Consider the case where in the application code:
   
   1. User thread 1 calls `d.explain("formatted")` where `d` is a `Dataset` object. This will set the `TreeNode.tag` keys `QueryPlan.OP_ID_TAG` and `QueryPlan.CODEGEN_ID_TAG`. 
   2. At the same time in user thread 2 they call `d.filter($"foo" > 1)`. This will create a new dataset that reads the `TreeNode.tags` during the clone. 
   
   At first I considered making `TreeNode.tags` a thread-safe map, but that only solves the concurrency issue. At least for this issue, there would still be a correctness issue with other threads setting different operator ids for the same node. `TreeNode.tags` is used in so many other places that I'm not sure if there are other correctness issues.



-- 
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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org