impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (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 19:16:04 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Anonymous Coward #27
Gerrit-HasComments: Yes

Mime
View raw message