You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by ze...@apache.org on 2026/07/24 18:08:19 UTC
(iceberg-go) branch main updated: fix(hadoop): validate metadata before dropping tables (#1491)
This is an automated email from the ASF dual-hosted git repository.
zeroshade pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-go.git
The following commit(s) were added to refs/heads/main by this push:
new 446cba12e fix(hadoop): validate metadata before dropping tables (#1491)
446cba12e is described below
commit 446cba12eb2012f131e5a55bfbaaf62fed3a9612
Author: Minh Vu <vu...@gmail.com>
AuthorDate: Fri Jul 24 20:08:14 2026 +0200
fix(hadoop): validate metadata before dropping tables (#1491)
## Problem
`DropTable` considered a directory to be a table based only on the
presence of a metadata-shaped filename, then recursively deleted the
directory. A partial or corrupt metadata file could therefore authorize
a destructive drop.
## Changes
Load and parse the current table metadata before removing the table
directory. Invalid metadata now returns an error and leaves the
directory untouched.
The existing successful-drop tests now create real table metadata rather
than placeholder JSON.
## Testing
Added a regression test with invalid metadata and a marker file that
must survive the failed drop.
- `go test ./catalog/hadoop`
---
catalog/hadoop/hadoop.go | 11 ++---------
catalog/hadoop/hadoop_integration_test.go | 24 +++++++++++------------
catalog/hadoop/hadoop_test.go | 32 +++++++++++++++++++++++--------
3 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/catalog/hadoop/hadoop.go b/catalog/hadoop/hadoop.go
index 218149cfe..74b5df899 100644
--- a/catalog/hadoop/hadoop.go
+++ b/catalog/hadoop/hadoop.go
@@ -887,19 +887,12 @@ func (c *Catalog) ListTables(_ context.Context, ns table.Identifier) iter.Seq2[t
}
}
-func (c *Catalog) DropTable(_ context.Context, ident table.Identifier) error {
- if err := validateTableIdentifier(ident); err != nil {
+func (c *Catalog) DropTable(ctx context.Context, ident table.Identifier) error {
+ if _, _, err := c.loadTable(ctx, ident); err != nil {
return err
}
tablePath := c.tableToPath(ident)
- isTable, err := isTableDir(c.filesystem, c.isLocal, tablePath)
- if err != nil {
- return fmt.Errorf("hadoop catalog: failed to inspect table directory: %w", err)
- }
- if !isTable {
- return fmt.Errorf("%w: %s", catalog.ErrNoSuchTable, strings.Join(ident, "."))
- }
return c.filesystem.RemoveAll(tablePath)
}
diff --git a/catalog/hadoop/hadoop_integration_test.go b/catalog/hadoop/hadoop_integration_test.go
index fb1642173..ac2e2277c 100644
--- a/catalog/hadoop/hadoop_integration_test.go
+++ b/catalog/hadoop/hadoop_integration_test.go
@@ -82,18 +82,16 @@ func (s *HadoopIntegrationSuite) sparkSQL(sql string) string {
return output
}
-// createFakeTable creates a minimal table directory structure owned by
-// the runner process. This avoids root-ownership issues that arise when
-// Spark creates tables via Docker (root in container).
-func (s *HadoopIntegrationSuite) createFakeTable(ident table.Identifier) {
+// createTable creates a valid table owned by the runner process. This avoids
+// root-ownership issues that arise when Spark creates tables via Docker.
+func (s *HadoopIntegrationSuite) createTable(ident table.Identifier) {
s.T().Helper()
- tablePath := filepath.Join(append([]string{s.warehouse}, ident...)...)
- metaDir := filepath.Join(tablePath, "metadata")
- s.Require().NoError(os.MkdirAll(metaDir, 0o755))
- s.Require().NoError(os.WriteFile(
- filepath.Join(metaDir, "v1.metadata.json"), []byte("{}"), 0o644,
+ _, err := s.cat.CreateTable(s.ctx, ident, iceberg.NewSchema(
+ 1,
+ iceberg.NestedField{ID: 1, Name: "id", Type: iceberg.PrimitiveTypes.Int64, Required: true},
))
+ s.Require().NoError(err)
}
// TestListTablesSparkCreated creates tables in Spark and verifies that
@@ -150,7 +148,7 @@ func (s *HadoopIntegrationSuite) TestDropTableExisting() {
err := s.cat.CreateNamespace(s.ctx, []string{"drop_ns"}, nil)
s.Require().NoError(err)
- s.createFakeTable([]string{"drop_ns", "to_drop"})
+ s.createTable([]string{"drop_ns", "to_drop"})
tablePath := filepath.Join(s.warehouse, "drop_ns", "to_drop")
_, err = os.Stat(tablePath)
@@ -176,7 +174,7 @@ func (s *HadoopIntegrationSuite) TestDropTableNamespacePreserved() {
err := s.cat.CreateNamespace(s.ctx, []string{"preserve_ns"}, nil)
s.Require().NoError(err)
- s.createFakeTable([]string{"preserve_ns", "tbl"})
+ s.createTable([]string{"preserve_ns", "tbl"})
err = s.cat.DropTable(s.ctx, []string{"preserve_ns", "tbl"})
s.Require().NoError(err)
@@ -217,8 +215,8 @@ func (s *HadoopIntegrationSuite) TestDropTableThenListReflects() {
err := s.cat.CreateNamespace(s.ctx, []string{"droplist_ns"}, nil)
s.Require().NoError(err)
- s.createFakeTable([]string{"droplist_ns", "keep"})
- s.createFakeTable([]string{"droplist_ns", "remove"})
+ s.createTable([]string{"droplist_ns", "keep"})
+ s.createTable([]string{"droplist_ns", "remove"})
err = s.cat.DropTable(s.ctx, []string{"droplist_ns", "remove"})
s.Require().NoError(err)
diff --git a/catalog/hadoop/hadoop_test.go b/catalog/hadoop/hadoop_test.go
index 15fa74186..7fbebd9c2 100644
--- a/catalog/hadoop/hadoop_test.go
+++ b/catalog/hadoop/hadoop_test.go
@@ -1442,10 +1442,11 @@ func (s *HadoopCatalogTestSuite) TestListTablesDoesNotYieldTablesInChildNamespac
func (s *HadoopCatalogTestSuite) TestDropTable() {
ctx := context.Background()
- s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
- s.createFakeTable([]string{"ns", "tbl"})
+ s.Require().NoError(s.cat.CreateNamespace(ctx, []string{"ns"}, nil))
+ _, err := s.cat.CreateTable(ctx, []string{"ns", "tbl"}, s.testSchema())
+ s.Require().NoError(err)
- err := s.cat.DropTable(ctx, []string{"ns", "tbl"})
+ err = s.cat.DropTable(ctx, []string{"ns", "tbl"})
s.Require().NoError(err)
// Verify the table directory is completely removed.
@@ -1460,21 +1461,35 @@ func (s *HadoopCatalogTestSuite) TestDropTableNotExists() {
func (s *HadoopCatalogTestSuite) TestDropTableVerifyCleanup() {
ctx := context.Background()
- s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
- s.createFakeTable([]string{"ns", "tbl"})
+ s.Require().NoError(s.cat.CreateNamespace(ctx, []string{"ns"}, nil))
+ _, err := s.cat.CreateTable(ctx, []string{"ns", "tbl"}, s.testSchema())
+ s.Require().NoError(err)
// Add a data file to confirm everything is purged.
dataDir := filepath.Join(s.warehouse, "ns", "tbl", "data")
s.Require().NoError(os.MkdirAll(dataDir, 0o755))
s.Require().NoError(os.WriteFile(filepath.Join(dataDir, "00000-0-0-00000.parquet"), []byte("data"), 0o644))
- err := s.cat.DropTable(ctx, []string{"ns", "tbl"})
+ err = s.cat.DropTable(ctx, []string{"ns", "tbl"})
s.Require().NoError(err)
_, statErr := os.Stat(filepath.Join(s.warehouse, "ns", "tbl"))
s.True(os.IsNotExist(statErr))
}
+func (s *HadoopCatalogTestSuite) TestDropTablePreservesDirectoryIfMetadataIsInvalid() {
+ ctx := context.Background()
+ s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
+ s.createFakeTable([]string{"ns", "tbl"})
+
+ markerPath := filepath.Join(s.warehouse, "ns", "tbl", "marker")
+ s.Require().NoError(os.WriteFile(markerPath, []byte("keep"), 0o644))
+
+ err := s.cat.DropTable(ctx, []string{"ns", "tbl"})
+ s.Require().Error(err)
+ s.FileExists(markerPath)
+}
+
func (s *HadoopCatalogTestSuite) TestDropTableShortIdentifier() {
err := s.cat.DropTable(context.Background(), []string{"tbl"})
s.Require().Error(err)
@@ -2063,8 +2078,9 @@ func (s *HadoopCatalogTestSuite) TestTableOperationsRejectParentTraversalIdentif
func (s *HadoopCatalogTestSuite) TestDropTableNamespacePreserved() {
// Dropping a table should not affect the parent namespace directory.
ctx := context.Background()
- s.Require().NoError(os.Mkdir(filepath.Join(s.warehouse, "ns"), 0o755))
- s.createFakeTable([]string{"ns", "tbl"})
+ s.Require().NoError(s.cat.CreateNamespace(ctx, []string{"ns"}, nil))
+ _, err := s.cat.CreateTable(ctx, []string{"ns", "tbl"}, s.testSchema())
+ s.Require().NoError(err)
s.Require().NoError(s.cat.DropTable(ctx, []string{"ns", "tbl"}))