viirya commented on PR #56928:
URL: https://github.com/apache/spark/pull/56928#issuecomment-4899101337
Thanks, I dug into this. I couldn't reproduce the leak as described, but I
agree the underlying design is fragile and worth changing.
What I found when I tried to reproduce it:
- Marking is an in-place `setTagValue` on the `LogicalRelation`, and tags do
propagate across `clone()` via `copyTagsFrom` — so your read of the mechanism
is right.
- But two implementation details happen to block the specific scenario.
First, each `sql("... FROM t")` re-resolves a fresh `LogicalRelation` from the
catalog, so a tag set by one query never reaches the next query's node (I
confirmed a feature-ON `ORDER BY` query followed by a feature-OFF `JOIN` over
the same table leaves the join's scans untagged). Second, when `t` is cached,
the relation is already replaced by `InMemoryRelation` by the time this rule
runs, so step 2 never actually tags the cached plan — I checked the cache-held
logical plan (`cacheBuilder.logicalPlan`) after a markable feature-ON query and
its `LogicalRelation` carries no tag.
So the leak doesn't happen today, but only because of that chain of
coincidences, not by design — which is exactly your point about mutating
logical plans. Rather than a full physical-plan redesign, would it be
acceptable to make the marking copy-based so the rule never mutates a node it
was handed? i.e. mark a `lr.copy()` instead of `lr` itself (`.copy()` preserves
the `exprId`s, so it's semantics-preserving). That restores the "an optimizer
rule doesn't mutate shared input nodes" invariant at essentially no cost and
guards against this breaking if those coincidences ever change. Happy to go the
`DisableUnnecessaryBucketedScan`-style physical route instead if you'd prefer
the stronger guarantee.
--
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]
