Amos Bird has posted comments on this change. Change subject: IMPALA-1654: Partition expr in DDL operations. ......................................................................
Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/3942/5/fe/src/main/java/com/cloudera/impala/analysis/ComputeStatsStmt.java File fe/src/main/java/com/cloudera/impala/analysis/ComputeStatsStmt.java: Line 360: for(HdfsPartition targetPartition : targetPartitions){ > space after "for" Done http://gerrit.cloudera.org:8080/#/c/3942/5/fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSet.java: Line 62: Table table = analyzer.getTable(tableName_, privilegeRequirement_, false); > L62 - L74 are also common between PartitionSpec and PartitionSet. Please mo Done Line 92: "partition spec may not contain constant predicates.", e)); > e.toSql() Done Line 98: "column(s): " + e + "."); > e.toSql() Done Line 124: // specified partition specs ignore IF EXISTS by setting partitionShouldExists_ to null. > Mention that the given conjuncts are assumed to only reference partition co Done Line 126: Analyzer analyzer, Table table, List<Expr> transformedConjuncts) { > transformedConjuncts -> conjuncts Done Line 133: BinaryPredicate bp = ((BinaryPredicate) e); > (BinaryPredicate) e Done Line 134: if (bp.getOp() != Operator.EQ) { > merge this condition and the one in L138, then we only need to set ignore a Done Line 140: Preconditions.checkState(clusterColumns.contains(partColumn)); > Preconditions.checkState(partColumn.getPosition() < table.getNumClusteringC Done Line 157: if (ignore) { > I think we can combine these two conditions into one if and also a single e Done http://gerrit.cloudera.org:8080/#/c/3942/5/fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java File fe/src/main/java/com/cloudera/impala/analysis/TupleDescriptor.java: Line 311: // returns all the partitioning columns from TupleDescriptor. > Use /** */ style comment Done Line 312: public List<SlotId> collectPartitioningColumns() { > getPartitionSlots() Done http://gerrit.cloudera.org:8080/#/c/3942/5/fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java: Line 76: private List<SlotId> partitionSlots_; > final Done http://gerrit.cloudera.org:8080/#/c/3942/5/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 1024: // TODO: Report the number of updated partitions/columns to the user? > Add a TODO here to bulk alter the partitions. I'm hoping you can address so Done Line 1030: if (partitions.isEmpty()) { > I'm thinking we should just ignore this to be consistent with the other DDL Done http://gerrit.cloudera.org:8080/#/c/3942/5/tests/metadata/test_ddl.py File tests/metadata/test_ddl.py: Line 497: def test_partition_ddl_predicates(self, vector): > now that you've rebased, you can use the unique_database fixture like in th Done -- 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Amos Bird <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Amos Bird <[email protected]> Gerrit-HasComments: Yes
