impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.
Date Wed, 25 Oct 2017 18:42:31 GMT
Dimitris Tsirogiannis 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)

Did you add any planner tests?

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.


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?


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


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 check and the disjunction
in some cases?


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.


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


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)


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.



-- 
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 18:42:31 +0000
Gerrit-HasComments: Yes

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