changgyoopark-db commented on code in PR #48088:
URL: https://github.com/apache/spark/pull/48088#discussion_r1757001234
##########
sql/connect/server/src/main/scala/org/apache/spark/sql/connect/service/SessionHolder.scala:
##########
@@ -58,7 +58,7 @@ case class SessionHolder(userId: String, sessionId: String,
session: SparkSessio
// Analyzing a large plan may be expensive, and it is not uncommon to build
the plan step-by-step
// with several analysis during the process. This cache aids the recursive
analysis process by
// memorizing `LogicalPlan`s which may be a sub-tree in a subsequent plan.
- private lazy val planCache: Option[Cache[proto.Relation, LogicalPlan]] = {
+ private lazy val planCache: Option[Cache[Long, LogicalPlan]] = {
Review Comment:
Thanks for pointing that out! Didn't think about it through. Unfortunately,
storing both Relation and LogicalPlan together isn't quite an option here as it
does neither reduce the memory usage nor eliminate the cost of Relation
comparison.
On the other hand, I guess supplying a manually generated plan ID when
constructing a protobuf message would be a footgun, so users wouldn't do it in
my opinion as long as the plan ID part is well-documented. The worst
consequence would be the very user getting incorrect results, but this won't
affect other users. Therefore, I think this PR is acceptable to some extent
(though disputable).
--
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]
