Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/21016 )
Change subject: IMPALA-12793: Create JDBC table without data source ...................................................................... Patch Set 5: (5 comments) Thanks for putting this together, this is looking good http://gerrit.cloudera.org:8080/#/c/21016/5/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java File fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java: http://gerrit.cloudera.org:8080/#/c/21016/5/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java@180 PS5, Line 180: if (!found) { : throw new ImpalaRuntimeException( : String.format("Invalid table property for a JDBC table: %s.", key)); : } I'm not sure we want to restrict the table properties to only the ones that the JDBC data source supports. http://gerrit.cloudera.org:8080/#/c/21016/5/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java@285 PS5, Line 285: Strings.isNullOrEmpty(dataSourceName) Nit: For the JDBC table case, can we pass in dataSourceName as IMPALA_BUILTIN_JDBC_DATASOURCE and check dataSourceName == IMPALA_BUILTIN_JDBC_DATASOURCE here? I think it would make the code a little bit clearer. http://gerrit.cloudera.org:8080/#/c/21016/5/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/21016/5/fe/src/main/java/org/apache/impala/catalog/local/LocalDataSourceTable.java@123 PS5, Line 123: Strings.isNullOrEmpty(dataSourceName) See comment from DataSourceTable.java http://gerrit.cloudera.org:8080/#/c/21016/5/fe/src/main/java/org/apache/impala/util/JsonUtil.java File fe/src/main/java/org/apache/impala/util/JsonUtil.java: http://gerrit.cloudera.org:8080/#/c/21016/5/fe/src/main/java/org/apache/impala/util/JsonUtil.java@43 PS5, Line 43: JSon Nit: I would prefer either "Json" (lowercase s) or "JSON" (all caps). Between those, I think "Json" fits the context. http://gerrit.cloudera.org:8080/#/c/21016/5/tests/custom_cluster/test_ext_data_sources.py File tests/custom_cluster/test_ext_data_sources.py: http://gerrit.cloudera.org:8080/#/c/21016/5/tests/custom_cluster/test_ext_data_sources.py@a66 PS5, Line 66: : : : : : : : : : : Do we need to remove this? -- To view, visit http://gerrit.cloudera.org:8080/21016 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I765aa86b430246786ad85ab6857cefaf4332c920 Gerrit-Change-Number: 21016 Gerrit-PatchSet: 5 Gerrit-Owner: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Thu, 22 Feb 2024 19:21:25 +0000 Gerrit-HasComments: Yes
