Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/6653/1//COMMIT_MSG Commit Message: Line 10: - SelectList.reset() didn't properly reset some of its members, though > reset() Done Line 14: I added resetting of all of SelectList's members that need to be > resetting Done Line 17: - SimplifyConditionalsRule was changing the meaning of queries that > contains -> contain Done Line 21: - ExprRewriteRulesTest was performing rewrites on the result exprs of > long lines Done Line 23: substituted. In normal query execution, we don't rewrite the result > anyways -> anyway Done http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: Line 80: protected String sqlString_; > fter this change will we still see the new SQL after subquery rewriting? Fo Yes, this is not changing the behavior of sqlString_, just moving it out of the "Members that need to be reset()" block. http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 328: // IMPALA-5125: Exprs containing aggregates should not be rewritten if the rewrite > Exprs containing aggregates should not be rewritten if the rewrite eliminat Done Line 331: RewritesOk("if(false, max(id), min(id))", rule, "min(id)"); > Add tests for cases where some but not all aggregates are eliminated. Done -- To view, visit http://gerrit.cloudera.org:8080/6653 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-HasComments: Yes
