You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "terrytlu (via GitHub)" <gi...@apache.org> on 2026/07/21 12:14:42 UTC
[PR] Hive: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
terrytlu opened a new pull request, #17317:
URL: https://github.com/apache/iceberg/pull/17317
HiveCatalog.listTables previously called HMS getTables and loaded every Table object into the client to filter out non-Iceberg tables. For namespaces with many tables this causes a timeout / OutOfMemoryError on the client side.
Add listIcebergTablesByFilter, which uses HMS get_table_names_by_filter to filter on the `table_type` parameter server-side, so only the matching Iceberg table names are returned (not full Table objects). listTables uses this path by default; the existing list-iceberg-tables-legacy property opts back into the previous client-side behavior.
Closes #17312
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3638795310
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
I followed the exact same pattern that Hive itself uses for this. In IcebergTableUtil.getTableFetcher the metastore filter is built as HIVE_FILTER_FIELD_PARAMS + "table_type like \"ICEBERG\""
https://github.com/apache/hive/blob/ea78454b90040bcde20d545bd1c353fcb61da600/iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/IcebergTableUtil.java#L925
Since the value "ICEBERG" doesn't contain any %/_ wildcard, like behaves exactly like an equality match here — so there's no functional difference from =, and this keeps us consistent with Hive's own usage.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643167345
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
That's a very bad API which exposes these details from the implementation. Please keep using like, but leave a comment that this is for HMS instances which don't support equals for property comparison.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3637581700
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
+ long start = System.currentTimeMillis();
+ List<String> icebergTableNames =
+ clients.run(client -> client.listTableNamesByFilter(database, filter, (short) -1));
+ LOG.debug(
+ "listIcebergTablesByFilter fetched {} iceberg table names from database {} in {} ms",
+ icebergTableNames == null ? 0 : icebergTableNames.size(),
+ database,
+ System.currentTimeMillis() - start);
+
+ if (icebergTableNames == null || icebergTableNames.isEmpty()) {
+ return ImmutableList.of();
+ }
+ return icebergTableNames.stream()
Review Comment:
nit newline after block
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3637575458
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
Review Comment:
It is hard to understand here. Could we inline, like:
```
clients.run(client -> client.listTableNamesByFilter(database, filter, (short) -1 /* no limit */));
```
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3637570613
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
Why `like`?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3639403033
##########
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);
Review Comment:
Do we need all of these parameters?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3642762171
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
Thanks for the question! I dug into this (with some help from AI to trace through the HMS code paths), and here is what I found — happy to be corrected if I missed anything.
You're right that = is generally cheaper than like, but in this specific case the difference seems to be effectively zero, for two reasons:
1. **No index benefit either way**: the filter is applied on TABLE_PARAMS.PARAM_VALUE, which has no index in the HMS schema (the index is on (TBL_ID, PARAM_KEY)). So both = and like end up scanning the same rows, and since we don't use any %/_ wildcards in the pattern, like "ICEBERG" should be semantically equivalent to = "ICEBERG" here.
2. **= can be problematic on some backends**: PARAM_VALUE is a CLOB on Derby and Oracle, where direct equality comparison fails — see HIVE-21614 ("Derby/Oracle does not support CLOB comparisons"). That seems to be exactly why newer HMS versions rewrite EQUALS into LIKE internally (see ExpressionTree.visit(LeafNode), L346-L350)：https://github.com/apache/hive/blob/abf94a5993763523f5249df69dc7ef5ca35bff89/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/parser/ExpressionTree.java#L346-L350 — but older versions like HMS 2.3.x don't have this rewrite, so sending = directly might break against those metastores.
This also seems consistent with what Hive itself does in IcebergTableUtil (L925), which uses like for the same filter.
Given all this, I can see two reasonable ways forward — I'd love your call on which you prefer:
**Option A (current): keep like**. Same performance in practice, and it works consistently across HMS versions and DB backends without any extra config.
**Option B: switch to =, but bring back the list-iceberg-tables-legacy flag**. This keeps the exact-match semantics, while giving users an escape hatch to fall back to the old client-side behavior if they hit issues on older/CLOB-based metastores.
My mild preference leans towards Option A (simpler, one less config to maintain), but I'm genuinely fine either way. What do you think?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3642769861
##########
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 {
Review Comment:
@pvary This was a leftover from reworking the removed legacy-comparison test and duplicated testListTablesFiltersOutNonIcebergTables. Removed.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3639364201
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
Seems like a mistake in the Hive code. "=" is always faster than "like"
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643347073
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1270,77 @@ public void testMetadataHashing(boolean isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path (listIcebergTablesByFilter), which
+ // relies on the HMS-side `get_table_names_by_filter` API, so we assert that non-Iceberg
+ // objects are excluded from listTables when they co-exist in the same database.
+
+ @Test
+ public void testListTablesFiltersOutNonIcebergTables() throws TException {
+ 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);
+ }
+ }
+
+ private org.apache.hadoop.hive.metastore.api.Table createHiveTable(
+ String hiveTableName, TableType type) {
+ // A minimal non-Iceberg table: all that matters for these tests is that it does not carry
Review Comment:
Removed the comment and renamed the helper to createNonIcebergTable. Thanks!
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5066827867
> Thanks for the research @terrytlu! Left a few final comments, but overall, we are getting ready
Thanks @pvary! Addressed all the final comments, pushed in the latest commit. Let me know if there's anything else!
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5045352182
How many tables do you have to get an OOM?
How many tables do you have to get a timeout?
Why do we want to have a new config for this?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3639404495
##########
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:
What is this text?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3639388907
##########
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 {
Review Comment:
What's the difference between this and `testListTablesFiltersOutNonIcebergTables`?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643201325
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1270,77 @@ public void testMetadataHashing(boolean isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path (listIcebergTablesByFilter), which
+ // relies on the HMS-side `get_table_names_by_filter` API, so we assert that non-Iceberg
+ // objects are excluded from listTables when they co-exist in the same database.
+
+ @Test
+ public void testListTablesFiltersOutNonIcebergTables() throws TException {
+ 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);
+ }
+ }
+
+ private org.apache.hadoop.hive.metastore.api.Table createHiveTable(
+ String hiveTableName, TableType type) {
+ // A minimal non-Iceberg table: all that matters for these tests is that it does not carry
Review Comment:
Remove this comment. Maybe rename the method to `createNonIcebergTable`
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5059480672
> > **3. New config** — `list-iceberg-tables-legacy` is just a defensive fallback. The old path has run stably in production for a long time, so the flag lets users revert if the new server-side-filter path hits an unexpected issue. It's off by default.
>
> Do we expect any issues with the new strategy? Seems like a quite nice optimisation, without any obvious user facing drawback. Also we prefer as few configs as possible.
@pvary I don't expect any issue with the new strategy for listTables: get_table_names_by_filter is a long-standing HMS API and the params filter works reliably for tables. The only caveat (unreliable table_type filtering for VIRTUAL_VIEW on Hive 2.3.x) affects listViews only, which keeps the client-side resolution anyway. I've drop the list-iceberg-tables-legacy config and always use the server-side filter.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643344778
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
Agreed — kept 'like' and added a comment explaining it's an intentional workaround for HMS backends (e.g. Derby/Oracle, where PARAM_VALUE is a CLOB) that don't support = on property values (HIVE-21614). Thanks for the guidance!
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
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: 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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5053595131
> How many tables do you have to get an OOM? How many tables do you have to get a timeout, and which component returns the timeout? Why do we want to have a new config for this?
Thanks for the questions, @pvary Let me address them point by point:
**1. OOM** — Theoretical so far; we haven't hit it in production. The old path loads a full `Table` object per table into client memory via `getTableObjectsByName`, so memory grows unbounded with namespace size — that's the risk.
**2. Timeout** — Covered in #17312: a namespace with ~46,000 tables still timed out even at `hive.metastore.client.socket.timeout=900s`. The timeout is raised on the HMS client side, since serializing/transferring that many full `Table` objects exceeds the socket timeout.
**3. New config** — `list-iceberg-tables-legacy` is just a defensive fallback. The old path has run stably in production for a long time, so the flag lets users revert if the new server-side-filter path hits an unexpected issue. It's off by default. Happy to drop the flag and keep only the new path if you prefer.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3642774607
##########
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);
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: 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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5068241602
> Did a small simplification on the private method javadoc. +1 pending tests
Thanks for the review and for the javadoc cleanup, @pvary! CI is all green now.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5066607734
Thanks for the research @terrytlu!
Left a few final comments, but overall, we are getting ready
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #17317:
URL: https://github.com/apache/iceberg/pull/17317
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643167345
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
That's a very bad API design which exposes these details from the implementation. Please keep using like, but leave a comment that this is for HMS instances which don't support equals for property comparison.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3638801876
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
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: 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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3642762171
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
Review Comment:
@pvary Thanks for the question! I dug into this (with some help from AI to trace through the HMS code paths), and here is what I found — happy to be corrected if I missed anything.
You're right that = is generally cheaper than like, but in this specific case the difference seems to be effectively zero, for two reasons:
1. **No index benefit either way**: the filter is applied on TABLE_PARAMS.PARAM_VALUE, which has no index in the HMS schema (the index is on (TBL_ID, PARAM_KEY)). So both = and like end up scanning the same rows, and since we don't use any %/_ wildcards in the pattern, like "ICEBERG" should be semantically equivalent to = "ICEBERG" here.
2. **= can be problematic on some backends**: PARAM_VALUE is a CLOB on Derby and Oracle, where direct equality comparison fails — see HIVE-21614 ("Derby/Oracle does not support CLOB comparisons"). That seems to be exactly why newer HMS versions rewrite EQUALS into LIKE internally (see ExpressionTree.visit(LeafNode), L346-L350)：https://github.com/apache/hive/blob/abf94a5993763523f5249df69dc7ef5ca35bff89/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/parser/ExpressionTree.java#L346-L350 — but older versions like HMS 2.3.x don't have this rewrite, so sending = directly might break against those metastores.
This also seems consistent with what Hive itself does in IcebergTableUtil (L925), which uses like for the same filter.
Given all this, I can see two reasonable ways forward — I'd love your call on which you prefer:
**Option A (current): keep like**. Same performance in practice, and it works consistently across HMS versions and DB backends without any extra config.
**Option B: switch to =, but bring back the list-iceberg-tables-legacy flag**. This keeps the exact-match semantics, while giving users an escape hatch to fall back to the old client-side behavior if they hit issues on older/CLOB-based metastores.
My mild preference leans towards Option A (simpler, one less config to maintain), but I'm genuinely fine either way. What do you think?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643188442
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1270,77 @@ public void testMetadataHashing(boolean isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path (listIcebergTablesByFilter), which
Review Comment:
We can remove this comment. The tests are sort and self-explanatory.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3639401308
##########
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:
What is this class?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5057621678
> **1. OOM** — Theoretical so far; we haven't hit it in production. The old path loads a full `Table` object per table into client memory via `getTableObjectsByName`, so memory grows unbounded with namespace size — that's the risk.
Thanks for clarifying
> **2. Timeout** — Covered in #17312: a namespace with ~46,000 tables still timed out even at `hive.metastore.client.socket.timeout=900s`. The timeout is raised on the HMS client side, since serializing/transferring that many full `Table` objects exceeds the socket timeout. With the new server-side filter, the same call returns in about 5 seconds.
Based on the description of #17312, the timeout is in the `getTableObjectsByName` call.
The issue is that we send too many tables in a single call, and HMS is not able to respond in a timely manner.
> **3. New config** — `list-iceberg-tables-legacy` is just a defensive fallback. The old path has run stably in production for a long time, so the flag lets users revert if the new server-side-filter path hits an unexpected issue. It's off by default.
Do we expect any issues with the new strategy? Seems like a quite nice optimisation, without any obvious user facing drawback. Also we prefer as few configs as possible.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5041135538
cc @pvary @nastra could you please review this when you have time? It fixes #17312 (listTables timeout/OOM on large namespaces).
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3637579330
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
+ long start = System.currentTimeMillis();
+ List<String> icebergTableNames =
+ clients.run(client -> client.listTableNamesByFilter(database, filter, (short) -1));
+ LOG.debug(
+ "listIcebergTablesByFilter fetched {} iceberg table names from database {} in {} ms",
+ icebergTableNames == null ? 0 : icebergTableNames.size(),
+ database,
+ System.currentTimeMillis() - start);
+
+ if (icebergTableNames == null || icebergTableNames.isEmpty()) {
Review Comment:
Is it possible to get `null`?
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3643345760
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -1266,4 +1270,77 @@ public void testMetadataHashing(boolean isTableEncrypted) {
assertThat(base64EncodedHash).isNull();
}
}
+
+ // The tests below exercise the Iceberg-only listing path (listIcebergTablesByFilter), which
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: 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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3638804287
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
+ long start = System.currentTimeMillis();
+ List<String> icebergTableNames =
+ clients.run(client -> client.listTableNamesByFilter(database, filter, (short) -1));
+ LOG.debug(
+ "listIcebergTablesByFilter fetched {} iceberg table names from database {} in {} ms",
+ icebergTableNames == null ? 0 : icebergTableNames.size(),
+ database,
+ System.currentTimeMillis() - start);
+
+ if (icebergTableNames == null || icebergTableNames.isEmpty()) {
Review Comment:
Good catch — you're right, it can't be null. The result comes from the Thrift get_table_names_by_filter call, and a Thrift list is never null after deserialization (at most an empty list). The check was purely defensive but unreachable, so I've removed it.
--
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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "terrytlu (via GitHub)" <gi...@apache.org>.
terrytlu commented on code in PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#discussion_r3638797967
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -354,6 +370,48 @@ private List<TableIdentifier> listIcebergTables(
.collect(Collectors.toList());
}
+ /**
+ * Lists the {@link TableIdentifier} of all Iceberg tables under the given database.
+ *
+ * <p>Implementation uses the HMS {@code get_table_names_by_filter} API, filtering on {@code
+ * parameters.table_type} on the server side so that only matching table names (rather than full
+ * Table objects) are returned.
+ *
+ * @param namespace the namespace corresponding to the database
+ * @param tableTypeValue the value of {@code parameters.table_type} to filter on, e.g. {@link
+ * BaseMetastoreTableOperations#ICEBERG_TABLE_TYPE_VALUE} for tables. HMS normalizes this
+ * value to upper case when persisting it, so the filter value is upper-cased to match.
+ */
+ private List<TableIdentifier> listIcebergTablesByFilter(
+ Namespace namespace, String tableTypeValue) throws TException, InterruptedException {
+ String database = namespace.level(0);
+ // HMS normalizes the `table_type` parameter value to upper case when persisting it
+ // (e.g. "iceberg" is stored as "ICEBERG"), so the filter value must be upper-cased to match.
+ String filter =
+ hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS
+ + BaseMetastoreTableOperations.TABLE_TYPE_PROP
+ + " like \""
+ + tableTypeValue.toUpperCase(Locale.ROOT)
+ + "\"";
+
+ // maxTables = -1 means no limit on the number of returned tables.
+ long start = System.currentTimeMillis();
+ List<String> icebergTableNames =
+ clients.run(client -> client.listTableNamesByFilter(database, filter, (short) -1));
+ LOG.debug(
+ "listIcebergTablesByFilter fetched {} iceberg table names from database {} in {} ms",
+ icebergTableNames == null ? 0 : icebergTableNames.size(),
+ database,
+ System.currentTimeMillis() - start);
+
+ if (icebergTableNames == null || icebergTableNames.isEmpty()) {
+ return ImmutableList.of();
+ }
+ return icebergTableNames.stream()
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: 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: Use server-side filter to list Iceberg tables in HiveCatalog [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #17317:
URL: https://github.com/apache/iceberg/pull/17317#issuecomment-5068324941
Merged to main.
Thanks @terrytlu for the PR!
--
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