Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/20574 )
Change subject: IMPALA-7131: Support external data sources in LocalCatalog mode ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3664 PS5, Line 3664: Collections.emptyLis > nit: use Collections.emptyList() to avoid creating new array objects. Fixed as suggested http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3675 PS5, Line 3675: Collections.emptyLis > nit: use Collections.emptyList() Fixed as suggested http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1233 PS3, Line 1233: ImmutableList<TDataSource> thriftDataSrcs = loadWithCaching( > I see. I think the DataSource name list could be inconsistent with the Data I got your point. Removed this unused function now. Will consider inconsistent issue if I have to add back this function. http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java: http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@232 PS5, Line 232: LOG.info("Unable to load DataSource objects, ", e); > nit: the stacktrace might be helpful, so let's log it as fixed http://gerrit.cloudera.org:8080/#/c/20574/5/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@244 PS5, Line 244: LOG.info("DataSource not found: " + dsName, e); > nit: log the stacktrace fixed http://gerrit.cloudera.org:8080/#/c/20574/5/testdata/workloads/functional-query/queries/QueryTest/data-source-tables.test File testdata/workloads/functional-query/queries/QueryTest/data-source-tables.test: http://gerrit.cloudera.org:8080/#/c/20574/5/testdata/workloads/functional-query/queries/QueryTest/data-source-tables.test@5 PS5, Line 5: CREATE DATA SOURCE TestGenericDataSource > I see this is created in testdata/bin/create-ext-data-source-table.sql. Are DataSource object is not persistent now. We have to recreate it when creating new external data source table. I also want to add test for dropping DataSource object. Renamed the DataSource name to avoid impact for other tests. http://gerrit.cloudera.org:8080/#/c/20574/5/testdata/workloads/functional-query/queries/QueryTest/data-source-tables.test@184 PS5, Line 184: DROP DATA SOURCE TestGenericDataSource; > Wouldn't this impact other tests that depend on this data source? Or is thi Renamed DataSource name to avoid impact to other tests. -- To view, visit http://gerrit.cloudera.org:8080/20574 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I40841c9be9064ac67771c4d3f5acbb3b552a2e55 Gerrit-Change-Number: 20574 Gerrit-PatchSet: 6 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 25 Oct 2023 18:30:55 +0000 Gerrit-HasComments: Yes
