Skip to content

Core: Registering tables to nonexistent target namespace leads to metadata deletion in HiveCatalog #13434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hsiang-c wants to merge 8 commits into from
Closed

Core: Registering tables to nonexistent target namespace leads to metadata deletion in HiveCatalog #13434

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
90aa75c
Fail table registration when target namespace doesn't exist
hsiang-c Jun 29, 2025
aa1f797
Revert changes to Spark procedure
hsiang-c Jul 2, 2025
25871ba
Check namespace existence before registering tables
hsiang-c Jul 5, 2025
a4ba226
Test catalog implementations not coverted by CatalogTests
hsiang-c Jul 5, 2025
5c1f393
Merge branch 'main' into namespace_check
hsiang-c Jul 5, 2025
6438f16
Align catalog client and server behavior by JDBC strict mode
hsiang-c Jul 6, 2025
8374e71
Make error messages consistent in core module
hsiang-c Jul 6, 2025
58e9711
Turn on strict mode only in the tests of iceberg-open-api module
hsiang-c Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions ...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestRegisterTableProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.iceberg.spark.extensions;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.assertj.core.api.Assertions.atIndex;

import java.util.List;
Expand Down Expand Up @@ -82,4 +83,32 @@ public void testRegisterTable() throws NoSuchTableException, ParseException {
.as("Should have the right datafile count in the procedure result")
.contains(originalFileCount, atIndex(2));
}

@TestTemplate
public void testRegisterTableToNonexistentNamespace()
throws NoSuchTableException, ParseException {
String targetNameWithNonexistentNamespace =
(catalogName.equals("spark_catalog") ? "" : catalogName + ".")
+ "missing_namespace."
+ "register_table";
long numRows = 1000;

sql("CREATE TABLE %s (id int, data string) using ICEBERG", tableName);
spark
.range(0, numRows)
.withColumn("data", functions.col("id").cast(DataTypes.StringType))
.writeTo(tableName)
.append();

Table table = Spark3Util.loadIcebergTable(spark, tableName);
String metadataJson = TableUtil.metadataFileLocation(table);

assertThatThrownBy(
() ->
sql(
"CALL %s.system.register_table('%s', '%s')",
catalogName, targetNameWithNonexistentNamespace, metadataJson))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot register table to nonexistent target namespace");
}
}
7 changes: 7 additions & 0 deletions .../v4.0/spark/src/main/java/org/apache/iceberg/spark/procedures/RegisterTableProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.iceberg.SnapshotSummary;
import org.apache.iceberg.Table;
import org.apache.iceberg.catalog.Catalog;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.spark.Spark3Util;
Expand Down Expand Up @@ -86,6 +87,12 @@ public InternalRow[] call(InternalRow args) {
"Cannot handle an empty argument metadata_file");

Catalog icebergCatalog = ((HasIcebergCatalog) tableCatalog()).icebergCatalog();
if (tableName.hasNamespace() && icebergCatalog instanceof SupportsNamespaces) {
Copy link
Contributor

@nastra nastra Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this check should be part of the procedure. Instead, such a check should be added in registerTable() in BaseMetastoreCatalog / RestSessionCatalog. Tests can then be added to CatalogTests for RestSessionCatalog. Tests for registerTable in BaseMetastoreCatalog are spread across different catalog implementations, so tests should be added there as well

Copy link
Contributor Author

@hsiang-c hsiang-c Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra Thank you for your feedback, I think it makes sense to me.

Copy link
Contributor Author

@hsiang-c hsiang-c Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nastra I've addressed your feedback. Please take another look when you have time, thanks.

Preconditions.checkArgument(
((SupportsNamespaces) icebergCatalog).namespaceExists(tableName.namespace()),
"Cannot register table to nonexistent target namespace %s",
tableName.namespace());
}
Table table = icebergCatalog.registerTable(tableName, metadataFile);
Long currentSnapshotId = null;
Long totalDataFiles = null;
Expand Down
8 changes: 6 additions & 2 deletions spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/SparkCatalogConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,12 @@ public enum SparkCatalogConfig {
"testhive",
SparkCatalog.class.getName(),
ImmutableMap.of(
"type", "hive",
"default-namespace", "default")),
"type",
"hive",
"default-namespace",
"default",
CatalogProperties.CACHE_ENABLED,
"false")),
HADOOP(
"testhadoop",
SparkCatalog.class.getName(),
Expand Down
Loading