Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8928 )
Change subject: IMPALA-4323: "SET ROW FORMAT" option added to "ALTER TABLE" command ...................................................................... Patch Set 3: (10 comments) Applied changes as recommended. http://gerrit.cloudera.org:8080/#/c/8928/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8928/3//COMMIT_MSG@10 PS3, Line 10: metadatad. > typo: metadata Done http://gerrit.cloudera.org:8080/#/c/8928/3/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/8928/3/common/thrift/JniCatalog.thrift@269 PS3, Line 269: file > Did you mean 'row' here? Done http://gerrit.cloudera.org:8080/#/c/8928/3/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/3/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@37 PS3, Line 37: this. > nit: you can remove 'this'. All the private fields are ending with '_' (see Done http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@49 PS3, Line 49: rowFormatParams.setPartition_set(getPartitionSet().toThrift()); : } > nit: indentation off by 2 Done http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@59 PS3, Line 59: tbl instanceof KuduTable > I think there are a few more cases to consider here. How about HBase tables Done http://gerrit.cloudera.org:8080/#/c/8928/3/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/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@463 PS3, Line 463: if (rowFormatParams.isSetPartition_set()) { : resultColVal.setString_val( : "Updated " + numUpdatedPartitions.getRef() + " partition(s)."); : } else { : resultColVal.setString_val("Updated table."); : } > nit: indentation is off Done http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2269 PS3, Line 2269: boolean > Add comment about the return value. Also, fix indentation of this function. Done http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2281 PS3, Line 2281: if (rowFormat.getFieldDelimiter() != null) { : sd.getSerdeInfo().putToParameters( : "serialization.format", rowFormat.getFieldDelimiter()); : sd.getSerdeInfo().putToParameters("field.delim", : rowFormat.getFieldDelimiter()); : } : if (rowFormat.getEscapeChar() != null) { : sd.getSerdeInfo().putToParameters("escape.delim", rowFormat.getEscapeChar()); : } : if (rowFormat.getLineDelimiter() != null) { : sd.getSerdeInfo().putToParameters("line.delim", rowFormat.getLineDelimiter()); : } > That block is similar to the block between L2305-2318. I believe you should Done http://gerrit.cloudera.org:8080/#/c/8928/3/tests/query_test/test_delimited_text.py File tests/query_test/test_delimited_text.py: http://gerrit.cloudera.org:8080/#/c/8928/3/tests/query_test/test_delimited_text.py@73 PS3, Line 73: def test_delimited_text_alter(self, vector, unique_database): > I believe most of these tests should be in test_ddl.py (alter-table.test). Done http://gerrit.cloudera.org:8080/#/c/8928/3/tests/query_test/test_delimited_text.py@148 PS3, Line 148: def test_delimited_text_alter_kudu_fail(self, vector, unique_database): : """ Test alter row format statement. IMPALA-4323. Ensure alter statement fails : for Kudu. """ : self.execute_query_expect_success(self.client, """ : create table if not exists %s.kdelimit : (c1 int primary key, c2 string, c3 string) : stored as kudu : """ % unique_database) : # Try to change the field delimiter : ex = self.execute_query_expect_failure(self.client, """ : alter table %s.kdelimit : set row format delimited : fields terminated by ' ' : """ % unique_database) : assert "ALTER TABLE SET ROW FORMAT is not supported on Kudu" in str(ex) > That should be an analysis test, along with any other test cases that exerc Done -- 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: 3 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: John Russell <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Comment-Date: Sat, 06 Jan 2018 18:26:39 +0000 Gerrit-HasComments: Yes
