iremcaginyurtturk commented on PR #2001:
URL: https://github.com/apache/iceberg-go/pull/2001#issuecomment-5948839910
Thanks @zeroshade — all four blocking items are addressed in 3d49411.
**`warehouse` bypass (glue.go:335).** You're right, and the lazy probe
missed it: with `warehouse` set, `getDefaultWarehouseLocation` resolves
`<warehouse>/<db>.db/<tbl>`, staging succeeds, and a federated database gets an
`EXTERNAL_TABLE` outside managed storage. Implemented your suggestion:
`CreateTable` now fetches the database once up front (`lookupDatabase`, still
tolerating `AccessDenied`/`NotFound` as non-federated) and decides federation
for both default and explicit-location creates. The result is threaded into
`CreateStagedTable` through a closure over `namespacePropsFromDatabase`,
factored out of `LoadNamespaceProperties`, so the S3 Tables path no longer
makes a second `GetDatabase` — every create is a single round-trip, and the
mocks moved from `.Times(2)` back to `.Once()`. Added
`TestGlueCreateTableS3TablesWarehouseSet`, which sets `props{"warehouse": ...}`
on a federated database and asserts the minimal-entry (`format=ICEBERG`, no
`StorageDescriptor`) allocate rat
her than a warehouse-located create.
**`AlreadyExists` on allocate (glue.go:425).** Applied your suggestion.
`TestGlueCreateTableS3TablesAllocateAlreadyExists` asserts
`ErrorIs(catalog.ErrTableAlreadyExists)` and that `DeleteTable` is never called
— we must not roll back a table this call did not create.
**Credential-boundary regression test (glue.go:456).** Added, using the seam
you pointed at. A `recordingMemFS` helper registers a scheme whose FileIO
factory records `utils.GetAwsConfig(ctx)`, and `assertRecordedConfig` requires
every resolution to be the catalog's `c.awsCfg` by pointer. Three tests cover
the generic `CreateTable`, `commitS3TablesTable` (including the `fs.Remove`
cleanup after a failed `UpdateTable`), and `CommitTable`. Each fails if its
wrap is removed.
**`RenameTable` (glue.go:1083).** I took the conservative option rather than
relying on unconfirmed service behaviour: rename is now refused for
`isS3TablesFederatedTable(fromGlueTable)`, before any write, with
`TestGlueRenameTableS3TablesRejected` asserting no `CreateTable`/`DeleteTable`.
Happy to switch to proving it in the gated live test instead if you'd prefer
rename to be supported.
**Nit (glue_test.go:2922).** Added the missing `AssertExpectations`, and
`TestGlueCreateTableS3TablesPassesCatalogID` now pins `catalogId` on every Glue
call of the two-phase path (`GetDatabase`, allocate `CreateTable`, `GetTable`,
`UpdateTable`) via `MatchedBy` on `CatalogId`.
**On resolving the `CreateTableOpt`s once** — partially addressed, and I'd
like your read. Evaluations are down from three to two: the federated path no
longer runs initial staging before the S3 Tables commit staging. The remaining
two are the location pre-check in `CreateTable` and the parse inside
`CreateStagedTable`. Collapsing to one means `CreateStagedTable` accepting a
pre-resolved `CreateTableCfg` instead of `opts`, and that loop is pre-existing
shared code (`catalog/internal/utils.go`, 3398683) used by sql/rest/hive/hadoop
as well. I'd rather not change that signature inside an S3 Tables feature PR —
happy to do it as a follow-up if you want it closed now.
`go build ./...`, `go vet`, `gofmt`, `golangci-lint` and the full `go test
./...` are clean. The branch also carries a merge of main, since
`TestGlueCreateTableAlreadyExists` landed there and needed a `GetDatabase` mock
now that `CreateTable` consults the database.
--
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]
