cloud-fan commented on code in PR #58462:
URL: https://github.com/apache/spark/pull/58462#discussion_r3910472199
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -510,13 +525,26 @@ private[sql] object CatalogV2Util {
timeTravelSpec: Option[TimeTravelSpec] = None,
writePrivilegesString: Option[String] = None,
options: CaseInsensitiveStringMap = CaseInsensitiveStringMap.empty()):
Table = {
+ val stateOptions = extractTableStateOptions(catalog, options)
+ getTableWithStateOptions(
+ catalog, ident, stateOptions, timeTravelSpec, writePrivilegesString)
+ }
+
+ /**
+ * Loads a table using table-state options already projected by the caller.
+ */
+ def getTableWithStateOptions(
+ catalog: CatalogPlugin,
+ ident: Identifier,
+ stateOptions: CaseInsensitiveStringMap,
+ timeTravelSpec: Option[TimeTravelSpec] = None,
+ writePrivilegesString: Option[String] = None): Table = {
val timeTravel: TimeTravel = timeTravelSpec match {
case Some(v: AsOfVersion) => new TimeTravel.AsOfVersion(v.version)
case Some(ts: AsOfTimestamp) => new
TimeTravel.AsOfTimestamp(ts.timestamp)
case None => null
}
val context = new TableContext(timeTravel,
parseWritePrivileges(writePrivilegesString))
- val stateOptions = extractTableStateOptions(catalog, options)
catalog.asTableCatalog.loadTable(ident, context, stateOptions)
Review Comment:
**Blocking (P1):** This passes the same `stateOptions` instance that Spark
retains in `TableCacheKey`/`currentTables` to arbitrary catalog code. Although
direct mutators throw, `CaseInsensitiveStringMap` exposes mutable live
`keySet`, `values`, and `entrySet` views, so a connector can change the map
during or after loading and corrupt the hash/key Spark relies on. That can
produce cache misses or let a table loaded for one branch be reused for
another. Please keep Spark's projected map private and pass `loadTable` a
defensive copy.
**Recommended change:** Keep the caller-projected map as Spark-private state
and construct a defensive CaseInsensitiveStringMap copy immediately before
invoking TableCatalog.loadTable; add a focused mutation regression test.
**Why this works:** External catalog code receives an equivalent copy, so
mutation through a collection view cannot alter the map used for table pinning,
refresh deduplication, or shared-cache lookup.
**Scope:** CatalogV2Util.getTableWithStateOptions and a focused Catalyst
catalog test.
**Compatibility:** Catalogs receive the same option contents and
case-insensitive lookup behavior, public signatures remain unchanged, and
callers still perform only one projection.
**Risks:** Adds one small map copy at the external catalog boundary.
**Constraints:** Retain the original projected map for every Spark-owned
cache and deduplication key. Do not expose that original instance to the
connector callback.
**Success:** A catalog that mutates the supplied options through a
collection view cannot change Spark's retained state key, and subsequent
same-state and different-state lookups remain correct.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -604,18 +632,22 @@ private[sql] object CatalogV2Util {
loadTable(catalog, ident).map(DataSourceV2Relation.create(_,
Some(catalog), Some(ident)))
}
- def lookupCachedRelation(
+ /**
+ * Looks up a cached relation using table-state options already projected by
the caller.
+ * Reusing the projection keeps table pinning and shared-cache lookup on the
same state key.
+ */
+ def lookupCachedRelationWithStateOptions(
cache: RelationCache,
catalog: CatalogPlugin,
ident: Identifier,
table: Table,
- options: CaseInsensitiveStringMap,
+ stateOptions: CaseInsensitiveStringMap,
conf: SQLConf): Option[DataSourceV2Relation] = {
cache.lookup(
catalog,
ident,
Some(table.id),
- extractTableStateOptions(catalog, options),
+ stateOptions,
Review Comment:
**Non-blocking (P2):** The existing tests verify the projected contents and
final cache/load results, but none counts `tableStateOptionKeys` calls.
Replacing this already-projected path with a full-options wrapper would
therefore keep every assertion green while reintroducing the repeated
projection this PR is meant to remove. Please use a resettable counting catalog
in `PlanResolutionSuite` and `DataSourceV2OptionSuite` to assert one projection
per resolution or refresh boundary alongside the existing semantic checks.
--
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]
