Amos Bird has posted comments on this change. Change subject: IMPALA-1654: Support general predicates in most partition DDL operations. ......................................................................
Patch Set 3: (14 comments) 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:: Done 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? for all alter table stmts. 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 Done Line 75: throw new AnalysisException("No partition matches the partition expr"); > Should we allow this to not be an error, now? Done 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 After partitionspec slotrefs being registered, it is needed. 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 H Done Line 55: private Boolean partitionShouldExist_; > I know this wasn't commented on before, but can you adda comment here expla see its getter and setter method. Should I move those comments? Line 58: private boolean isKV_ = false; > ", because only they require partition specs in a particular KV form. See P your comment got truncated. Line 68: this.partitionKeyValues_ = Lists.newArrayList(); > Can you initialize these member variables inline at their declaration? Done Line 115: // Only HDFS tables are partitioned. > Do we really not allow S3 tables to be partitioned? Well I don't know. It came from the trunk. 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? Done "bounded" means fully bounded by a slotref, that is, a conjunct contains exactly one. I've changed the errmsg. Line 237: List<HdfsPartition> matchingPartitions; > Can we just use partitions_ in place of matchingPartitions? Done 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. Done 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. Yeah. Where should I put the tests? Is there a wiki that I ca n refer to? -- 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
