laskoviymishka commented on code in PR #2366:
URL: https://github.com/apache/iceberg-go/pull/2366#discussion_r4421997218
##########
catalog/rest/rest.go:
##########
@@ -1343,7 +1346,11 @@ func (r *Catalog) tableFromResponse(
}
fsF = refresher.loadFS
} else {
- fsF = iceio.LoadFSFunc(config, loc)
+ if r.ioRegistry == nil {
Review Comment:
Could this branch move into a `LoadFSFunc` sibling of `loadRegistryFS`?
Right now nil-means-global is decided in two places, and a nil `*Registry`
panics as soon as it loads
([registry.go](https://github.com/apache/iceberg-go/blob/3742b99cd72d5424ef202528b9f217078e4cf2e2/io/registry.go#L174-L183)).
With one helper, the next call site can't get that check wrong.
##########
catalog/rest/scan_planning.go:
##########
@@ -266,7 +266,7 @@ func (r *Catalog) PlanFiles(ctx context.Context, req
table.ScanPlanningRequest)
result := table.ScanPlanningResult{
Tasks: tasks,
- IO: planIOFromCredentials(completed.StorageCredentials,
req.MetadataLocation, r.planIOBaseProps(req)),
+ IO:
planIOFromCredentialsWithRegistry(completed.StorageCredentials,
req.MetadataLocation, r.planIOBaseProps(req), r.ioRegistry),
Review Comment:
Could we add a test that goes through `PlanFiles` on a catalog built with
`WithIORegistry`? As I read it, both tests that set the option call
`tableFromResponse`, and `TestIsolatedIORegistryForPlanCredentials`
([io_registry_test.go](https://github.com/apache/iceberg-go/blob/3742b99cd72d5424ef202528b9f217078e4cf2e2/catalog/rest/io_registry_test.go#L56-L77))
calls `planIOFromCredentialsWithRegistry` directly. So passing `nil` here
would still pass, and plan IO would quietly fall back to the global registry.
##########
catalog/rest/scan_planning.go:
##########
@@ -472,11 +472,16 @@ func remoteScanTasks(envelopes []ScanTasks, req
table.ScanPlanningRequest) ([]ta
// so vended keys win on collision. With none vended it returns a nil PlanIO,
// and the scan falls back to the table's own FileIO.
func planIOFromCredentials(creds []StorageCredential, location string,
baseProps iceberg.Properties) table.PlanIO {
+ return planIOFromCredentialsWithRegistry(creds, location, baseProps,
nil)
Review Comment:
Do we still need the registry-less wrapper? `PlanFiles` now calls the
`WithRegistry` variant, so this one is only used by the four tests in
[scan_planning_test.go](https://github.com/apache/iceberg-go/blob/3742b99cd72d5424ef202528b9f217078e4cf2e2/catalog/rest/scan_planning_test.go#L1935).
Could we keep a single `planIOFromCredentials(creds, location, baseProps,
registry)` and have those tests pass `nil`? `loadRegistryFS` already treats nil
as the global registry, so behavior stays the same.
--
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]
