gaborkaszab commented on code in PR #11597:
URL: https://github.com/apache/iceberg/pull/11597#discussion_r1867329974
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -412,6 +412,34 @@ private void validateTableIsIcebergTableOrView(
}
}
+ @Override
+ public boolean tableExists(TableIdentifier identifier) {
+ TableIdentifier baseTableIdentifier = identifier;
+ if (!isValidIdentifier(identifier)) {
+ if (isValidMetadataIdentifier(identifier)) {
Review Comment:
nit: might just be a personal preference but I find the code more readable
and compact when we check for exit criteria first, like this:
```
if (!isValidMetadataIdentifier()) {
return false;
}
baseTableIdentifier = ...;
```
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -412,6 +412,34 @@ private void validateTableIsIcebergTableOrView(
}
}
+ @Override
+ public boolean tableExists(TableIdentifier identifier) {
+ TableIdentifier baseTableIdentifier = identifier;
+ if (!isValidIdentifier(identifier)) {
+ if (isValidMetadataIdentifier(identifier)) {
+ baseTableIdentifier =
TableIdentifier.of(identifier.namespace().levels());
+ } else {
+ return false;
+ }
+ }
+
+ String database = baseTableIdentifier.namespace().level(0);
+ String tableName = baseTableIdentifier.name();
+ try {
+ Table table = clients.run(client -> client.getTable(database,
tableName));
+ HiveOperationsBase.validateTableIsIceberg(table, fullTableName(name,
identifier));
Review Comment:
Here we check if the base table is an Iceberg table, but in case of metadata
tables in the error message we print the metadata table name instead of the
base table name. Doesn't seem to matter much but I think we should print the
base table name here as the check is also performed against the base table.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -388,6 +388,41 @@ public void
testHiveTableAndIcebergTableWithSameName(TableType tableType)
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTableName);
}
+ @Test
+ public void testTableExists() throws TException, IOException {
+ String testTableName = "test_table_exists";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, testTableName);
+ TableIdentifier metadataIdentifier = TableIdentifier.of(DB_NAME,
testTableName, "partitions");
+
+ assertThat(catalog.tableExists(identifier))
+ .as("Table should not exist before create")
+ .isFalse();
+ catalog.buildTable(identifier, SCHEMA).create();
+ assertThat(catalog.tableExists(identifier)).as("Table should exist after
create").isTrue();
+ assertThat(catalog.tableExists(metadataIdentifier))
+ .as("Metadata table should also exist")
+ .isTrue();
+
+ boolean dropped = catalog.dropTable(identifier);
+ assertThat(dropped).as("Should drop a table that does exist").isTrue();
+ assertThat(catalog.tableExists(identifier)).as("Table should not exist
after drop").isFalse();
+ assertThat(catalog.tableExists(metadataIdentifier))
+ .as("Metadata table should not exist after drop")
+ .isFalse();
+
+ // recreate a hive table with the same name shall return false instead of
throw exception
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(testTableName, TableType.EXTERNAL_TABLE));
+ assertThat(catalog.tableExists(identifier))
+ .as("Catalog shall return false even if hive table with the same name
exists")
+ .isFalse();
+ assertThat(catalog.tableExists(metadataIdentifier))
+ .as("Metadata table should not exist")
+ .isFalse();
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
testTableName);
+ }
Review Comment:
I think tableExists() should give false if the provided identifier refers to
a view. Would it make sense to add some test coverage for that as well?
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -388,6 +388,41 @@ public void
testHiveTableAndIcebergTableWithSameName(TableType tableType)
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTableName);
}
+ @Test
+ public void testTableExists() throws TException, IOException {
+ String testTableName = "test_table_exists";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, testTableName);
+ TableIdentifier metadataIdentifier = TableIdentifier.of(DB_NAME,
testTableName, "partitions");
+
+ assertThat(catalog.tableExists(identifier))
+ .as("Table should not exist before create")
+ .isFalse();
+ catalog.buildTable(identifier, SCHEMA).create();
Review Comment:
nit: I'd put an empty line above just to separate different steps of the
test. L397-399 tests a precondition of the test while this performs one of the
actual tests.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -388,6 +388,41 @@ public void
testHiveTableAndIcebergTableWithSameName(TableType tableType)
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTableName);
}
+ @Test
+ public void testTableExists() throws TException, IOException {
+ String testTableName = "test_table_exists";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, testTableName);
+ TableIdentifier metadataIdentifier = TableIdentifier.of(DB_NAME,
testTableName, "partitions");
+
+ assertThat(catalog.tableExists(identifier))
+ .as("Table should not exist before create")
+ .isFalse();
+ catalog.buildTable(identifier, SCHEMA).create();
+ assertThat(catalog.tableExists(identifier)).as("Table should exist after
create").isTrue();
+ assertThat(catalog.tableExists(metadataIdentifier))
+ .as("Metadata table should also exist")
+ .isTrue();
+
+ boolean dropped = catalog.dropTable(identifier);
Review Comment:
nit: instead of introducing a variable could you merge this line with the
following similarly as you do at L397?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
