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 7: (5 comments) Minor updates for missed comments. Also additional comments and questions. 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(); > You are correct. The file format is set when the partition is created. This I would argue that regardless of the internal state of the metadata, the users would understand that setting a fileformat to one that does not support delimiters would mean any previous delimiter information is ignored. Also, it would be better if we gave some indication that the command was not going to do anything. Maybe the user meant to make it a sequence file with different delimiters but missed that they typed the wrong fileformat because it accepted the delimiter changes. This is somewhat inconsistent with the "CREATE TABLE" command, but I don't think we'd necessarily want that command to fail just because they passed in delimiter information to an incompatible fileformat. I'm ok with checks or no, but the checks were originally added after Dimitris comments about other cases to consider. I originally was only checking Kudu tables like setFileFormat does. Should it fail for just Kudu tables (like fileformat), or maintain the checks? Or is this something that should be posted somewhere for broader discussion and possibly broader code changes? 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, > This flag should be called reloadTableSchema. The current name is confusing Correction, this flag is actually the FilePartitionMetadata. I now understand why the naming and comments are confusing. When using individual partitions (e.g. PartitionSet params), the code path goes through "bulkAlterPartitions" which marks the partitions dirty, but does not reload the metadata. When not using partition parameters, the metadata (file partition and schema) are loaded based on these two flags. Should a new JIRA be opened to cleanup the inconsistency in how these are handled? Appears the code changes are containable to this one class. The question would be, which way should it be, preloaded, or marked dirty and loaded on next read? http://gerrit.cloudera.org:8080/#/c/8928/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2267 PS4, Line 2267: private boolean alterTableSetRowFormat(Table tbl, > Make sense. Sounds like the naming is simply wrong and we mean "table metad Done http://gerrit.cloudera.org:8080/#/c/8928/4/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/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1348 PS4, Line 1348: 'fox ','jumped ','over~' > Done? Done http://gerrit.cloudera.org:8080/#/c/8928/4/testdata/workloads/functional-query/queries/QueryTest/alter-table.test@1375 PS4, Line 1375: STRING,STRING,STRING > Missed this one? 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: 7 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: John Russell <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Comment-Date: Thu, 11 Jan 2018 04:59:39 +0000 Gerrit-HasComments: Yes
