Copilot commented on code in PR #11193:
URL: https://github.com/apache/gravitino/pull/11193#discussion_r3285154979
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -430,19 +411,53 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
}
}
- private static String getIcebergAndPaimonFilter() {
+ /**
+ * Best-effort removal of non-Hive tables (Iceberg, Paimon, Hudi) from
{@code allTables} using the
+ * HMS server-side {@code listTableNamesByFilter} API. This API only
supports exact-key lookups on
+ * dot-free parameter keys, so tables whose only marker is a dotted key
(e.g. Spark-managed Hudi
+ * tables exposing only {@code spark.sql.sources.provider=hudi}) cannot be
filtered out here; see
+ * the {@code list-all-tables} catalog property for the documented
limitation. We prefer this over
+ * {@code getTableObjectsByName} which materializes every Table and is slow
on databases with many
+ * tables.
+ */
+ private void filterOutNonHiveTables(String database, List<String> allTables)
+ throws InterruptedException {
+ List<String> icebergAndPaimonTables =
+ clientPool.run(
+ c ->
+ c.listTableNamesByFilter(
+ catalogName, database, buildIcebergAndPaimonFilter(),
MAX_TABLES));
+ allTables.removeAll(icebergAndPaimonTables);
+
+ // HoodieHiveSyncTool sets `provider=hudi` only on the base table; derived
`_ro` / `_rt`
+ // tables carry only dotted keys, so we strip them by exact name match
against the base list.
+ List<String> hudiBaseTables =
+ clientPool.run(
+ c ->
+ c.listTableNamesByFilter(
+ catalogName, database, buildHudiBaseTableFilter(),
MAX_TABLES));
+ removeHudiDerivedTables(allTables, hudiBaseTables);
+ }
+
+ private static String buildIcebergAndPaimonFilter() {
String icebergFilter = String.format("%stable_type like \"ICEBERG\"",
HIVE_FILTER_FIELD_PARAMS);
String paimonFilter = String.format("%stable_type like \"PAIMON\"",
HIVE_FILTER_FIELD_PARAMS);
return String.format("%s or %s", icebergFilter, paimonFilter);
}
- private void removeHudiTables(List<String> allTables, List<String>
hudiTables) {
- for (String hudiTable : hudiTables) {
+ private static String buildHudiBaseTableFilter() {
+ return String.format("%sprovider like \"hudi\"", HIVE_FILTER_FIELD_PARAMS);
+ }
+
+ /**
+ * Removes Hudi base tables together with their derived read-optimized
({@code _ro}) and real-time
+ * ({@code _rt}) tables. Exact name match is used because {@code startsWith}
would incorrectly
+ * drop unrelated tables such as {@code <base>_root}.
+ */
+ private void removeHudiDerivedTables(List<String> allTables, List<String>
hudiBaseTables) {
+ for (String hudiBase : hudiBaseTables) {
allTables.removeIf(
- t ->
- t.equals(hudiTable)
- || t.startsWith(hudiTable + "_ro")
- || t.startsWith(hudiTable + "_rt"));
+ t -> t.equals(hudiBase) || t.equals(hudiBase + "_ro") ||
t.equals(hudiBase + "_rt"));
}
}
Review Comment:
`removeHudiDerivedTables` currently calls `allTables.removeIf(...)` once per
Hudi base table, which repeatedly scans the full `allTables` list
(O(|allTables| * |hudiBaseTables|)). This can become a noticeable CPU cost on
large schemas. Build a set of names to remove and run a single `removeIf` pass
instead.
##########
docs/apache-hive-catalog.md:
##########
@@ -44,10 +44,32 @@ Besides the [common catalog
properties](./gravitino-server-config.md#apache-grav
| `default.catalog` | The default catalog name for the
Hive3 metastore backend; this configuration is ignored when using a Hive2
metastore.
| hive | No
| 1.1.0 |
:::note
-For `list-all-tables=false`, the Hive catalog will filter out:
-- Iceberg tables by table property `table_type=ICEBERG`
-- Paimon tables by table property `table_type=PAIMON`
-- Hudi tables by table property `provider=hudi`
+When `list-all-tables=false`, the Hive catalog removes the following on a
best-effort basis:
Review Comment:
The `list-all-tables` table entry above still uses the old description and
doesn’t mention the best-effort filtering behavior / known limitation that’s
now documented in the note below (and in the Java property metadata). This can
confuse users reading only the properties table.
##########
catalogs/catalog-hive/src/main/java/org/apache/gravitino/catalog/hive/HiveCatalogOperations.java:
##########
@@ -430,19 +411,53 @@ public NameIdentifier[] listTables(Namespace namespace)
throws NoSuchSchemaExcep
}
}
- private static String getIcebergAndPaimonFilter() {
+ /**
+ * Best-effort removal of non-Hive tables (Iceberg, Paimon, Hudi) from
{@code allTables} using the
+ * HMS server-side {@code listTableNamesByFilter} API. This API only
supports exact-key lookups on
+ * dot-free parameter keys, so tables whose only marker is a dotted key
(e.g. Spark-managed Hudi
+ * tables exposing only {@code spark.sql.sources.provider=hudi}) cannot be
filtered out here; see
+ * the {@code list-all-tables} catalog property for the documented
limitation. We prefer this over
+ * {@code getTableObjectsByName} which materializes every Table and is slow
on databases with many
+ * tables.
+ */
+ private void filterOutNonHiveTables(String database, List<String> allTables)
+ throws InterruptedException {
+ List<String> icebergAndPaimonTables =
+ clientPool.run(
+ c ->
+ c.listTableNamesByFilter(
+ catalogName, database, buildIcebergAndPaimonFilter(),
MAX_TABLES));
+ allTables.removeAll(icebergAndPaimonTables);
+
+ // HoodieHiveSyncTool sets `provider=hudi` only on the base table; derived
`_ro` / `_rt`
+ // tables carry only dotted keys, so we strip them by exact name match
against the base list.
+ List<String> hudiBaseTables =
+ clientPool.run(
+ c ->
+ c.listTableNamesByFilter(
+ catalogName, database, buildHudiBaseTableFilter(),
MAX_TABLES));
+ removeHudiDerivedTables(allTables, hudiBaseTables);
+ }
+
+ private static String buildIcebergAndPaimonFilter() {
String icebergFilter = String.format("%stable_type like \"ICEBERG\"",
HIVE_FILTER_FIELD_PARAMS);
String paimonFilter = String.format("%stable_type like \"PAIMON\"",
HIVE_FILTER_FIELD_PARAMS);
return String.format("%s or %s", icebergFilter, paimonFilter);
}
- private void removeHudiTables(List<String> allTables, List<String>
hudiTables) {
- for (String hudiTable : hudiTables) {
+ private static String buildHudiBaseTableFilter() {
+ return String.format("%sprovider like \"hudi\"", HIVE_FILTER_FIELD_PARAMS);
+ }
+
+ /**
+ * Removes Hudi base tables together with their derived read-optimized
({@code _ro}) and real-time
+ * ({@code _rt}) tables. Exact name match is used because {@code startsWith}
would incorrectly
+ * drop unrelated tables such as {@code <base>_root}.
+ */
+ private void removeHudiDerivedTables(List<String> allTables, List<String>
hudiBaseTables) {
+ for (String hudiBase : hudiBaseTables) {
allTables.removeIf(
- t ->
- t.equals(hudiTable)
- || t.startsWith(hudiTable + "_ro")
- || t.startsWith(hudiTable + "_rt"));
+ t -> t.equals(hudiBase) || t.equals(hudiBase + "_ro") ||
t.equals(hudiBase + "_rt"));
Review Comment:
This change fixes a correctness bug (avoids over-removal of tables like
`<base>_root` by switching from `startsWith` to exact matches), but there’s no
regression test covering the new behavior. Please add a unit test that ensures
only `<base>`, `<base>_ro`, and `<base>_rt` are removed and that
similarly-prefixed tables (e.g. `<base>_root`) remain listed.
--
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]
