Alex Behm has posted comments on this change. Change subject: IMPALA-5180: Don't balk at non-deterministic exprs ......................................................................
Patch Set 3: (8 comments) Fix looks much better http://gerrit.cloudera.org:8080/#/c/6575/3//COMMIT_MSG Commit Message: Line 7: IMPALA-5180: Don't balk at non-deterministic exprs Msg is cryptic. Can you summarize what is changed in this patch, as opposed to what bug/behavior is fixed? Line 9: HDFS partition pruning can run afoul of our logic of considering Not sure what "run afoul" means, please be specific. http://gerrit.cloudera.org:8080/#/c/6575/3/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: Line 1029: public boolean isBoundByTupleIds(List<TupleId> tids) { What about this? Our isBound() family of functions should be consistent. Line 1037: * Returns true if expr is fully bound by slotIds and contains at least Not your change, but can you modify the comment to explain what "bound" means? The concept has caused a lot of confusion in the past especially to newcomers. Also, please split up the comment into more sentences. Line 1045: HashSet<SlotId> idSet = Sets.newHashSet(); idSet -> exprSlotIds Line 1046: int members = 0; Why is this needed? Do you mean Preconditions.checkState(!exprSlotIds.isEmpty())? Line 1047: this.getIdsHelper(null, idSet); remove 'this' http://gerrit.cloudera.org:8080/#/c/6575/3/testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test File testdata/workloads/functional-planner/queries/PlannerTest/hdfs.test: Line 1026: select count(*) from keep this query as simple as possible, i.e., same as above -- To view, visit http://gerrit.cloudera.org:8080/6575 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I91054c6bf017401242259a1eff5e859085285546 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-HasComments: Yes
