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 <vercego...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 25 Oct 2017 18:42:31 +0000 Gerrit-HasComments: Yes
