yyanyy commented on code in PR #58462:
URL: https://github.com/apache/spark/pull/58462#discussion_r3929063618
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala:
##########
@@ -510,14 +525,31 @@ 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)
+ // Callers may retain stateOptions as a cache key.
CaseInsensitiveStringMap exposes mutable
+ // collection views, so do not let catalog code mutate the retained
instance.
+ val catalogStateOptions =
+ new CaseInsensitiveStringMap(stateOptions.asCaseSensitiveMap())
Review Comment:
Thanks, good catch. I first switched the defensive copy to `new
CaseInsensitiveStringMap(stateOptions)` and added duplicate-case coverage. That
preserves the effective normalized value, but revealed another regression:
iterating `stateOptions` exposes lowercased keys, so rebuilding from it loses
the original spellings that `asCaseSensitiveMap()` should retain. In short,
copying from `asCaseSensitiveMap()` preserves casing but can change the
effective duplicate-case value, while copying from `stateOptions` preserves the
value but loses casing.
Rather than introduce special copy machinery to reconstruct both
representations, I decided to fix the underlying mutability gap
for`CaseInsensitiveStringMap`, so that we can safely pass the original
`stateOptions` instance without copying, since the change for that seems to not
be a lot.
--
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]
