Alex Behm 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 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/8928/6/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8928/6/fe/src/main/cup/sql-parser.cup@1051 PS6, Line 1051: row_format_val:row_format Much better, thank you! http://gerrit.cloudera.org:8080/#/c/8928/4/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/4/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@72 PS4, Line 72: affectedPartitions = ((HdfsTable) tbl).getPartitions(); > I would argue that regardless of the internal state of the metadata, the us I feel pretty strongly that table-level delimiter changes should not be prevented by partitions with an incompatible format because those partitions are not impacted at all. Regarding your point "Also, it would be better if we gave some indication that the command was not going to do anything.": * Even if we leave the current partition format check, there is also the case where all partitions have a compatible format, so I'm not sure I understand your argument. In that case the command would similarly "do nothing" but be allowed. * The command does something. It modifies the table metadata which impacts the format of subsequently added partitions. The setFileFormat case is really different because Kudu tables are not backed by a collection of files in a filesystem, but rather managed by a record manager. So it doesn't make sense to change the "file format" of a Kudu table because Kudu is not backed by regular files. When altering the table-level row format, I think it would make most sense to check format compatibility at table level (i.e. the default partition). http://gerrit.cloudera.org:8080/#/c/8928/4/fe/src/main/java/org/apache/impala/analysis/AlterTableSetRowFormatStmt.java@77 PS4, Line 77: throw new AnalysisException("ALTER TABLE SET ROW FORMAT is only supported " + We should include info about the conflicting partition so the user can debug. http://gerrit.cloudera.org:8080/#/c/8928/4/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/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@460 PS4, Line 460: reloadFileMetadata = alterTableSetRowFormat(tbl, > Correction, this flag is actually the FilePartitionMetadata. I now underst Thanks for investigating. I agree it's better to clean this up in a separate change/JIRA. Do you mind filing one? Feel free to assign to yourself if you want to continue working on it :) http://gerrit.cloudera.org:8080/#/c/8928/7/testdata/workloads/functional-query/queries/QueryTest/alter-table.test File testdata/workloads/functional-query/queries/QueryTest/alter-table.test: http://gerrit.cloudera.org:8080/#/c/8928/7/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1404 PS7, Line 1404: # Test select after alter taqble to ensure only no partition changes. typo: taqble to ensure no partition changes http://gerrit.cloudera.org:8080/#/c/8928/7/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1410 PS7, Line 1410: ---- TYPES We should also validate that new partitions are created based on the new table-level row format. -- 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: 7 Gerrit-Owner: Adam Holley <g...@holleyism.com> Gerrit-Reviewer: Adam Holley <g...@holleyism.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: John Russell <jruss...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Comment-Date: Thu, 11 Jan 2018 06:14:27 +0000 Gerrit-HasComments: Yes
