diff --git a/test/s3tables/catalog_trino/trino_issue_9074_repro_test.go b/test/s3tables/catalog_trino/trino_issue_9074_repro_test.go new file mode 100644 index 000000000..afe5811f8 --- /dev/null +++ b/test/s3tables/catalog_trino/trino_issue_9074_repro_test.go @@ -0,0 +1,76 @@ +package catalog_trino + +import ( + "fmt" + "strings" + "testing" +) + +// TestTrinoIssue9074CTASFreshTable is a regression test for the follow-up +// report on https://github.com/seaweedfs/seaweedfs/issues/9074. +// +// The original fix purged stale data on DROP so a re-CREATE at the same +// path could succeed. The reporter then observed that even a fresh table +// name fails — they ran a CTAS without an explicit `location =` clause and +// hit the same "Cannot create a table on a non-empty location" error. +// +// Root cause: when (a) the user does not pass `location = ...`, and (b) the +// namespace has no Iceberg `location` property, Trino's REST catalog goes +// through TrinoRestCatalog.newCreateTableTransaction's eager branch +// (`tableBuilder.create().newTransaction()`) which immediately invokes the +// REST POST. Our handleCreateTable persists `/metadata/v1.metadata.json` +// as part of that call. Trino's IcebergMetadata.beginCreateTable then runs +// `fileSystem.listFiles(location).hasNext()` and trips on the metadata file +// we just wrote. +// +// Fix: handleGetNamespace / handleCreateNamespace now advertise a default +// `location` property of `s3:///` when the +// namespace doesn't already carry one. Trino's `defaultTableLocation` then +// returns `/-` (UUID added by the standard +// `iceberg.unique-table-location=true` connector setting), and the REST +// catalog takes the deferred `createTransaction` branch that does NOT call +// REST POST until commit-time. The empty-location check sees a genuinely +// empty UUID-suffixed path and passes; on commit, our metadata write lands +// alongside the data files Trino has already produced. +// +// This test exercises the exact pattern from the report: a fresh schema, a +// fresh table name, no `location =` clause, and a CTAS on top. +func TestTrinoIssue9074CTASFreshTable(t *testing.T) { + env := setupTrinoTest(t) + defer env.Cleanup(t) + + schemaName := "issue9074_" + randomString(6) + srcName := "src_" + randomString(6) + ctasName := "ctas_" + randomString(6) + srcQualified := fmt.Sprintf("iceberg.%s.%s", schemaName, srcName) + ctasQualified := fmt.Sprintf("iceberg.%s.%s", schemaName, ctasName) + + runTrinoSQL(t, env.trinoContainer, fmt.Sprintf("CREATE SCHEMA IF NOT EXISTS iceberg.%s", schemaName)) + defer runTrinoSQLAllowNamespaceNotEmpty(t, env.trinoContainer, fmt.Sprintf("DROP SCHEMA IF EXISTS iceberg.%s", schemaName)) + defer runTrinoSQL(t, env.trinoContainer, fmt.Sprintf("DROP TABLE IF EXISTS %s", srcQualified)) + defer runTrinoSQL(t, env.trinoContainer, fmt.Sprintf("DROP TABLE IF EXISTS %s", ctasQualified)) + + t.Logf(">>> CREATE source %s (no explicit location)", srcQualified) + runTrinoSQL(t, env.trinoContainer, fmt.Sprintf( + "CREATE TABLE %s (id INTEGER, name VARCHAR)", srcQualified)) + + runTrinoSQL(t, env.trinoContainer, fmt.Sprintf( + "INSERT INTO %s VALUES (1, 'a'), (2, 'b'), (3, 'c')", srcQualified)) + + ctasSQL := fmt.Sprintf("CREATE TABLE IF NOT EXISTS %s AS (SELECT * FROM %s)", ctasQualified, srcQualified) + t.Logf(">>> CTAS (no location override): %s", ctasSQL) + runTrinoSQL(t, env.trinoContainer, ctasSQL) + + countOutput := runTrinoSQL(t, env.trinoContainer, fmt.Sprintf("SELECT count(*) FROM %s", ctasQualified)) + got := mustParseCSVInt64(t, countOutput) + if got != 3 { + t.Fatalf("CTAS target: expected 3 rows, got %d (resulting table should not be empty)", got) + } + + out := runTrinoSQL(t, env.trinoContainer, fmt.Sprintf("SELECT name FROM %s ORDER BY id", ctasQualified)) + for _, want := range []string{"a", "b", "c"} { + if !strings.Contains(out, want) { + t.Fatalf("CTAS target missing row %q; got:\n%s", want, out) + } + } +} diff --git a/weed/s3api/iceberg/handlers_namespace.go b/weed/s3api/iceberg/handlers_namespace.go index abd9cca5b..832509573 100644 --- a/weed/s3api/iceberg/handlers_namespace.go +++ b/weed/s3api/iceberg/handlers_namespace.go @@ -144,7 +144,7 @@ func (s *Server) handleCreateNamespace(w http.ResponseWriter, r *http.Request) { result := CreateNamespaceResponse{ Namespace: Namespace(createResp.Namespace), - Properties: normalizeNamespaceProperties(createResp.Properties), + Properties: withDefaultNamespaceLocation(createResp.Properties, bucketName, createResp.Namespace), } writeJSON(w, http.StatusOK, result) } @@ -188,7 +188,7 @@ func (s *Server) handleGetNamespace(w http.ResponseWriter, r *http.Request) { result := GetNamespaceResponse{ Namespace: Namespace(getResp.Namespace), - Properties: normalizeNamespaceProperties(getResp.Properties), + Properties: withDefaultNamespaceLocation(getResp.Properties, bucketName, getResp.Namespace), } writeJSON(w, http.StatusOK, result) } diff --git a/weed/s3api/iceberg/utils.go b/weed/s3api/iceberg/utils.go index e349c122e..bc3a7be01 100644 --- a/weed/s3api/iceberg/utils.go +++ b/weed/s3api/iceberg/utils.go @@ -169,3 +169,38 @@ func normalizeNamespaceProperties(properties map[string]string) map[string]strin } return properties } + +// namespaceLocationProperty is the well-known Iceberg key used to advertise a +// default base path for tables created in a namespace. +const namespaceLocationProperty = "location" + +// defaultNamespaceLocation returns the s3 path under which tables in the +// namespace should be stored when the namespace itself does not carry an +// explicit "location" property. The flattened namespace path mirrors the on- +// disk layout used by handleCreateTable so a deferred client-side commit ends +// up at the same place a server-computed one would. +func defaultNamespaceLocation(bucket string, namespace []string) string { + if bucket == "" || len(namespace) == 0 { + return "" + } + return fmt.Sprintf("s3://%s/%s", bucket, flattenNamespacePath(namespace)) +} + +// withDefaultNamespaceLocation populates the Iceberg "location" property on a +// namespace response when the storage layer does not carry one. Trino's REST +// catalog falls back to an eager createTable code path when the namespace +// has no location, which races our metadata write against Trino's emptiness +// check and surfaces as a "Cannot create a table on a non-empty location" +// error on the very first CREATE TABLE. Advertising a default location lets +// Trino take the deferred-transaction path and pick a unique per-table path. +// See issue #9074. +func withDefaultNamespaceLocation(properties map[string]string, bucket string, namespace []string) map[string]string { + properties = normalizeNamespaceProperties(properties) + if _, ok := properties[namespaceLocationProperty]; ok { + return properties + } + if def := defaultNamespaceLocation(bucket, namespace); def != "" { + properties[namespaceLocationProperty] = def + } + return properties +}