Vihang Karajgaonkar 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: (14 comments) http://gerrit.cloudera.org:8080/#/c/14750/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14750/6//COMMIT_MSG@9 PS6, Line 9: In HMS-3 the translation layer converts a managed kudu table into a external kudu table : and adds additional table property 'external.table.purge' to 'true'. This means any : installation which is using HMS-3 (or a Hive version which has HIVE-22158) will always : create Kudu tables as external tables. This is problematic since the the output of show : create table will now be different and may confuse the users. : : In order to improve the user experience of such synchronized tables (external tables with : external.table.purge property set to true), this patch adds support in Impala to create : external Kudu tables. Previous versions of Impala disallowed creating a external Kudu : table if the Kudu table did not exist. 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. The command will error out if the Kudu table already : exists. However, this applies to only the synchronized tables. Previous way to create a : pure external table behaves the same. > Nit: We usually keep our commit messages to a width of about 70 characters I will modify my git settings to have the line width as 70. Fixed this manually now. 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 Done 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 Done 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" > Is it harmful to show this? The reason that we skip the existing 4 properti As per my understanding, this property is internal to HMS which is set when the table is translated. According to me it would be confusing to the user if we show it in the output of show create table since its not really in user's control to translate the table. The HMS may or may not be configured to do the translation. In case HMS is not configured for transformation, having this property seen as TRANSLATED_TO_EXTERNAL=true may be misleading to the user. 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... sorry for the typos. Let me know if the updated one is clearer. http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@71 PS6, Line 71: super.AnalysisError(appendSynchronizedTblProps(stmt, isExternalPurgeTbl), expectedError); > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@101 PS6, Line 101: AnalyzesOk("create table tab (x int, y int, primary key(x, y)) partition by " + : "range(x, y) (partition value = (1+1, 2+2), partition value = ((1+1+1)+1, 10), " + Yes, the indentation was changed by Intellij since the addition of new boolean parameter increases the max line-width. I redid these changes so that diffs look cleaner. http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@221 PS6, Line 221: "(PARTITION VALUE = 'abc')' is not a key column. Only key columns can be used " > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@277 PS6, Line 277: "partition by range(a, b) (partition (0, 0) < values <= (1, 1)) stored as kudu", > line too long (92 > 90) Done http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@286 PS6, Line 286: "partitioning columns: (1 vs 2). Range partition: 'PARTITION 0 < VALUES <= 1'", > line too long (91 > 90) Done 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 Done 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? Thanks for catching this. Removed it. 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 I updated the file based on Joe's suggestion. Can you please take a relook? Not sure if I understood fully what your suggestion is. http://gerrit.cloudera.org:8080/#/c/14750/6/fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java@267 PS6, Line 267: Assert.assertThat(output, CoreMatchers.startsWith( : "CREATE EXTERNAL TABLE functional_kudu" : + ".alltypes (\n" + : " id INT NOT NULL ENCODING AUTO_ENCODING COMPRESSION DEFAULT_COMPRESSION,\n" + > Nit: My preference would be to keep the whitespace and formatting very simi makes sense. Done -- 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: Tue, 10 Dec 2019 22:38:50 +0000 Gerrit-HasComments: Yes
