You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nk1506 (via GitHub)" <gi...@apache.org> on 2024/03/17 15:47:37 UTC
[PR] Hive: Test to do validation hive content and iceberg table with the same name [iceberg]
nk1506 opened a new pull request, #9980:
URL: https://github.com/apache/iceberg/pull/9980
Hive should not allow creating an iceberg table, if there is already another content available with same name.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1537212633
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -78,13 +82,23 @@
import org.apache.iceberg.types.Types;
import org.apache.thrift.TException;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.io.TempDir;
+@ExtendWith(ParameterizedTestExtension.class)
Review Comment:
you don't have to make the full class a parameterized test. https://github.com/apache/iceberg/blob/820fc3ceda386149f42db8b54e6db9171d1a3a6d/flink/v1.18/flink/src/test/java/org/apache/iceberg/flink/sink/TestBucketPartitionerFlinkIcebergSink.java#L137-L141 is all you need and that's what I meant in https://github.com/apache/iceberg/pull/9980#discussion_r1535435047
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535425970
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
+
+ // create a hive table
+ String hiveTableName = "test_hive_table";
+ org.apache.hadoop.hive.metastore.api.Table hiveTable =
+ createHiveTable(hiveTableName, TableType.EXTERNAL_TABLE);
+ HIVE_METASTORE_EXTENSION.metastoreClient().createTable(hiveTable);
+
+ catalog.setListAllTables(true);
+ List<TableIdentifier> tableIdents2 = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents2).as("should be 2 tables in namespace .").hasSize(2);
Review Comment:
`assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace())).hasSize(2).containsExactly(...)` (no need to define `List<TableIdentifier> tableIdents2`)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1539385906
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +353,43 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @ParameterizedTest
+ @EnumSource(
+ value = TableType.class,
+ names = {"EXTERNAL_TABLE", "VIRTUAL_VIEW", "MANAGED_TABLE"})
+ public void testHiveTableAndIcebergTableWithSameName(TableType tableType)
+ throws TException, IOException {
+
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(1)
+ .containsExactly(TABLE_IDENTIFIER);
+
+ // create a hive table with a defined table type.
+ String hiveTableName = "test_hive_table";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTableName, tableType));
+
+ catalog.setListAllTables(true);
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(2)
+ .containsExactly(TABLE_IDENTIFIER, identifier);
+ catalog.setListAllTables(false); // reset to default.
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(() -> catalog.createTable(identifier, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith(String.format("Not an iceberg table: hive.%s", identifier));
Review Comment:
actually nvm, things look like they work as expected
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535428044
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
Review Comment:
no need to store this in a variable
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1538906232
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +353,43 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @ParameterizedTest
+ @EnumSource(
+ value = TableType.class,
+ names = {"EXTERNAL_TABLE", "VIRTUAL_VIEW", "MANAGED_TABLE"})
+ public void testHiveTableAndIcebergTableWithSameName(TableType tableType)
+ throws TException, IOException {
+
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(1)
+ .containsExactly(TABLE_IDENTIFIER);
+
+ // create a hive table with a defined table type.
+ String hiveTableName = "test_hive_table";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTableName, tableType));
+
+ catalog.setListAllTables(true);
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(2)
+ .containsExactly(TABLE_IDENTIFIER, identifier);
+ catalog.setListAllTables(false); // reset to default.
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(() -> catalog.createTable(identifier, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith(String.format("Not an iceberg table: hive.%s", identifier));
Review Comment:
isn't it weird that the actual commit succeeds but the error is thrown when a refresh is done? https://github.com/apache/iceberg/blob/817a5e1be1616af77329965ac3742c14ca3ae116/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L148
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535431150
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
+
+ // create a hive table
+ String hiveTableName = "test_hive_table";
+ org.apache.hadoop.hive.metastore.api.Table hiveTable =
+ createHiveTable(hiveTableName, TableType.EXTERNAL_TABLE);
+ HIVE_METASTORE_EXTENSION.metastoreClient().createTable(hiveTable);
+
+ catalog.setListAllTables(true);
+ List<TableIdentifier> tableIdents2 = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents2).as("should be 2 tables in namespace .").hasSize(2);
+
+ TableIdentifier identifierWithHiveTableName = TableIdentifier.of(DB_NAME, hiveTableName);
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(
+ () ->
+ catalog.createTable(
+ identifierWithHiveTableName, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith("Not an iceberg table: hive.hivedb.test_hive_table");
+
+ assertThat(catalog.tableExists(identifierWithHiveTableName)).isFalse();
+
+ assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
+ }
+
+ @Test
+ public void testHiveViewAndIcebergTableWithSameName() throws TException, IOException {
Review Comment:
please also reflect the comments from above in this test method
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535428308
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
Review Comment:
```suggestion
assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace())).hasSize(1).containsExactly(...);
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9980:
URL: https://github.com/apache/iceberg/pull/9980
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535435047
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
Review Comment:
you could change this to a `@ParameterizedTest`, since the only difference between the two tests is the `TableType`. So the test would have a `TableType` parameter, where you pass `TableType.EXTERNAL_TABLE` and `TableType.VIRTUAL_VIEW` via `@EnumSource` (there are examples in the codebase on how to use parameterized tests with enums)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nk1506 (via GitHub)" <gi...@apache.org>.
nk1506 commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1539203705
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +353,43 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @ParameterizedTest
+ @EnumSource(
+ value = TableType.class,
+ names = {"EXTERNAL_TABLE", "VIRTUAL_VIEW", "MANAGED_TABLE"})
+ public void testHiveTableAndIcebergTableWithSameName(TableType tableType)
+ throws TException, IOException {
+
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(1)
+ .containsExactly(TABLE_IDENTIFIER);
+
+ // create a hive table with a defined table type.
+ String hiveTableName = "test_hive_table";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTableName, tableType));
+
+ catalog.setListAllTables(true);
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(2)
+ .containsExactly(TABLE_IDENTIFIER, identifier);
+ catalog.setListAllTables(false); // reset to default.
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(() -> catalog.createTable(identifier, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith(String.format("Not an iceberg table: hive.%s", identifier));
Review Comment:
Sorry , I didn't understand the part as `actual commit succeeds` . in this case the error is being thrown before doing actual commit and the commit for corresponding operation is never happening.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1539267925
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +353,43 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @ParameterizedTest
+ @EnumSource(
+ value = TableType.class,
+ names = {"EXTERNAL_TABLE", "VIRTUAL_VIEW", "MANAGED_TABLE"})
+ public void testHiveTableAndIcebergTableWithSameName(TableType tableType)
+ throws TException, IOException {
+
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(1)
+ .containsExactly(TABLE_IDENTIFIER);
+
+ // create a hive table with a defined table type.
+ String hiveTableName = "test_hive_table";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTableName, tableType));
+
+ catalog.setListAllTables(true);
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(2)
+ .containsExactly(TABLE_IDENTIFIER, identifier);
+ catalog.setListAllTables(false); // reset to default.
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(() -> catalog.createTable(identifier, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith(String.format("Not an iceberg table: hive.%s", identifier));
Review Comment:
> in this case the error is being thrown before doing actual commit and the commit for corresponding operation is never happening
I've actually debugged this locally and the commit succeeds. The error is then thrown when a refresh is done
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535430072
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
+
+ // create a hive table
+ String hiveTableName = "test_hive_table";
+ org.apache.hadoop.hive.metastore.api.Table hiveTable =
+ createHiveTable(hiveTableName, TableType.EXTERNAL_TABLE);
+ HIVE_METASTORE_EXTENSION.metastoreClient().createTable(hiveTable);
+
+ catalog.setListAllTables(true);
+ List<TableIdentifier> tableIdents2 = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents2).as("should be 2 tables in namespace .").hasSize(2);
+
+ TableIdentifier identifierWithHiveTableName = TableIdentifier.of(DB_NAME, hiveTableName);
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(
+ () ->
+ catalog.createTable(
+ identifierWithHiveTableName, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith("Not an iceberg table: hive.hivedb.test_hive_table");
Review Comment:
```suggestion
.hasMessageStartingWith(String.format("Not an iceberg table: %s", identifier));
```
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
+
+ // create a hive table
+ String hiveTableName = "test_hive_table";
+ org.apache.hadoop.hive.metastore.api.Table hiveTable =
+ createHiveTable(hiveTableName, TableType.EXTERNAL_TABLE);
+ HIVE_METASTORE_EXTENSION.metastoreClient().createTable(hiveTable);
+
+ catalog.setListAllTables(true);
+ List<TableIdentifier> tableIdents2 = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents2).as("should be 2 tables in namespace .").hasSize(2);
+
+ TableIdentifier identifierWithHiveTableName = TableIdentifier.of(DB_NAME, hiveTableName);
Review Comment:
```suggestion
TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1535425250
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +351,70 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @Test
+ public void testHiveTableAndIcebergTableWithSameName() throws TException, IOException {
+ List<TableIdentifier> tableIdents = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents).as("should be 1 table in namespace .").hasSize(1);
+
+ // create a hive table
+ String hiveTableName = "test_hive_table";
+ org.apache.hadoop.hive.metastore.api.Table hiveTable =
+ createHiveTable(hiveTableName, TableType.EXTERNAL_TABLE);
+ HIVE_METASTORE_EXTENSION.metastoreClient().createTable(hiveTable);
+
+ catalog.setListAllTables(true);
+ List<TableIdentifier> tableIdents2 = catalog.listTables(TABLE_IDENTIFIER.namespace());
+ assertThat(tableIdents2).as("should be 2 tables in namespace .").hasSize(2);
Review Comment:
this should also use `.containsExactly(..)` to check what exactly comes back
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Tests to do validation hive content and iceberg table with the same name [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9980:
URL: https://github.com/apache/iceberg/pull/9980#discussion_r1539267925
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java:
##########
@@ -349,8 +353,43 @@ public void testListTables() throws TException, IOException {
HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME, hiveTableName);
}
- private org.apache.hadoop.hive.metastore.api.Table createHiveTable(String hiveTableName)
- throws IOException {
+ @ParameterizedTest
+ @EnumSource(
+ value = TableType.class,
+ names = {"EXTERNAL_TABLE", "VIRTUAL_VIEW", "MANAGED_TABLE"})
+ public void testHiveTableAndIcebergTableWithSameName(TableType tableType)
+ throws TException, IOException {
+
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(1)
+ .containsExactly(TABLE_IDENTIFIER);
+
+ // create a hive table with a defined table type.
+ String hiveTableName = "test_hive_table";
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, hiveTableName);
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTableName, tableType));
+
+ catalog.setListAllTables(true);
+ assertThat(catalog.listTables(TABLE_IDENTIFIER.namespace()))
+ .hasSize(2)
+ .containsExactly(TABLE_IDENTIFIER, identifier);
+ catalog.setListAllTables(false); // reset to default.
+
+ // create an iceberg table with the same name
+ assertThatThrownBy(() -> catalog.createTable(identifier, schema, PartitionSpec.unpartitioned()))
+ .isInstanceOf(NoSuchIcebergTableException.class)
+ .hasMessageStartingWith(String.format("Not an iceberg table: hive.%s", identifier));
Review Comment:
> in this case the error is being thrown before doing actual commit and the commit for corresponding operation is never happening
I've actually debugged this locally and the commit succeeds. The error is then thrown when a refresh is done
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org