Amos Bird has posted comments on this change. Change subject: IMPALA-1654: General partition exprs in DDL operations. ......................................................................
Patch Set 16: (9 comments) http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetLocationStmt.java: PS16, Line 42: NUM_PARTITION_LOG_LIMIT > nit: Could you put a comment on this that its related to consistent logging Done PS16, Line 83: List<HdfsPartition> sortedPartitions = Lists.newArrayList(partitions); : Collections.sort(sortedPartitions); : List<String> sortedPartitionNames = Lists.newArrayList(); : int num = 0; : for (HdfsPartition partition : sortedPartitions) { : sortedPartitionNames.add(partition.getPartitionName()); : ++num; : if (num == NUM_PARTITION_LOG_LIMIT) break; : } > nit: you can make this more readable with Lists.transform(sortedPartitions. Done http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/analysis/DropStatsStmt.java File fe/src/main/java/com/cloudera/impala/analysis/DropStatsStmt.java: Line 35: PartitionSet partitionSet_ = null; > private final Done Line 90: TableRef tableRef = > move this up and replace the analyzer.getTargetDbName() and analyzer.getTab Done http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java File fe/src/main/java/com/cloudera/impala/catalog/HdfsTable.java: Line 116: import java.io.IOException; > Please fix duplicate imports and the order of imports as well. Should be gr Done Line 1021: if (hdfsPartition != null) { droppedPartitions.add(hdfsPartition); } > Redundant braces for a single line "if" stmt. Done http://gerrit.cloudera.org:8080/#/c/3942/16/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: PS16, Line 1751: numUpdatedPartitions > please add a description for this. Done PS16, Line 1782: try( > nit: double spacing. Done http://gerrit.cloudera.org:8080/#/c/3942/16/testdata/workloads/functional-query/queries/QueryTest/compute-stats.test File testdata/workloads/functional-query/queries/QueryTest/compute-stats.test: Line 1: ==== > Should we add more tests here with PartitionSet? I cannot get e2e tests running correctly in my local env. It's quite hard for me to blind code some tests here :) -- To view, visit http://gerrit.cloudera.org:8080/3942 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f Gerrit-PatchSet: 16 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <amosb...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Amos Bird <amosb...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-HasComments: Yes
