itsbilal commented on code in PR #2075:
URL: https://github.com/apache/iceberg-go/pull/2075#discussion_r4159298437
##########
catalog/rest/rest.go:
##########
@@ -1302,6 +1303,7 @@ func (r *Catalog) tableFromResponse(
r,
table.WithMetricsReporter(reporter),
table.WithScanPlanningIOProperties(scanPlanningConfig),
+ table.WithSavedConfig(config),
Review Comment:
This is baking in assumptions about how the config map was initially created
in an externally-created table, and I'd argue it's a bad idea because it
violates a pretty simple contract: `WithSavedConfig` persists the config that
will be used the next time a new table instance is recreated off of one. We
actually don't have a guarantee that `RefreshTableCredentials` only gets tables
that were created by `tableFromResponse` - the use-case I was going to use it
for is one where we manually create a table using `table.New`.
It's not too much for an AI, but the additional layering of props is
sounding too difficult to keep track of (+ it's not laid out in any
comment/interface in the code), so I'd prefer to keep the code as-is unless I
get a human response advising otherwise. The only downside of the current
approach is a slightly larger map that's being saved inside the table struct.
--
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]
