Alex Behm has posted comments on this change. Change subject: IMPALA-3167: Fix assignment of WHERE-clause predicate through grouping agg + outer join. ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/4960/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: PS1, Line 1426: hasOjTest > Where is this function used? Sorry, this was an experiment. Not needed. Removed. PS1, Line 1558: evalByJoin > This variable name is a bit confusing to me. Is "evalAfterJoin" better? Done http://gerrit.cloudera.org:8080/#/c/4960/1/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: PS1, Line 851: public static <C extends Expr> void removeDuplicates(List<C> l) > Suggest to refactor this function a bit like below (if possible): Exprs are not hashable, i.e. do not implement hashCode(). Fixing that is beyond the scope of this change and also dangerous (wrong results, if not done properly). I'd rather not change the API of this function, because that will force changes in many parts of the code which could introduce new bugs and make backporting more difficult. We don't really need thread safety here, so I don't think we need to go out of out way to add it. PS1, Line 853: List<C> origList = Lists.newArrayList(l); : l.clear(); : for (C expr: origList) if (!l.contains(expr)) l.add(expr); > Would it be better to use a Set instead of List? See comment above. Not hashable. Also a Set is not good because it does not preserve insertion order and may lead to tests with non-deterministic results. -- To view, visit http://gerrit.cloudera.org:8080/4960 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I774d13a13ad1e8fe82512df98dc29983bdd232eb Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Anonymous Coward #27 Gerrit-HasComments: Yes
