peter-toth commented on code in PR #57582:
URL: https://github.com/apache/spark/pull/57582#discussion_r3695746271
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -280,7 +281,8 @@ class RelationResolution(
catalog,
ident,
finalTimeTravelSpec,
- Option(writePrivileges))
+ Option(writePrivileges),
+ finalOptions)
Review Comment:
**Finding 9** (Non-blocking, and I think it belongs in the follow-up
@aokolnychyi parked rather than in this PR): separate from the
state-vs-cosmetic split, the lookup behind both new guards can fail to find a
matching entry even when one exists.
`CacheManager.lookupCachedTable` matches cache entries by *name only* and
returns just one of them -- the head of the matches
(`CacheManager.scala:438-449`) -- and since `cachedData` is prepended on each
cache (`CacheManager.scala:165`), that is the most recently cached entry. The
new guards then compare *that one candidate's* options against the current
read's (`RelationResolution.scala:320`, `V2TableRefreshUtil.scala:93`) and
reuse its `Table` only when they are equal; nothing goes back to check the
other name matches. A table has more than one entry because `cachedData` is
keyed by the normalized plan and the plan carries `options`
(`CacheManager.scala:145`), so `spark.table(t).cache()` and
`spark.read.option("split-size", "5").table(t).cache()` are two entries. A read
with `split-size=5` therefore gets the pin only if that entry happens to be the
newer of the two.
Reproduced on `3377cd99` -- same query both times, only the caching order
swapped:
```scala
// cache both spark.table(t) and spark.read.option("split-size",
"5").table(t)
val df = spark.read.option("split-size", "5").table(t).filter("id > 0")
df.queryExecution.analyzed
inMemoryCatalog.resetLoadTableCalls()
df.collect()
inMemoryCatalog.loadTableCalls.map(_._2.get("split-size")).filter(_ != null)
// split-size entry cached last -> List() pinned to the cached table, no
reload
// option-free entry cached last -> List(5) matching entry never offered,
reloads latest
```
So today whether a query keeps its session-pinned version is decided by
which of that table's cache entries was written most recently. It can also cost
an outright cache hit, once the table has moved on from what was cached --
which is the situation the pin exists for. Cache
`spark.read.option("split-size", "5").table(t)`, let the table get a new
version, then re-run that exact query with a newer cache entry for `t` present:
the lookup offers the newer entry, the guard rejects it, the relation is loaded
fresh at the new version, and `sameResult` then fails against the v=old entry
that would have matched. With the caching order swapped, the same query is
served from the cache.
`TableCatalog#tableStateOptions` would shrink the key but not close this:
with two entries differing only in state options, the lookup still offers just
one of them. The multi-reference case makes it concrete -- one query reading
`t` on `branch=b1` and `t` on `branch=b2`, with both branches cached, resolves
each reference against the same single newest candidate (`tryResolvePersistent`
runs per reference and there is no memo across them), so exactly one of the two
gets pinned to its cached snapshot and which one flips with the caching order.
Closing it means making the shared-cache lookup options-aware instead of
filtering after the fact -- `RelationCache.lookup` returns
`Option[LogicalPlan]` today, so it would need to expose all name matches (or
take the options) and let the caller pick the one that matches.
On your split-size point: you're right, and it corrects something I asserted
earlier in this thread. I argued the old `cached.copy(options = finalOptions)`
branch "buys nothing" once the options differ, on the grounds that the
resulting plan can't match the cached entry's fingerprint anyway. That holds
for the `useCachedData` hit, but the branch was also providing version pinning,
which is independent of it -- so for a purely cosmetic option the guard does
give up something real.
--
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]
