Joe McDonnell 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: (3 comments) This matches what I was expecting, and it looks good. I have a few nits about line formatting that I'd like you to fix. Beyond that, I may have comments later about the RESULTS-HIVE / RESULTS-HIVE-3 way of arranging the tests in show-create-table.test. I'm leaning towards this being the right way to do it, but I have to think on it some more. 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 or so. When you use "git log", it adds a couple characters of space, so if the lines get too long, the output in the shell wraps and makes it harder to read. We don't strictly enforce it for everything. Output from a tool or for performance results often exceeds this width. Etc. 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@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), " + Nit: I'm seeing lots of weird indentation changes in this file. For example, here it is adding an extra level of indentation that I think is not warranted (for the "range(x,y)..." line and the one below). Unless you are changing the line or something requires you to change the indentation, keep the indentation from before. This applies across this whole file. I think most of these are probably auto-indented by a tool. 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@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 similar to before. To be specific: 1. Keep "CREATE EXTERNAL TABLE functional_kudu.alltypes (\n" on one line. 2. Use the indentation from before (except that it will have 2 extra spaces of indentation due to the if). That means that the quotes line up, like: "CREATE TABLE..." " id INT NOT NULL..." " bool_col BOOLEAN..." rather than "CREATE TABLE..." + " id INT NOT NULL..." + " bool_col BOOLEAN NULL..." I hope this makes sense. There will still be some long lines, and I'm less concerned about that. -- 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 01:51:07 +0000 Gerrit-HasComments: Yes
