Alex Behm has posted comments on this change. Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates ......................................................................
Patch Set 1: (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() Line 14: I added reseting of all of SelectList's members that need to be reset, resetting Line 17: contains agg functions, e.g. because "select if(true, 0, sum(id))" contains -> contain Line 21: SelectStmt, which causes problems if the result exprs have been long lines Line 23: anyways, so the fix is to match normal query execution and rewrite the select anyways -> anyway 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? For example, look for: LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql()); in AnalysisContext.java Pick any query from AnalyzeSubqueriesTest.java for testing 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 shouldn't be rewritten. Exprs containing aggregates should not be rewritten if the rewrite eliminates all aggregates. Line 331: RewritesOk("case when true then 0 when false then sum(id) end", rule, null); Add tests for cases where some but not all aggregates are eliminated. -- 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: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
