yyanyy commented on code in PR #58462:
URL: https://github.com/apache/spark/pull/58462#discussion_r3919424923
##########
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:
Thanks for calling this out! I noticed this earlier but initially decided
not to add a copy since it's not very easy to mutate `CaseInsensitiveStringMap`
without some explicit tricks, as the direct mutators throw as called out, and
also other callers passing maps to connectors do not do this defensive copy
either. However, this path indeed is more delicate since Spark retains the map
as a cache for deduplication key, so I agree it should be protected.
I now create a defensive copy immediately before `TableCatalog.loadTable`
and added a test.
I also considered limiting the copy only to callers that retain the map, but
that would require splitting or adding plumbing to the loading path merely to
avoid a small map copy, which kind of defeat half of the purpose of this PR; so
I decided to keep the uniform copy in the common helper for readability,
despite that there are code paths that would perform an unnecessary copy;
please let me know if this is fine, or a more limited copy approach is still
preferred; or I can even follow up to make `CaseInsensitiveStringMap` to be
truly immutable in all possible cases.
--
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]
