Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8928 )
Change subject: Added the "SET ROW FORMAT" option to the "ALTER TABLE" command. ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/8928/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8928/1//COMMIT_MSG@7 PS1, Line 7: Added the "SET ROW FORMAT" option to the "ALTER TABLE" command. Adds "IMPALA-4323: " at the start of the line. Adds "Testing:" description. http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/cup/sql-parser.cup@1003 PS1, Line 1003: | KW_ALTER KW_TABLE table_name:table opt_partition_set:partition KW_SET Add some unit tests(white & black) into ParserTest.java. See https://github.com/cloudera/Impala/blob/cdh5-trunk/fe/src/test/java/org/apache/impala/analysis/ParserTest.java#L2317 http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java File fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java: http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@35 PS1, Line 35: nit: use space instead of tab http://gerrit.cloudera.org:8080/#/c/8928/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/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2231 PS1, Line 2231: private boolean alterTableSetFileFormat(Table tbl, I guess you might use the code in this function. I think there are some redundant code. Would you please check there is any room to refactor the both code? I am worry about the following case. Let's assume: 1. Function foo has a bug and a patch should be applied to the function. 2. Function boo is spawned from the function foo. 3. Unfortunately a developer isn't aware of function boo and the bug can exist in boo. http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2269 PS1, Line 2269: private boolean alterTableSetRowFormat(Table tbl, List<List<TPartitionKeyValue>> partitionSet, nit: all lines should be limited to 90 characters in width http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2283 PS1, Line 2283: sd.getSerdeInfo().putToParameters("field.delim", rowFormat.getFieldDelimiter()); nit: exceeds the limit http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2293 PS1, Line 2293: if (tbl instanceof HdfsTable) ((HdfsTable) tbl).addDefaultPartition(msTbl.getSd()); nit: exceeds the limit http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2305 PS1, Line 2305: partition.getSerdeInfo().putToParameters("field.delim", rowFormat.getFieldDelimiter()); nit: exceeds the limit http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2308 PS1, Line 2308: partition.getSerdeInfo().putToParameters("escape.delim", rowFormat.getEscapeChar()); nit: exceeds the limit http://gerrit.cloudera.org:8080/#/c/8928/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2311 PS1, Line 2311: partition.getSerdeInfo().putToParameters("line.delim", rowFormat.getLineDelimiter()); nit: exceeds the limit http://gerrit.cloudera.org:8080/#/c/8928/1/tests/query_test/test_delimited_text.py File tests/query_test/test_delimited_text.py: http://gerrit.cloudera.org:8080/#/c/8928/1/tests/query_test/test_delimited_text.py@164 PS1, Line 164: def test_delimited_text_partitioned(self, vector, unique_database): This test is very similar to test_delimited_text_alter. Do you refactor the both code? -- To view, visit http://gerrit.cloudera.org:8080/8928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I96e347463504915a6f33932552e4d1f61e9b1154 Gerrit-Change-Number: 8928 Gerrit-PatchSet: 1 Gerrit-Owner: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Comment-Date: Wed, 03 Jan 2018 13:56:34 +0000 Gerrit-HasComments: Yes
