impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vuk Ercegovac (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.
Date Wed, 25 Oct 2017 21:38:35 GMT
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
......................................................................


Patch Set 5:

(8 comments)

missed the planner tests last time; they are included in the next patch.

http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java:

http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@228
PS5, Line 228: 	  if (newConjunct instanceof BoolLiteral) {
             : 	      smap.put(conjunct, newConjunct);
             : 	      continue;
             : 	  }
> nit: remove tabs. Also, I am not sure I understand what is happening here.
removed... I had an earlier attempt that tried to collapse the case where lhs is null.


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@308
PS5, Line 308: correlated aggregates
> what is a correlated aggregate?
clarified.


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@323
PS5, Line 323: c
> use capital C just to be consistent with L321
Done


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@329
PS5, Line 329: 10 IS NULL OR
> Since we know what the constant literal is, can't we avoid the is null chec
I tried this but saw that for example, (1 + null) did not get simplified to null when this
rewrite was invoked. Even if a case like this is simplified, will we guarantee all such simplifications?
(we should, but I cannot depend on that at the moment).


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@355
PS5, Line 355: or 10 IS NOT NULL
> Same comment as above.
comment had an error. fixed.


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@360
PS5, Line 360: .
> nit: remove '.'
Done


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@366
PS5, Line 366: 
> nit: remove empty lines (L366, L369 and 371)
Done


http://gerrit.cloudera.org:8080/#/c/8322/5/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@428
PS5, Line 428: newSubquery.reset();
             :       newSubquery.analyze(rhsQuery.getAnalyzer());
> Why do you need to go through reset/analyze? Same question for L459-460.
the select was cloned, so is in an analyzed state. added a comment. let me know if this should
be handled in some other way-- I can assume that caller code will cover this case, and currently
it will-- but I opted to be explicit about it here.

The case for L459 is a new select, so I've removed that reset.



-- 
To view, visit http://gerrit.cloudera.org:8080/8322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 5
Gerrit-Owner: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercegovac@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2017 21:38:35 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message