Alex Behm has posted comments on this change. Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries ......................................................................
Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/7706/1/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: Line 692: stmt.selectList_.isDistinct()) && > I am not sure the BinaryPredicate check is redundant. The first is applied My mistake. It's definitely not redundant, I somehow misinterpreted 'expr'. http://gerrit.cloudera.org:8080/#/c/7706/2/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: Line 341: canRewriteCorrelatedSubquery(expr, onClauseConjuncts); Why not fold the new logic into this function and move everything down? Our checks are already somewhat spread out and it seems worth while to consolidate as much as possible. Line 721: com.google.common.base.Function<Expr, String> toSql = Add a static helper that does toSql() on a list. Might live in Expr or ToSqlUtils or wherever you think t's appropriate. http://gerrit.cloudera.org:8080/#/c/7706/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java: Line 733: "id %s (select %s from functional.alltypestiny t where t.bool_col = false " + Add a correlated predicate in the usbquery that does not reference 't' from the subquery. This is to cover the new logic that requires the correlated predicate to reference tuples from the subquery. Line 737: AnalyzesOk(String.format("select id from functional.allcomplextypes t where id " + Just to be sure: Your original fix broke this test right? Line 777: "where t.int_struct_col.f1 = v.id)", cmpOp, aggFn), Why this change? -- To view, visit http://gerrit.cloudera.org:8080/7706 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
