Alex Behm has posted comments on this change. Change subject: IMPALA-4309: Introduce Expr rewrite phase and supporting classes. ......................................................................
Patch Set 7: (2 comments) Please take another look at the latest patch set. - Did some final polish - Resurrected the ExprRewriterTest http://gerrit.cloudera.org:8080/#/c/4746/7/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java: Line 102: public List<HdfsPartition> prunePartitions(Analyzer analyzer, List<Expr> conjuncts) > make this an ImmutableList to express that it doesn't change conjuncts? alt The conjuncts list is actually modified. See existing comment. I don't know of a simple way to guard against changing the Exprs in place. Line 122: Expr clonedConjunct = exprRewriter_.rewrite(conjunct.clone(), analyzer); > why not simply directly apply the rule like before? Because the conjunct could have nested BetweenPredicates and we now need an ExprRewriter to handle that. -- To view, visit http://gerrit.cloudera.org:8080/4746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2279dc984bcf7742db4fa3b1aa67283ecbb05e6e Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
