Dimitris Tsirogiannis 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: (11 comments) 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 Also, you don't need to outline the changes at that level. Instead, for this change you can add an example or the new syntax allowed. 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? http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8928/3/fe/src/main/cup/sql-parser.cup@1007 PS3, Line 1007: // row_format_val cannot be used as it conflicts with cache_op_val. Hm, why would it conflict with cache_op_val? Can you post the error message if you use row_format_val here? 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 rowFormat_), so it's pretty clear what this variable represents. 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 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? Also, what happens if the file format is not text (e.g. parquet)? Should we still allow this to be set? 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 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. 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 be able to create a utility function that takes a RowFormat and a StorageDescriptor and updates the latter. 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). 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 exercise the analysis phase. -- 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: Fri, 05 Jan 2018 23:46:15 +0000 Gerrit-HasComments: Yes
