aokolnychyi commented on code in PR #57582:
URL: https://github.com/apache/spark/pull/57582#discussion_r3713607509
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -61,11 +61,10 @@ class RelationResolution(
with LookupCatalog
with SQLConfHelper {
- type CacheKey = (Seq[String], Option[TimeTravelSpec])
-
val v1SessionCatalog = catalogManager.v1SessionCatalog
- private def relationCache: mutable.Map[CacheKey, LogicalPlan] =
AnalysisContext.get.relationCache
+ private def relationCache: mutable.Map[RelationCacheKey, LogicalPlan] =
+ AnalysisContext.get.relationCache
Review Comment:
Question: given that this is a private method, do we need the return type
annotation or can we drop it to stay within one line?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/RelationResolution.scala:
##########
@@ -556,8 +548,10 @@ class RelationResolution(
private def toCacheKey(
catalog: CatalogPlugin,
ident: Identifier,
- timeTravelSpec: Option[TimeTravelSpec] = None): CacheKey = {
- ((catalog.name +: ident.namespace :+ ident.name).toImmutableArraySeq,
timeTravelSpec)
+ timeTravelSpec: Option[TimeTravelSpec],
+ options: CaseInsensitiveStringMap): RelationCacheKey = {
+ RelationCacheKey(
+ (catalog.name +: ident.namespace :+ ident.name).toImmutableArraySeq,
timeTravelSpec, options)
Review Comment:
Can we define a helper var for name parts?
```
val nameParts = catalog.name +: ident.namespace :+ ident.name
RelationCacheKey(nameParts.toImmutableArraySeq, timeTravelSpec, options)
or
val nameParts = (catalog.name +: ident.namespace :+
ident.name).toImmutableArraySeq
RelationCacheKey(nameParts, timeTravelSpec, options)
or
RelationCacheKey(
nameParts = (catalog.name +: ident.namespace :+
ident.name).toImmutableArraySeq,
timeTravelSpec,
options)
```
##########
sql/core/src/main/scala/org/apache/spark/sql/classic/DataFrameWriter.scala:
##########
@@ -176,7 +176,10 @@ final class DataFrameWriter[T] private[sql](ds:
Dataset[T]) extends sql.DataFram
val catalog = CatalogV2Util.getTableProviderCatalog(
supportsExtract, catalogManager, dsOptions)
- (catalog.loadTable(ident), Some(catalog), Some(ident))
+ // Forward the user options to the catalog, mirroring the read
path in
+ // DataSourceV2Utils.loadV2Source.
+ (CatalogV2Util.getTable(catalog, ident, options = dsOptions),
Review Comment:
This is a bit hard to read, can we use the same trick with a helper method
from the branch below?
```
val t = CatalogV2Util.getTable(catalog, ident, options = dsOptions)
(t, Some(catalog), Some(ident))
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2TableRefreshUtil.scala:
##########
@@ -81,19 +82,20 @@ private[sql] object V2TableRefreshUtil extends
SQLConfHelper with Logging {
plan: LogicalPlan,
versionedOnly: Boolean,
schemaValidationMode: SchemaValidationMode): LogicalPlan = {
- val currentTables = mutable.HashMap.empty[(TableCatalog, Identifier),
Table]
+ val currentTables =
+ mutable.HashMap.empty[(TableCatalog, Identifier,
CaseInsensitiveStringMap), Table]
Review Comment:
Can we define a type alias so that this fits into one line?
--
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]
