Jim Apple has posted comments on this change. Change subject: IMPALA-1654: Support general predicates in most partition DDL operations. ......................................................................
Patch Set 3: (17 comments) Still getting my head around this. http://gerrit.cloudera.org:8080/#/c/1563/2//COMMIT_MSG Commit Message: Please expand the commit message to describe the format of the new statements and the restrictions on them. http://gerrit.cloudera.org:8080/#/c/1563/3/be/src/service/query-exec-state.cc File be/src/service/query-exec-state.cc: PS3, Line 533: request_result_set_.reset(new vector<TResultRow>); : request_result_set_->assign( : ddl_resp->result_set.rows.begin(), ddl_resp->result_set.rows.end()); These two statement can be combined into one using the constructor of std::vector that takes two iterators. http://gerrit.cloudera.org:8080/#/c/1563/3/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: Line 145: // by COMPUTE STATS and ALTER TABLE. All ALTER TABLE, or just some types of alterations? http://gerrit.cloudera.org:8080/#/c/1563/2/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: Line 111: // If set, delete the stats only for a particular partition, but do not recompute the "particular set of partitions" Line 194: // Parameters for ALTER TABLE rename commands "id list of partitions to drop" http://gerrit.cloudera.org:8080/#/c/1563/3/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetCachedStmt.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetCachedStmt.java: Line 74: if (parts == null) { or parts is not null but is an empty list Line 75: throw new AnalysisException("No partition matches the partition expr"); Should we allow this to not be an error, now? http://gerrit.cloudera.org:8080/#/c/1563/3/fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java File fe/src/main/java/com/cloudera/impala/analysis/AnalysisContext.java: Line 440: analysisResult_.isComputeStatsStmt() || Why did these lines need to be added here? Were these statements not being authorized before? http://gerrit.cloudera.org:8080/#/c/1563/3/fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java File fe/src/main/java/com/cloudera/impala/analysis/PartitionSpec.java: Line 45: private List<Long> partitionIds_; Why do you need partitionIds_? Can you get that from calling getId() each HdfsPartition in partitions_? Line 55: private Boolean partitionShouldExist_; I know this wasn't commented on before, but can you adda comment here explaining what partitionShouldExist_ represents? Line 58: private boolean isKV_ = false; ", because only they require partition specs in a particular KV form. See PARTITION_EXPR_ERROR." Line 68: this.partitionKeyValues_ = Lists.newArrayList(); Can you initialize these member variables inline at their declaration? Line 115: // Only HDFS tables are partitioned. Do we really not allow S3 tables to be partitioned? Line 232: "Partition expr is not applicable. It should be bounded by " + I find this error message opaque. What does "bounded" mean in this sense? Also, does this check that it is bounded by at most one partition column? Line 237: List<HdfsPartition> matchingPartitions; Can we just use partitions_ in place of matchingPartitions? http://gerrit.cloudera.org:8080/#/c/1563/3/fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java: Line 93: Analyzer analyzer, List<Expr> conjuncts, boolean alterPartition) Please add a comment explaining what this parameter is used for. http://gerrit.cloudera.org:8080/#/c/1563/3/fe/src/test/java/com/cloudera/impala/analysis/ParserTest.java File fe/src/test/java/com/cloudera/impala/analysis/ParserTest.java: This also deserves some end-to-end tests. -- To view, visit http://gerrit.cloudera.org:8080/1563 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Amos Bird <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Amos Bird <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-HasComments: Yes
