Jim Apple has posted comments on this change. Change subject: IMPALA-1654: Support general predicates in most partition DDL operations. ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/1563/4//COMMIT_MSG Commit Message: Line 16: Change-Id: I2c9162fcf9d227b8daf4c2e761d57bab4e26408f This language about "partitionSpec" is hard to follow without the code in front of the reader. Can you describe, instead, exactly which DDL statements have changed? Can you also say what their new syntax is with a representative set of examples withotu referencing specific grammar productions like "compound_predicate"? PS4, Line 19: Can you please explain here what "fully bounded" means? 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 75: throw new AnalysisException("No partition matches the partition expr"); > Done Can you also add to your commit message which DDL statements now allow the number of partitions they affect to be 0? 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 55: private Boolean partitionShouldExist_; > see its getter and setter method. Should I move those comments? yes please Line 58: private boolean isKV_ = false; > your comment got truncated. It was not truncated. I meant only that you should add that quote to the comment. Line 115: // Only HDFS tables are partitioned. > Well I don't know. It came from the trunk. Alex, Dimitris? Line 232: "Partition expr is not applicable. It should be bounded by " + > Done grep -iIrF 'fully bounded' fe/src/ returns nothing for me, and I find the phrase somewhat opaque. Can you please explain what it means in the error message, so users like me understand? http://gerrit.cloudera.org:8080/#/c/1563/4/fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/com/cloudera/impala/planner/HdfsPartitionPruner.java: PS4, Line 91: Did you mean "no file"? Also, by "may be added", do you mean added as in ADD PARTITION or returned in the List<HdfsPartition>? Finally, I still don't understand the use of this parameter - what does the set of files have to do with partition pruning, and which ALTER TABLE statements care. Can you explain more in this comment, please? 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: > Yeah. Where should I put the tests? Is there a wiki that I ca n refer to? They can go in /tests/metadata/. Unfortunately, I don't know of a wiki page describing this. -- 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
