impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anonymous Coward (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3167: Fix assignment of WHERE-clause predicate through grouping agg + outer join.
Date Mon, 07 Nov 2016 13:32:31 GMT
Anonymous Coward #27 has posted comments on this change.

Change subject: IMPALA-3167: Fix assignment of WHERE-clause predicate through grouping agg
+ outer join.
......................................................................


Patch Set 1:

(3 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?


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):

public static <C extends Expr> List<C> removeDuplicates(List<C> l) {
  if (l == null) {
    return l;
  } else {
    return Lists.newArrayList(Sets.newHashSet(l));
  }
}

This makes this function thread-safe. The passed in list won't be modified, and thus can be
safely shared among threads.


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?
Set<C> s = Sets.newHashSet(l);
l.clear();
l.addAll(s);


-- 
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #27
Gerrit-HasComments: Yes

Mime
View raw message