Quanlong Huang 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 6: (8 comments) http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java: http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java@128 PS6, Line 128: // Throw error if 'external.table.purge' is manually set. : AnalysisUtils.throwIfNotNull( : tblProperties_.get(KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE), : String.format("Property '%s' cannot be altered for Kudu tables", : KuduTable.TBL_PROP_EXTERNAL_TABLE_PURGE)); I'm not sure if we should remove this restriction. I found some use cases changing this property in Hive: https://docs.cloudera.com/HDPDocuments/HDP3/HDP-3.1.4/using-hiveql/content/hive_drop_external_table_data.html Maybe we should be consistent with Hive to allow this. http://gerrit.cloudera.org:8080/#/c/14750/6/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/6/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@295 PS6, Line 295: a nit: an http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@457 PS6, Line 457: nit: empy line 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" > Ah, I see. Thanks for explaining. The missing piece for me was that this ge Is it harmful to show this? The reason that we skip the existing 4 properties is that they are reflected by the CreateTable syntax. To be specifit: If "EXTERNAL"="true", the sql is "CREATE EXTERNAL TABLE...". If "comment" has a value, it's shown by "CREATE TABLE ... COMMENT ...". If "sort.columns" and "sort.order" have values, they're shown by "CREATE TABLE ... SORT BY ...". Users may use the CreateTable sql to migrate their tables to another Hive cluster. Ignoring the 4 existing properties may be essential to not be duplicated with the syntax clauses. However, for "TRANSLATED_TO_EXTERNAL", should we still show it since we now show the result as "CREATE EXTERNAL TABLE..." instead of "CREATE TABLE ..."? It may be elegant to let users be able to create an identical table in another Hive cluster. http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/main/java/org/apache/impala/catalog/Table.java@199 PS6, Line 199: Returns if the given HMS table is external table or not based on table type or table : * properties. nit: refine this comment? Not quite clear for me... http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java File fe/src/test/java/org/apache/impala/catalog/CatalogTest.java: http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@693 PS6, Line 693: the nit: redudant http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@728 PS6, Line 728: } else { : // in Hive-3 due to HMS translation, this table becomes an external table : assertEquals(TableType.EXTERNAL_TABLE.toString(), : table.getMetaStoreTable().getTableType()); : } Do we still need this? http://gerrit.cloudera.org:8080/#/c/14750/6/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/6/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@263 PS6, Line 263: // unfortunately, we cannot reformat the below lines to make them shorter since If it looks ugly to wrap the lines, we can define the expected string as a var so the lines still have the same ident level. -- 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: 6 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-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Wed, 04 Dec 2019 03:29:43 +0000 Gerrit-HasComments: Yes
