Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22878 )
Change subject: IMPALA-14016: Add multi-catalog support for local catalog mode ...................................................................... Patch Set 7: (10 comments) Nice work! Left a few comments, but none of them require big changes. http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@102 PS7, Line 102: collect(Collectors.toSet()).stream() Can we replace it with 'distinct()'? http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@118 PS7, Line 118: first nit: please rename to 'firstDuplicate' to make it more verbose http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@122 PS7, Line 122: nit: extra space http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@123 PS7, Line 123: Ambiguous table name Can we test it? Probably it's enough to just sping up multiple REST catalogs for the same warehouse location. http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@185 PS7, Line 185: MetaProvider Do we need 'Metaprovider' here? Above we just have 'provider -> provider.loadFunction(dbName, functionName);' Same goes for methods below. http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@243 PS7, Line 243: ImmutableList.<MetaProvider>builder() : .add(primaryProvider_) : .addAll(secondaryProviders_).build(); nit: could be extracted to a method "getAllProviders()". This could be also used at L264. http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java File fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java: http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java@50 PS7, Line 50: nit: too much indent http://gerrit.cloudera.org:8080/#/c/22878/7/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java File java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/22878/7/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@126 PS7, Line 126: for (int i = 0; i < args.length; i++) { : String arg = args[i]; : switch (arg) { : case "--port": : if (i + 1 < args.length) { : try { : port = Integer.parseInt(args[++i]); : } catch (NumberFormatException e) { : LOG.error( : "Error: --port requires a valid integer value. Got: {}", args[i]); : printUsage(); : System.exit(1); : } : } else { : LOG.error("Error: --port requires a value."); : printUsage(); : System.exit(1); : } : break; : case "--catalog-location": : if (i + 1 < args.length) { : catalogLocation = args[++i]; : } else { : LOG.error("Error: --catalog-location requires a value."); : printUsage(); : System.exit(1); : } : break; : case "--help": : printUsage(); : System.exit(0); : break; : default: : LOG.error("Error: Unknown option: {}", arg); : printUsage(); : System.exit(1); : } : } : : new IcebergRestCatalogTest(port, catalogLocation).start(true); : } : : private static void printUsage() { : LOG.info("Usage: java -jar your-iceberg-rest-catalog.jar [OPTIONS]"); : LOG.info("Options:"); : LOG.info( : " --port <port_number> Port for the REST catalog server (default: " : + DEFAULT_REST_PORT + ")"); : LOG.info( : " --catalog-location <path> Base location for the Hadoop catalog (default: " : + DEFAULT_HADOOP_CATALOG_LOCATION + ")"); : LOG.info(" --help Display this help message"); : } optional: I think it would be shorter and more readable with apache's commons-cli utilities. But most importantly, it would be much more straightforward to add new options. http://gerrit.cloudera.org:8080/#/c/22878/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test: http://gerrit.cloudera.org:8080/#/c/22878/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test@27 PS7, Line 27: SELECT ice_air.iata, ice_air.lat, berg_air.lon from ice.airports_parquet ice_air INNER JOIN berg.airports_parquet berg_air ON berg_air.iata = ice_air.iata and berg_air.iata = "00M" nit: for readability, please use multiple lines http://gerrit.cloudera.org:8080/#/c/22878/7/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22878/7/tests/custom_cluster/test_iceberg_rest_catalog.py@69 PS7, Line 69: nit: continuation lines need +4 indent -- To view, visit http://gerrit.cloudera.org:8080/22878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbdd0f7085345e7954d9f6f264202699182dd1e1 Gerrit-Change-Number: 22878 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 15 Aug 2025 13:32:44 +0000 Gerrit-HasComments: Yes
