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 <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 21:38:35 +0000 Gerrit-HasComments: Yes
