Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14750 )
Change subject: IMPALA-9092: Add support for creating external Kudu table ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/14750/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14750/1//COMMIT_MSG@18 PS1, Line 18: After this patch, Impala will check if the Kudu : table exists and if it does not it will create a Kudu table based on the schema provided : in the create table statement. I wonder if it's worth adding some kind of validation that the Kudu schema matches the Impala schema. http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@292 PS1, Line 292: // this is a legacy external table nit: Not sure "legacy" is the right term here. These kinds of external tables still exist though, right? They're just non-synchronized. http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java File fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java@75 PS1, Line 75: "TRANSLATED_TO_EXTERNAL" Sorry if this is a newbie question, but how is this used? http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2212 PS1, Line 2212: it already exists. If the Kudu table is not present, we will create it. nit: spacing alignment's a bit off http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2225 PS1, Line 2225: if (!KuduTable.isSynchronizedTable(newTable)) { : // if this is not a synchronized table, we assume that the table must be existing : // in kudu and use the column spec from Kudu : KuduCatalogOpExecutor.populateExternalTableColsFromKudu(newTable); : } else { : // if this is a synchronized table (managed or external.purge table) then we : // create it in Kudu first : KuduCatalogOpExecutor.createSynchronizedTable(newTable, params); : } : // if the table is not synchronized table (means it is a external table), we need to : // create the HMS table. Otherwise, we create the Kudu table first and then : // the HMS table above. Additionally, in the second case, KuduHMSIntegration : // creates a HMS table for you and hence we don't need to create the HMS table here : boolean createHMSTable = : !KuduTable.isSynchronizedTable(newTable) || !isKuduHmsIntegrationEnabled(newTable); nit: this might be more self-explanatory as boolean createHMSTable; if (!KudTable.isSynchronizedTable(newTable)) { populateExternalTableColsFromKudu(...); createHMSTable = true; } else { createSynchronizedTable(...); createHMSTable = !isKuduHmsIntegrationEnabled() } http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java@98 PS1, Line 98: // if this is a external purge table which user wants us to create, the table : // name property must not be empty : Preconditions.checkState(!Strings.isNullOrEmpty(kuduTableName)); I wonder if we should be somehow validating the input schema, and either be throwing an error if they don't match, or disregarding the input schema and instead using the existing Kudu schema. http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java File fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@252 PS1, Line 252: @Ignore("Ignored until IMPALA-9092 is fixed") Shouldn't be ignored anymore? http://gerrit.cloudera.org:8080/#/c/14750/1/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@265 PS1, Line 265: "CREATE EXTERNAL TABLE functional_kudu.alltypes" + "\n" + : "STORED AS KUDU" + "\n" + : "TBLPROPERTIES")); >The output of show create table on a synchronized table will display the full >column and partition spec similar to the managed tables. I think I'm lacking some Impala context. From the commit message, I thought the two outputs would be basically the same except for the EXTERNAL and purge property. Is that not what that meant, or is this not what you were referring to? -- To view, visit http://gerrit.cloudera.org:8080/14750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I76f81d41db0cf2269ee1b365857164a43677e14d Gerrit-Change-Number: 14750 Gerrit-PatchSet: 1 Gerrit-Owner: Vihang Karajgaonkar <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Wed, 20 Nov 2019 20:58:16 +0000 Gerrit-HasComments: Yes
