Quanlong Huang 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 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/20574/3/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/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3664 PS3, Line 3664: return createGetPartialCatalogObjectError( : CatalogLookupStatus.DATA_SOURCE_NOT_FOUND); Why we can't return an empty list for this case? http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3794 PS3, Line 3794: tables nit: this should be "dbs". We don't send table names here. http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@3809 PS3, Line 3809: // TODO(todd) implement other global information. Just curious, any other global info we are missing? It'd be nice to file specifit JIRAs. 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@671 PS3, Line 671: req.object_desc.data_source = new TDataSource(); : if (dsName != null) { : req.object_desc.data_source.name = dsName; : } else { : req.object_desc.data_source.name = ""; : } : req.object_desc.data_source.hdfs_location = ""; : req.object_desc.data_source.class_name = ""; : req.object_desc.data_source.api_version = ""; nit: these can be simplified as if (dsName == null) dsName = ""; req.object_desc.setData_source(new TDataSource(dsName, "", "", "")); Is there any error if we don't set the fields of 'hdfs_location', 'class_name' and 'api_version' in the request? http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1233 PS3, Line 1233: public List<String> loadDataSourceNames() throws TException { This seems unused. http://gerrit.cloudera.org:8080/#/c/20574/3/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/3/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@207 PS3, Line 207: } catch (Exception e) { Can we ignore all the exceptions? I'm not sure what exceptions will be thrown here. Can we log them? http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@218 PS3, Line 218: } catch (Exception e) { Need logs for the exception here too http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java: http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@20 PS3, Line 20: import java.util.List; nit: unused import http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@23 PS3, Line 23: import org.apache.hadoop.hive.metastore.TableType; nit: unused import http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@31 PS3, Line 31: import org.apache.impala.thrift.TCatalogObjectType; nit: unused import http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@37 PS3, Line 37: import org.apache.impala.thrift.TTable; nit: unused import http://gerrit.cloudera.org:8080/#/c/20574/3/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@103 PS3, Line 103: the nit: duplicated "the" -- 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: 3 Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <gsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Wenzhe Zhou <wz...@cloudera.com> Gerrit-Comment-Date: Wed, 18 Oct 2023 06:45:23 +0000 Gerrit-HasComments: Yes
