[SPARK-58389][SQL][FOLLOWUP] Pin DSv2 table instances by state options during analysis - #57799
[SPARK-58389][SQL][FOLLOWUP] Pin DSv2 table instances by state options during analysis#57799yyanyy wants to merge 4 commits into
Conversation
Add a catalog capability for identifying table-state options and a query-scoped table cache keyed by those options. Preserve complete option matching for finalized relations and shared CACHE TABLE reuse while pinning one concrete Table per state within an analysis context.
fe24fdc to
e2cf3a0
Compare
| getOrLoadRelation(ref) | ||
| val useSharedRelationCache = | ||
| ref.context.isInstanceOf[V2TableReference.TemporaryViewContext] | ||
| getOrLoadRelation(ref, useSharedRelationCache) |
There was a problem hiding this comment.
The other case here for now is TransactionContext that shouldn't use shared relation cache to avoid replacing the table earlier than the appropriate transaction check to decide if a cache reuse is safe (txn.registerScans?)
e2cf3a0 to
b42dda9
Compare
uros-b
left a comment
There was a problem hiding this comment.
@szehon-ho Could you PTAL at these changes?
| * Option key matching is case-insensitive. Option values remain case-sensitive. Parsed Spark time | ||
| * travel is handled independently and must not be included in the returned set. | ||
| * <p> | ||
| * Catalogs that do not implement this capability are handled conservatively: Spark treats every |
There was a problem hiding this comment.
Well, I think we historically treated it the opposite way. Specifically, we didn't support any read options that defined the state. Therefore, we shared the table instance across all of them. We can't silently switch this behavior in 4.3. In fact, I think if the catalog doesn't implement this method, it should mean it does NOT support any state defining options. Otherwise, we will UNPIN versions within one query UNLESS the catalog implements the new mix-in interface that it may not know about. For example, we will break Iceberg in 4.3 unless we also change the connector code.
There was a problem hiding this comment.
What about this description?
/**
* A catalog capability for identifying options that affect table state such as branch or tag.
* <p>
* Spark may need to resolve the same table more than once while analyzing or refreshing a query.
* By default, Spark reuses a single table instance for all references to the same identifier,
* treating options as unable to select a different table state. A catalog can implement this
* interface to declare which raw options may cause the catalog to select a different table state,
* such as a branch, tag, snapshot, or version. Spark then reuses one table instance only for
* references whose table-state options match, while preserving every reference's complete option
* map for scan planning.
* <p>
* Catalogs that do not implement this interface are assumed to have no state-affecting options, so
* all references to the same identifier continue to share a single table instance regardless of
* their read or write options.
* <p>
* Option key matching is case-insensitive. Option values remain case-sensitive. State that Spark
* parses and handles independently, such as time travel, must not be included in the returned set.
*
* @since 4.3.0
*/
@Evolving
public interface SupportsTableStateOptions extends CatalogPlugin {
/**
* Returns the raw option keys that may affect the table state selected by {@code loadTable}. An
* empty set is equivalent to not implementing this interface: no option affects table state.
*
* @return a non-null set of case-insensitive option keys
*/
Set<String> tableStateOptionKeys();
}
There was a problem hiding this comment.
Had some offline discussion on this and post here for posterity: making the default behavior to consider all options as table state option could prevent accidental correctness issue when options can dictate different table versions, but this introduced 2 problems:
- this will make all DSV2 connectors not implementing the interface to start missing cache when options are stated differently, causing performance problem
- it will break the table pinning behavior within one query when the table was specified twice with different options, and this could potentially result in correctness concern
| * so that reusing a concrete table cannot silently combine states the catalog considers | ||
| * different. | ||
| */ | ||
| def tableStateOptions( |
There was a problem hiding this comment.
Can we call it extractTableStateOptions?
| }.toMap | ||
| new CaseInsensitiveStringMap(projected.asJava) | ||
| case _ => | ||
| options |
There was a problem hiding this comment.
I think this needs to be flipped to return an empty map to preserve the original 4.2 behavior.
| if finalTimeTravelSpec.isEmpty && writePrivileges == null && !u.isStreaming | ||
| if pinnedTable.isEmpty && finalTimeTravelSpec.isEmpty && | ||
| writePrivileges == null && !u.isStreaming | ||
| cached <- lookupSharedRelationCache(catalog, ident, t) |
There was a problem hiding this comment.
I think the logic here needs to be lookup using state keys only, if found, copy with the final options.
There was a problem hiding this comment.
Offline conversation for posterity: we were worried that taking all options into consideration for looking up shared relation cache could cause non-deterministic behavior when users specify queries against tables in one single spark session, where some queries have the exact option list matching shared relation cache, while some others don't; in this case user could observe different queries using different table versions and cause confusion. One query referring to the same table with different non-state-options also will be undeterministic and depend on the order of specifying such tables. (There's test cases covering scenarios mentioned above.) Also, we were not able to think of a concrete use case where the non-state options need to participate in the selection for the table from shared relation cache, as it was used for obtaining the table without a table reload; the actual reusing of the cache the data, do consider all options correctly.
| if finalTimeTravelSpec.isEmpty && writePrivileges == null && !u.isStreaming | ||
| if pinnedTable.isEmpty && finalTimeTravelSpec.isEmpty && | ||
| writePrivileges == null && !u.isStreaming | ||
| cached <- lookupSharedRelationCache(catalog, ident, t) |
There was a problem hiding this comment.
We have to push down the lookup into RelationCache with something like:
def lookup(
catalog: CatalogPlugin,
ident: Identifier,
tableId: Option[String],
stateOptions: CaseInsensitiveStringMap,
resolver: Resolver): Option[DataSourceV2Relation] = {
...
}
…ate-cache-20260804 # Conflicts: # sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogV2UtilSuite.scala
a0da62c to
86219d0
Compare
What changes were proposed in this pull request?
SPARK-58389 changed DSv2 relation caching to include all read options. This is required
to preserve each reference's complete option bag, but it also means references that differ
only in scan-specific options can independently call
loadTableand receive differentconcrete table versions within one query.
This PR separates relation reuse from table-state pinning:
relationCacheremains keyed by all read options and reuses a finalized relationonly when the complete option bags match.
tableCacheis keyed by catalog, identifier, time travel, and table-stateoptions. References with the same table-state key reuse one concrete
Table, while retainingtheir own complete options in their relations.
sharedRelationCacheand CacheManager reuse continue to require complete option matching.This PR adds the evolving
SupportsTableStateOptionscatalog capability so a catalog can declarewhich raw options may affect the table state selected by
loadTable, such as a branch, tag,snapshot, or version. Catalogs that do not implement the capability are handled conservatively:
all raw options are considered table-state-affecting.
While applying the same table-pinning model to cacheable
V2TableReferenceresolution, this PRalso fixes two existing gaps in
getOrLoadRelation:loadTable(identifier)without passing the options captured in the tablereference. A table-cache miss now uses the options-aware catalog API with the reference's
complete option bag.
sharedRelationCache. Temporary-view re-resolution now consultssharedRelationCachewhile establishing the initial table pin, allowing it to preserve aTablealready pinned through CacheManager.Transaction references still use the
Tableloaded through the transaction catalog and do notconsult
sharedRelationCache. Write targets remain non-cacheable and bypass the query-scoped readcaches.
The resulting resolution flow is:
relationCacheusing the full-option relation key.tableCacheusing the table-state key.tableCachehit, construct a relation from the pinnedTableand the current reference'scomplete options. Do not call
loadTableor consultsharedRelationCache.tableCachemiss, load the currentTablethrough the applicable options-aware path.sharedRelationCachelookup applies, reuse its relation only when table identity and alloptions match. The shared cached
Tableestablishes the initial pin on a match; otherwise, thenewly loaded
Tableestablishes it.TableintableCacheand the finalized relation inrelationCache.Execution-time table refresh uses the same table-state option projection to preserve this
first-resolution-wins behavior.
Why are the changes needed?
A catalog may accept both table-state options and scan-specific options. Using the complete option
bag for relation reuse is necessary, but using it as the only level of caching can cause references
in the same table-state domain to load different concrete table versions during one query.
The new
tableCachepins one concreteTableper state key without weakening full-option matchingfor finalized relations,
sharedRelationCache, or CacheManager.Forwarding options from
V2TableReferenceis also necessary because those options may select thetable state being reloaded. Temporary views additionally need the
sharedRelationCachebridge topreserve a CacheManager-pinned
Table.Different state domains, including different parsed time-travel specifications, continue to
resolve and pin independently.
Does this PR introduce any user-facing change?
Yes, for catalog implementors only.
This adds the evolving
SupportsTableStateOptionscatalog capability. There is no new SQL syntaxor configuration, and catalogs that do not implement it retain the conservative behavior where all
raw options are considered table-state-affecting.
The table-consistency fix otherwise addresses behavior introduced on the unreleased master branch
by SPARK-58389.
How was this patch tested?
Added regression coverage for table-state projection and pinning, conservative fallback,
sharedRelationCacheand CacheManager matching, nested analysis, execution refresh, andtemporary-view, transaction, and write-target
V2TableReferencebehavior.Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex