s3tables, iceberg: give tables created without a location their own directory (#12475)

* s3tables: give a table created without a location its own directory An Iceberg table created without a location kept its files under its name path, so a rename left them where the next table of that name would land. Such a table now gets s3://<bucket>/<namespace>/<name>-<uuid>, stored as the record's warehouseLocation and returned by GetTable; its first commit writes there without moving the root. Records without one keep resolving to their name path, so existing tables need no rewrite. Lance datasets stay at their name path, where Lance clients and the vector engine look for them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: create tables and views in a directory of their own A table or view created without a location took its name path, or a unique one only when that path was already taken. It now always gets <name>-<uuid>, so a later rename never leaves its files under a name another table can take. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: load and commit a table at its warehouse location A table created through S3 Tables without a location has no metadata until its first commit; loading or committing it now uses the directory recorded for it, and only a record without one falls back to its name path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: fail a stage-create whose marker is not saved The marker is how the commit finds the location the stage-create handed out. Without it the commit fell back to the name path while the client's files sat under the returned location. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: expect tables created without a location in their own directory Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: rename a table that keeps its files elsewhere before reusing its name Enterprise keeps the storage marker on a renamed table's files, so the name of a table whose files sit under its name path stays taken there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * s3tables: remove the table directory when a create backs out Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: answer a create that lost the race with the registered table Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: answer a view create that lost the race with the registered view Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: fail a staged commit whose name another create registered Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: remove the directory of a create whose name another table took Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: keep the metadata of a staged commit whose name is taken Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: write the lakekeeper table's data under its warehouse location Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: commit a staged table at the location of its own stage Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: never continue another stage when a commit's own marker is gone Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * iceberg: test filer honors exclusive creates and token-guarded deletes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
2026-11-08 21:08:27 +01:00 · 2026-11-07 16:37:31 -08:00
1 parent 55a4a5974c
commit 4f24b756ba
@@ -14,20 +14,10 @@ import (
 )

 // TestDuckDBDeterministicLocationRead validates that DuckDB's iceberg
-// extension can resolve and scan a table that lives at the deterministic
-// <bucket>/<namespace>/<tableName> path produced when no client-side UUID
-// is appended to the table location. This is the layout produced by:
-//
-//   - iceberg-spark (no concept of UUID-suffixed locations)
-//   - pyiceberg (no concept of UUID-suffixed locations)
-//   - Trino with iceberg.unique-table-location=false
-//
-// Post-#9246, REST clients can resolve table locations under a namespace
-// because GetNamespace advertises a default `location` property. This
-// test creates a namespace + table via the REST API directly (so no
-// client-side UUID is added), then asks DuckDB to ATTACH the catalog and
-// describe the table — verifying DuckDB walks the catalog → metadata →
-// schema chain successfully against a deterministic on-disk path.
+// extension can resolve and scan a table created through the REST API with no
+// location. The catalog gives such a table the directory
+// <bucket>/<namespace>/<tableName>-<uuid>, and DuckDB has to reach its
+// metadata and schema through the catalog rather than by the table's name.
 //
 // The test does not insert data files — DuckDB scanning an empty table
 // is sufficient to validate the catalog/metadata resolution path. If
@@ -55,14 +45,11 @@ func TestDuckDBDeterministicLocationRead(t *testing.T) {
 	createNamespaceWithToken(t, env, token, bucketName, namespace)
 	createIcebergTableWithToken(t, env, token, bucketName, namespace, tableName)

-	// Sanity-check the catalog row exposes a deterministic location.
+	// The table gets a directory of its own, named after it and its UUID.
 	tableLoc := getTableLocation(t, env, token, bucketName, namespace, tableName)
-	wantPrefix := fmt.Sprintf("s3://%s/%s/%s", bucketName, namespace, tableName)
-	if !strings.HasPrefix(tableLoc, wantPrefix) {
-		t.Fatalf("table location %q should start with %q (deterministic, no UUID suffix)", tableLoc, wantPrefix)
-	}
-	if strings.Contains(strings.TrimPrefix(tableLoc, wantPrefix), "-") {
-		t.Fatalf("table location %q should not have a -<uuid> suffix after %q", tableLoc, wantPrefix)
+	wantPrefix := fmt.Sprintf("s3://%s/%s/%s-", bucketName, namespace, tableName)
+	if !strings.HasPrefix(tableLoc, wantPrefix) || strings.Contains(strings.TrimPrefix(tableLoc, wantPrefix), "/") {
+		t.Fatalf("table location %q should be a directory %s<uuid>", tableLoc, wantPrefix)
 	}

 	sqlContent := fmt.Sprintf(`
@@ -476,12 +476,24 @@ func (env *TestEnvironment) readTable(t *testing.T, mode string) string {

 // listTableMetadata lists the table's metadata directory through the filer so
 // the repair assertion looks at real objects rather than at what the catalog
-// reports about itself.
+// reports about itself. A table created without a location sits in a
+// <table>-<uuid> directory, so every directory of the table's name is read.
 func (env *TestEnvironment) listTableMetadata(t *testing.T) []string {
 	t.Helper()

-	url := fmt.Sprintf("http://%s:%d/buckets/%s/%s/%s/metadata/?limit=200",
-		env.bindIP, env.filerPort, tableBucketName, expectedNamespace, expectedTable)
+	var names []string
+	for _, dir := range env.listFilerDir(t, fmt.Sprintf("/buckets/%s/%s/", tableBucketName, expectedNamespace)) {
+		if dir == expectedTable || strings.HasPrefix(dir, expectedTable+"-") {
+			names = append(names, env.listFilerDir(t, fmt.Sprintf("/buckets/%s/%s/%s/metadata/", tableBucketName, expectedNamespace, dir))...)
+		}
+	}
+	return names
+}
+
+func (env *TestEnvironment) listFilerDir(t *testing.T, dir string) []string {
+	t.Helper()
+
+	url := fmt.Sprintf("http://%s:%d%s?limit=200", env.bindIP, env.filerPort, dir)
 	req, err := http.NewRequest(http.MethodGet, url, nil)
 	if err != nil {
 		t.Fatalf("Failed to build filer request: %v", err)
@@ -494,6 +506,9 @@ func (env *TestEnvironment) listTableMetadata(t *testing.T) []string {
 	}
 	defer resp.Body.Close()
 	body, _ := io.ReadAll(resp.Body)
+	if resp.StatusCode == http.StatusNotFound {
+		return nil
+	}
 	if resp.StatusCode != http.StatusOK {
 		t.Fatalf("filer listing returned %d: %s", resp.StatusCode, body)
 	}
@@ -13,11 +13,9 @@ import (

 // TestSparkDeterministicLocationCTAS exercises Spark's iceberg-spark
 // connector against the SeaweedFS REST catalog with no explicit `LOCATION`
-// clause and no UUID-suffixing. iceberg-spark does not implement Trino's
-// unique-table-location feature, so every CREATE TABLE here produces a
-// deterministic <namespace-location>/<tableName> path. The test asserts
-// the fresh-CTAS pattern works AND the resulting on-disk layout has no
-// UUID-suffixed sibling — i.e. one directory per table.
+// clause. The catalog picks the location, <namespace>/<tableName>-<uuid>, and
+// the test asserts the fresh-CTAS pattern works and each table's files sit in
+// that one directory, none under the bare table name.
 //
 // The fix in #9246 (advertise a default `location` namespace property)
 // is what allows iceberg-spark to resolve the table location at all when
@@ -82,13 +80,11 @@ print(f"ctas row count: {count}")
 		}
 	}

-	// Layout assertion: each table's data must land at the deterministic
-	// <namespace>/<tableName>/ path, with no UUID-suffixed sibling.
+	// Layout assertion: each table's files land in one <tableName>-<uuid>
+	// directory, and nothing sits under the bare table name.
 	keys := listSparkBucketKeysUnder(t, env, tableBucket, namespace+"/")
-	expectKeyAtSparkLocation(t, keys, namespace, srcTable)
-	expectKeyAtSparkLocation(t, keys, namespace, ctasTable)
-	rejectUUIDSuffixedSparkSibling(t, keys, namespace, srcTable)
-	rejectUUIDSuffixedSparkSibling(t, keys, namespace, ctasTable)
+	expectKeysInOneSparkLocation(t, keys, namespace, srcTable)
+	expectKeysInOneSparkLocation(t, keys, namespace, ctasTable)
 }

 func listSparkBucketKeysUnder(t *testing.T, env *TestEnvironment, bucket, prefix string) []string {
@@ -113,23 +109,18 @@ func listSparkBucketKeysUnder(t *testing.T, env *TestEnvironment, bucket, prefix
 	return keys
 }

-func expectKeyAtSparkLocation(t *testing.T, keys []string, namespace, table string) {
+func expectKeysInOneSparkLocation(t *testing.T, keys []string, namespace, table string) {
 	t.Helper()
-	want := namespace + "/" + table + "/"
+	dirs := map[string]bool{}
 	for _, k := range keys {
-		if strings.HasPrefix(k, want) {
-			return
+		if strings.HasPrefix(k, namespace+"/"+table+"/") {
+			t.Fatalf("found key %q under the bare table name %q", k, table)
+		}
+		if rest, ok := strings.CutPrefix(k, namespace+"/"+table+"-"); ok {
+			dirs[strings.SplitN(rest, "/", 2)[0]] = true
 		}
 	}
-	t.Fatalf("expected at least one object under %q; got keys: %v", want, keys)
-}
-
-func rejectUUIDSuffixedSparkSibling(t *testing.T, keys []string, namespace, table string) {
-	t.Helper()
-	prefix := namespace + "/" + table + "-"
-	for _, k := range keys {
-		if strings.HasPrefix(k, prefix) {
-			t.Fatalf("found UUID-suffixed sibling key %q for table %q (Spark should produce no -<uuid> suffix)", k, table)
-		}
+	if len(dirs) != 1 {
+		t.Fatalf("want the files of %q in one %s-<uuid> directory, found %d; keys: %v", table, table, len(dirs), keys)
 	}
 }
@@ -384,7 +384,15 @@ func runLakekeeperTableBucketRepro(t *testing.T, env *TestEnvironment) {
 		t.Fatalf("Create S3 client failed: %v", err)
 	}

-	objectKey := fmt.Sprintf("%s/%s/data/part-%d.parquet", namespaceName, tableName, time.Now().UnixNano())
+	table, err := client.GetTable(ctx, bucketARN, namespaceName, tableName)
+	if err != nil {
+		t.Fatalf("GetTable failed: %v", err)
+	}
+	tableDir := strings.TrimPrefix(table.WarehouseLocation, "s3://"+bucketName+"/")
+	if tableDir == "" || tableDir == table.WarehouseLocation {
+		t.Fatalf("unexpected warehouse location %q", table.WarehouseLocation)
+	}
+	objectKey := fmt.Sprintf("%s/data/part-%d.parquet", tableDir, time.Now().UnixNano())
 	createResp, err := s3Client.CreateMultipartUpload(ctx, &s3.CreateMultipartUploadInput{
 		Bucket: aws.String(bucketName),
 		Key:    aws.String(objectKey),
@@ -556,6 +564,18 @@ func (c *s3TablesClient) CreateTable(ctx context.Context, bucketARN, namespace,
 	return c.doRequest(ctx, "CreateTable", http.MethodPut, path, req, nil)
 }

+func (c *s3TablesClient) GetTable(ctx context.Context, bucketARN, namespace, name string) (*s3tables.GetTableResponse, error) {
+	query := url.Values{}
+	query.Set("tableBucketARN", bucketARN)
+	query.Set("namespace", namespace)
+	query.Set("name", name)
+	var resp s3tables.GetTableResponse
+	if err := c.doRequest(ctx, "GetTable", http.MethodGet, "/get-table?"+query.Encode(), nil, &resp); err != nil {
+		return nil, err
+	}
+	return &resp, nil
+}
+
 func (c *s3TablesClient) DeleteTable(ctx context.Context, bucketARN, namespace, name string) error {
 	path := "/tables/" + url.PathEscape(bucketARN) + "/" + url.PathEscape(namespace) + "/" + url.PathEscape(name)
 	return c.doRequest(ctx, "DeleteTable", http.MethodDelete, path, nil, nil)