Alex Behm has posted comments on this change. Change subject: IMPALA-1654: Partition expr in DDL operations. ......................................................................
Patch Set 5: (16 comments) Thanks, Amos! Glad to hear you are willing to work on some of the follow-on tasks :) The code is shaping up well. I still need to do another pass over the tests. 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" 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 move them into PartitionSpecBase.analyze(). You could add a table_ member and similar changes. We should really should try to minimize the code redundancy. Line 92: "partition spec may not contain constant predicates.", e)); e.toSql() Line 98: "column(s): " + e + "."); e.toSql() Line 124: // specified partition specs ignore IF EXISTS by setting partitionShouldExists_ to null. Mention that the given conjuncts are assumed to only reference partition columns. Line 126: Analyzer analyzer, Table table, List<Expr> transformedConjuncts) { transformedConjuncts -> conjuncts Line 133: BinaryPredicate bp = ((BinaryPredicate) e); (BinaryPredicate) e no need for the extra wrapping in parenthesis Line 134: if (bp.getOp() != Operator.EQ) { merge this condition and the one in L138, then we only need to set ignore and break once Line 140: Preconditions.checkState(clusterColumns.contains(partColumn)); Preconditions.checkState(partColumn.getPosition() < table.getNumClusteringColumns()); you could also add a helper function boolean Table.isClusteringColumn(Column c) Line 157: if (ignore) { I think we can combine these two conditions into one if and also a single error message. Something like: IF EXISTS requires a single fully specified partition. Ignoring IF EXISTS since the given partition exprs could select a set of partitions. 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 Returns a list of slot ids that correspond to partition columns. Line 312: public List<SlotId> collectPartitioningColumns() { getPartitionSlots() 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 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 some of these in a follow-on change :) Line 1030: if (partitions.isEmpty()) { I'm thinking we should just ignore this to be consistent with the other DDL operations that can take a partition set. DROP STATS does not have an IF EXISTS clause, so we don't need to handle that either. 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 the other tests in this file you can then remove the execute_serially marker -- 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
