impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
Date Tue, 22 Aug 2017 04:32:55 GMT
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 <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message