mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-11-09 21:38:19 +01:00
ba9e74d8a7604ebb215230c00a0eee7dbbb3139e
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 qzhelloandChris Lu
|
69c84801e4 |
fix(s3tables/iceberg): make metadata spec-compliant and accept real-world manifest names (#9703)
* fix(s3tables/iceberg): make metadata spec-compliant and accept real-world manifest names
Two related issues prevent SeaweedFS S3 Tables from interoperating with
strict Iceberg clients (Java/Spark/Flink/Trino):
1. iceberg-go v0.5.0 serializes empty TableMetadata state by dropping
keys via `omitempty` on optional pointer/slice fields. The Iceberg
table spec, however, requires `current-snapshot-id`, `snapshots`,
`snapshot-log`, `metadata-log`, and `refs` to be present even when
empty (`current-snapshot-id` must be -1 for a table with no
snapshots). Java's TableMetadataParser uses JsonUtil.getLong on
`current-snapshot-id` and throws "Cannot parse missing long
current-snapshot-id" against responses produced by this server.
2. The Iceberg layout validator only accepts manifest filenames that
match Iceberg's internal naming (`{uuid}-m{n}.avro`,
`snap-{n}-{n}-{uuid}.avro`). Real writers — notably Flink's sink —
emit manifests like
`{flink-job-id}-{checkpoint}-{operator-id}-{n}.avro`, which the
validator rejects with 403, breaking INSERT commits.
Fixes:
* Add ensureMetadataSpecCompliance helper that backfills the five
spec-required empty-state fields when iceberg-go omits them or emits
explicit JSON null. Apply it on every code path that writes
v*.metadata.json to S3 or returns metadata to clients
(handlers_table create-table, handlers_commit, commit_helpers
create-on-commit, plus MarshalJSON on LoadTableResult and
CommitTableResponse). Real values from non-empty tables are never
overwritten.
* Add catch-all regex entries to metadataFilePatterns accepting any
*.avro / *.metadata.json filename composed of [A-Za-z0-9._-]. The
Iceberg spec does not mandate filename format; the strict patterns
remain for documentation. Metadata-directory subdirectory rejection
and the data-file path validation are unchanged.
No upstream dependencies are forked: iceberg-go stays at v0.5.0 and
go.mod is untouched. The compliance layer can be removed once upstream
emits spec-compliant output.
Tests (all pass under `go test -race`):
- metadata_compliance_test.go: 5 cases covering missing fields,
preserved real values, explicit null, invalid JSON, empty input.
- iceberg_layout_test.go: 3 groups (16 subtests) covering real-world
manifest names from Flink/Spark/Iceberg, security boundary
(subdirectories, bad extensions), and data-file regression.
* fix(s3tables/iceberg): preserve metadata key order and keep config field stable
Two small follow-ups on the spec-compliance fix:
* ensureMetadataSpecCompliance now splices missing keys in at the byte
level just before the closing brace, so iceberg-go's struct-declared
key order survives the backfill. The previous unmarshal/remarshal
through map[string]json.RawMessage silently alphabetized every key in
the document, which is spec-legal but breaks byte-equality fixtures
and any downstream hashing of the persisted metadata. The slower
remarshal path is kept for the rare explicit-null replacement case.
* LoadTableResult.MarshalJSON now serializes Config without omitempty,
matching the struct field tag. The custom marshaler had silently
flipped the tag to ,omitempty, which made the "config" key disappear
from the response whenever s3Endpoint was unset (since
buildFileIOConfig returned an empty but non-nil Properties map).
Tests:
- PreservesOriginalKeyOrder pins the byte-level output against
iceberg-go's emitted shape; would have caught the alphabetization
regression.
- EmptyObjectBackfilled covers the {} -> sentinels-only case (no
leading comma).
- AllPresentReturnsSameBytes confirms the no-op path returns input
bytes unchanged, with whitespace intact.
- iceberg_layout_test pins the catch-all $ anchor: metadata/file.avro.txt
must still be rejected.
* fix(s3tables/iceberg): guard ensureMetadataSpecCompliance against top-level null
json.Unmarshal of a JSON `null` literal succeeds but leaves the map nil.
The current byte-append path no-ops gracefully on this input, but the
slow remarshal path would panic with "assignment to entry in nil map"
if the input ever combined `null` with the explicit-null detection. Add
an explicit nil-map short-circuit so the safety property is obvious
from the source, and a test that pins the contract.
* test(s3tables/iceberg): assert full byte equality in AllPresentReturnsSameBytes
The prefix check only caught a missing "{\n " opener, so the test
would have passed even if the function silently reordered keys or
collapsed whitespace later in the document. Switch to a full string
comparison so any future regression in the no-op path is loud.
---------
Co-authored-by: Chris Lu <chris.lu@gmail.com>
|
||
|
Chris Lu
|
9554e259dd |
fix(iceberg): route catalog clients to the right bucket and vend S3 endpoint (#9109)
* fix(iceberg): route catalog clients to the right bucket and vend S3 endpoint DuckDB ATTACH 's3://<bucket>/' AS cat (TYPE 'ICEBERG', ...) was failing with "schema does not exist" because GET /v1/config ignored the warehouse query parameter and returned no overrides.prefix, so subsequent requests fell through to the hard-coded "warehouse" default bucket instead of the one the client attached. LoadTable also returned an empty config, forcing clients to discover the S3 endpoint out-of-band and producing 403s on direct iceberg_scan calls. - handleConfig now echoes overrides.prefix = bucket and defaults.warehouse when ?warehouse=s3://<bucket>/ is supplied. - getBucketFromPrefix honors a warehouse query parameter as a fallback for clients that skip the /v1/config handshake. - LoadTable responses advertise s3.endpoint and s3.path-style-access so clients can reach data files without separate configuration. Refs #9103 * address review feedback on iceberg S3 endpoint vending - deriveS3AdvertisedEndpoint is now a method on S3Options; honors externalUrl / S3_EXTERNAL_URL, switches to https when -s3.key.file is set, uses the https port when configured, and brackets IPv6 literals via util.JoinHostPort. - handleCreateTable returns s.buildFileIOConfig() in both its staged and final LoadTableResult branches so create and load flows see the same FileIO hints. - Add unit test coverage for the endpoint derivation scenarios. * address CI and review feedback for #9109 - DuckDB integration test now runs under its own newOAuthTestEnv (with a valid IAM config) so the OAuth2 client_credentials flow DuckDB requires actually works; the shared env has no registered credentials, which was the cause of the CI failure. Helper createTableWithToken was added to create tables via Bearer auth. - Tighten TestIssue9103_LoadTableDoesNotVendS3FileIOCredentials to also assert s3.path-style-access = "true", so a partial regression where the endpoint is vended but path-style is dropped still fails. - deriveS3AdvertisedEndpoint now logs a startup hint when it infers the host from os.Hostname because the bind IP is a wildcard, pointing operators at -s3.externalUrl / S3_EXTERNAL_URL for reverse-proxy deployments where the inferred name is not externally reachable. - handleConfig has a comment explaining that any sub-path in the warehouse URL is dropped because catalog routing is bucket-scoped. * fix(iceberg): make advertised S3 endpoint strictly opt-in; add region The wildcard-bind fallback to os.Hostname() in deriveS3AdvertisedEndpoint was hijacking correctly-configured clients: on the CI runner it produced http://runnervmrc6n4:<port>, which Spark (running in Docker) could not resolve, so Spark iceberg tests began failing after the endpoint started being vended in LoadTable responses. Change the rule so advertising is opt-in and never guesses a host that might not be routable: - -s3.externalUrl / S3_EXTERNAL_URL wins (covers reverse-proxy). - Otherwise, only an explicit, non-wildcard -s3.bindIp is used. - Wildcard / empty bind returns "" so no FileIO endpoint is vended and existing clients keep using their own configuration. buildFileIOConfig additionally vends s3.region (defaulting to the same value baked into table bucket ARNs) whenever it vends an endpoint, so DuckDB's attach does not fail with "No region was provided via the vended credentials" when the operator has opted in. The DuckDB issue-9103 integration test runs under an env with a wildcard bind, so it explicitly sets AWS_REGION in the docker run to pick up the same default. The HTTP-level LoadTable-vending test was dropped because its expectation is now conditional and already covered by unit tests in iceberg_issue_9103_test.go. |
||
|
Chris Lu
|
2818251dd5 |
fix(iceberg): clean stale data before creating a table (#9074) (#9077)
* fix(iceberg): clean stale data before creating a table CREATE TABLE AS from Trino fails against the SeaweedFS Iceberg REST catalog with "Cannot create a table on a non-empty location". The catalog assigns every new table the deterministic <ns>/<table> path, and Trino's pre-write check rejects the CTAS whenever leftover objects live there — typically files from a prior DROP that did not purge the data, or an earlier aborted CTAS. Make the catalog the authority for table existence: before writing any metadata, look up the table in the S3 Tables catalog. If it is already registered, return the existing definition (idempotent create). If it is not registered, any objects still sitting at the target location are stale, so purge them before proceeding. Live tables are never touched — the cleanup path is guarded by the catalog lookup. Fixes #9074 * test(trino): regression for create/drop/recreate table (#9074) Exercises the exact sequence from the reported bug: CREATE TABLE without an explicit location, INSERT, DROP, then CREATE again with the same name, followed by a CTAS on top. Previously the recreate failed with "Cannot create a table on a non-empty location" because stale data files from the dropped table lingered at the deterministic <schema>/<table> path. The test also asserts the recreated table does not see the dropped data and that a drop/recreate CTAS cycle works. * fix(iceberg): purge dropped table location on DROP TABLE Recreate-at-same-location was failing with "Cannot create a table on a non-empty location" because DROP TABLE only removed the catalog entry and left data files behind. Trino's pre-write emptiness check then rejected the subsequent CREATE. Look up the table's storage location before deleting the catalog entry, and after a successful DeleteTable, purge the filer subtree at that location. The lookup uses the catalog — the authoritative owner of the name→location mapping — so cleanup is gated on a live table having existed; failed lookups skip cleanup and leave storage alone. Also pin the regression test to an explicit, fixed location so the recreate genuinely targets the same path the drop was supposed to free. * refactor(iceberg): use errors.As in isNoSuchTableError * fix(iceberg): drop create-preflight cleanup; harden cleanup path - Remove the destructive cleanupStaleTableLocation call from the CreateTable preflight. Storage cleanup now lives exclusively on the DROP path, so CreateTable has no side effects on storage beyond the new table's own metadata write. - Validate tablePath in cleanupStaleTableLocation by rejecting empty, ".", "..", or backslash-bearing segments before joining with the bucket prefix, so a crafted location cannot escape the bucket subtree. path.Clean would silently collapse traversal, so segments are checked raw. * test(trino): defer table drops in recreate test for failure-safe cleanup |
||
|
Chris Lu
|
761ec7da00 |
fix(iceberg): use dot separator for namespace paths instead of unit separator (#8960)
* fix(iceberg): use dot separator for namespace paths instead of unit separator The Iceberg REST Catalog handler was using \x1F (unit separator) to join multi-level namespaces when constructing S3 location and filer paths. The S3 Tables storage layer uses "." (dot) as the namespace separator, causing tables created via the Iceberg REST API to point to different paths than where S3 Tables actually stores them. Fixes #8959 * fix(iceberg): use dot separator in log messages for readable namespace output * fix(iceberg): use path.Join for S3 location path segments Use path.Join to construct the namespace/table path segments in fallback S3 locations for robustness and consistency with handleCreateTable. * test(iceberg): add multi-level namespace integration tests for Spark and Trino Add regression tests for #8959 that create a two-level namespace (e.g. "analytics.daily"), create a table under it, insert data, and query it back. This exercises the dot-separated namespace path construction and verifies that Spark/Trino can actually read the data at the S3 location returned by the Iceberg REST API. * fix(test): enable nested namespace in Trino Iceberg catalog config Trino requires `iceberg.rest-catalog.nested-namespace-enabled=true` to support multi-level namespaces. Without this, CREATE SCHEMA with a dotted name fails with "Nested namespace is not enabled for this catalog". * fix(test): parse Trino COUNT(*) output as integer instead of substring match Avoids false matches from strings.Contains(output, "3") by parsing the actual numeric result with strconv.Atoi and asserting equality. * fix(test): use separate Trino config for nested namespace test The nested-namespace-enabled=true setting in Trino changes how SHOW SCHEMAS works, causing "Internal error" for all tests sharing that catalog config. Move the flag to a dedicated config used only by TestTrinoMultiLevelNamespace. * fix(iceberg): support parent query parameter in ListNamespaces for nested namespaces Add handling for the Iceberg REST spec's `parent` query parameter in handleListNamespaces. When Trino has nested-namespace-enabled=true, it sends `GET /v1/namespaces?parent=<ns>` to list child namespaces. The parent value is decoded from the Iceberg unit separator format and converted to a dot-separated prefix for the S3 Tables layer. Also simplify TestTrinoMultiLevelNamespace to focus on namespace operations (create, list, show tables) rather than data operations, since Trino's REST catalog has a non-empty location check that conflicts with server-side metadata creation. * fix(test): expand Trino multi-level namespace test and merge config helpers - Expand TestTrinoMultiLevelNamespace to create a table with explicit location, insert rows, query them back, and verify the S3 file path contains the dot-separated namespace (not \x1F). This ensures the original #8959 bug would be caught by the Trino integration test. - Merge writeTrinoConfig and writeTrinoNestedNamespaceConfig into a single parameterized function using functional options. |
||
|
Chris Lu
|
d6825ffce2 |
Iceberg: implement stage-create finalize flow (phase 1) (#8279)
* iceberg: implement stage-create and create-on-commit finalize * iceberg: add create validation error typing and stage-create integration test * tests: merge stage-create integration check into catalog suite * tests: cover stage-create finalize lifecycle in catalog integration * iceberg: persist and cleanup stage-create markers * iceberg: add stage-create rollout flag and marker pruning * docs: add stage-create support design and rollout plan * docs: drop stage-create design draft from PR * iceberg: use conservative 72h stage-marker retention * iceberg: address review comments on create-on-commit and tests * iceberg: keep stage-create metadata out of table location * refactor(iceberg): split iceberg.go into focused files |