terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3642773861
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1271,108 @@ public void testMetadataHashing(boolean
isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path
(listIcebergTablesByFilter). Hive
+ // table/listing filtering is off by default and relies on the HMS-side
+ // `get_table_names_by_filter`
+ // filter, so we assert that non-Iceberg objects are excluded from
listTables even
+ // when other tables/views co-exist in the same database.
+
+ @Test
+ public void testListTablesFiltersOutNonIcebergTables() throws TException,
IOException {
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier icebergTable = TableIdentifier.of(ns, "iceberg_tbl");
+ TableIdentifier hiveTable = TableIdentifier.of(ns, "hive_tbl");
+
+ catalog.createTable(icebergTable, getTestSchema());
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTable.name(),
TableType.EXTERNAL_TABLE));
+
+ try {
+ assertThat(catalog.listTables(ns)).containsExactly(icebergTable);
+ } finally {
+ catalog.dropTable(icebergTable);
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTable.name());
+ }
+ }
+
+ @Test
+ public void testListTablesByFilterReturnsAllIcebergTables() throws Exception
{
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier t1 = TableIdentifier.of(ns, "t1");
+ TableIdentifier t2 = TableIdentifier.of(ns, "t2");
+
+ catalog.createTable(t1, getTestSchema());
+ catalog.createTable(t2, getTestSchema());
+
+ try {
+ assertThat(catalog.listTables(ns)).containsExactlyInAnyOrder(t1, t2);
+ } finally {
+ catalog.dropTable(t1);
+ catalog.dropTable(t2);
+ }
+ }
+
+ @Test
+ public void testListTablesFiltersNonIcebergTables() throws TException,
IOException {
+ // The server-side filter should exclude a non-Iceberg table that
co-exists with an Iceberg
+ // table in the same database.
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier icebergTable = TableIdentifier.of(ns, "iceberg_tbl");
+ TableIdentifier hiveTable = TableIdentifier.of(ns, "hive_tbl");
+
+ catalog.createTable(icebergTable, getTestSchema());
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTable.name(),
TableType.EXTERNAL_TABLE));
+
+ try {
+ assertThat(catalog.listTables(ns))
+ .as("server-side filter should exclude the hive table")
+ .containsExactly(icebergTable);
+ } finally {
+ catalog.dropTable(icebergTable);
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTable.name());
+ }
+ }
+
+ private org.apache.hadoop.hive.metastore.api.Table createHiveTable(
+ String hiveTableName, TableType type) throws IOException {
+ Map<String, String> parameters = Maps.newHashMap();
+ parameters.put(
+ serdeConstants.SERIALIZATION_CLASS,
"org.apache.hadoop.hive.serde2.thrift.test.IntString");
Review Comment:
@pvary This helper was copied from the existing createHiveTable in
TestHiveTable / TestHiveViewCatalog. You're right — none of the SerDe
class/parameters/view-text values matter here; the only thing the test needs is
a table without the table_type=ICEBERG property. I've simplified it to a
minimal table definition.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1271,108 @@ public void testMetadataHashing(boolean
isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path
(listIcebergTablesByFilter). Hive
+ // table/listing filtering is off by default and relies on the HMS-side
+ // `get_table_names_by_filter`
+ // filter, so we assert that non-Iceberg objects are excluded from
listTables even
+ // when other tables/views co-exist in the same database.
+
+ @Test
+ public void testListTablesFiltersOutNonIcebergTables() throws TException,
IOException {
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier icebergTable = TableIdentifier.of(ns, "iceberg_tbl");
+ TableIdentifier hiveTable = TableIdentifier.of(ns, "hive_tbl");
+
+ catalog.createTable(icebergTable, getTestSchema());
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTable.name(),
TableType.EXTERNAL_TABLE));
+
+ try {
+ assertThat(catalog.listTables(ns)).containsExactly(icebergTable);
+ } finally {
+ catalog.dropTable(icebergTable);
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTable.name());
+ }
+ }
+
+ @Test
+ public void testListTablesByFilterReturnsAllIcebergTables() throws Exception
{
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier t1 = TableIdentifier.of(ns, "t1");
+ TableIdentifier t2 = TableIdentifier.of(ns, "t2");
+
+ catalog.createTable(t1, getTestSchema());
+ catalog.createTable(t2, getTestSchema());
+
+ try {
+ assertThat(catalog.listTables(ns)).containsExactlyInAnyOrder(t1, t2);
+ } finally {
+ catalog.dropTable(t1);
+ catalog.dropTable(t2);
+ }
+ }
+
+ @Test
+ public void testListTablesFiltersNonIcebergTables() throws TException,
IOException {
+ // The server-side filter should exclude a non-Iceberg table that
co-exists with an Iceberg
+ // table in the same database.
+ Namespace ns = Namespace.of(DB_NAME);
+ TableIdentifier icebergTable = TableIdentifier.of(ns, "iceberg_tbl");
+ TableIdentifier hiveTable = TableIdentifier.of(ns, "hive_tbl");
+
+ catalog.createTable(icebergTable, getTestSchema());
+ HIVE_METASTORE_EXTENSION
+ .metastoreClient()
+ .createTable(createHiveTable(hiveTable.name(),
TableType.EXTERNAL_TABLE));
+
+ try {
+ assertThat(catalog.listTables(ns))
+ .as("server-side filter should exclude the hive table")
+ .containsExactly(icebergTable);
+ } finally {
+ catalog.dropTable(icebergTable);
+ HIVE_METASTORE_EXTENSION.metastoreClient().dropTable(DB_NAME,
hiveTable.name());
+ }
+ }
+
+ private org.apache.hadoop.hive.metastore.api.Table createHiveTable(
+ String hiveTableName, TableType type) throws IOException {
+ Map<String, String> parameters = Maps.newHashMap();
+ parameters.put(
+ serdeConstants.SERIALIZATION_CLASS,
"org.apache.hadoop.hive.serde2.thrift.test.IntString");
+ parameters.put(
+ serdeConstants.SERIALIZATION_FORMAT,
"org.apache.thrift.protocol.TBinaryProtocol");
+
+ SerDeInfo serDeInfo =
+ new SerDeInfo(null,
"org.apache.hadoop.hive.serde2.thrift.ThriftDeserializer", parameters);
+
+ StorageDescriptor sd =
+ new StorageDescriptor(
+ Lists.newArrayList(),
+ temp.resolve(hiveTableName).toString(),
+ "org.apache.hadoop.mapred.TextInputFormat",
+ "org.apache.hadoop.mapred.TextOutputFormat",
+ false,
+ -1,
+ serDeInfo,
+ Lists.newArrayList(),
+ Lists.newArrayList(),
+ Maps.newHashMap());
+
+ return new org.apache.hadoop.hive.metastore.api.Table(
+ hiveTableName,
+ DB_NAME,
+ "test_owner",
+ 0,
+ 0,
+ 0,
+ sd,
+ Lists.newArrayList(),
+ Maps.newHashMap(),
+ "viewOriginalText",
+ "viewExpandedText",
Review Comment:
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
