You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org> on 2017/08/17 19:08:47 UTC

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Dimitris Tsirogiannis has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7706

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................

IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

This commit fixes an issue where a query will return wrong results if it
has an aggregate subquery with a correlated inequality predicate.
Since the rewrite for this case is not currently supported, an
exception is now thrown during the analysis.

Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
3 files changed, 64 insertions(+), 9 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/7706/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has uploaded a new patch set (#3).

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................

IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

This commit fixes an issue where a query will return wrong results if it
has an aggregate subquery with a correlated inequality predicate.
Since the rewrite for this case is not currently supported, an
exception is now thrown during the analysis.

Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
3 files changed, 115 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/7706/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 3:

(5 comments)

pretty much done

http://gerrit.cloudera.org:8080/#/c/7706/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 1417:   public static String toSqlHelper(List<Expr> exprs) {
listToSql()?


http://gerrit.cloudera.org:8080/#/c/7706/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java:

Line 399:     // Ensure that all the extracted correlated predicates can be added to the on-clause
nit: we usually use caps for clauses and such, i.e. ON-clause


Line 648:   private static void canRewriteCorrelatedSubquery(Expr expr) throws AnalysisException {
validateCorrelatedSubqueryStmt()?


Line 676:    * added to the on-clause of the join that results from the subquert rewrite; It throws
ON-clause and typo: subquery. Also use "." instead of ";"


Line 683:     Preconditions.checkNotNull(correlatedPredicates);
Preconditions.checkState(inlineView.isAnalyzed());


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 5: Code-Review+2

Rebase, keep Alex's +2

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 1:

(4 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 681:             List<Expr> slotRefs = Lists.newArrayList();
Might be easier/shorter:
 
List<TupleId> tids = Lists.newArrayList();
getIds(tids, null);
return !Collections.disjoint(tids, subqueryTblIds);


Line 690:     // Inequality correlated predicates are not currently supported in aggregate
This bug is not specific to inequality predicates. I think it's really that we only support equality predicates. For example, try queries like this:

select 1 from functional.alltypes t1
where int_col = (select max(id) from functional.alltypes t2 where t1.id = t2.id and coalesce(t1.bool_col, t2.bool_col));

select 1 from functional.alltypes t1
where int_col = (select max(id) from functional.alltypes t2 where t1.id = t2.id and t1.string_col like t2.string_col);

Those queries fail in other mysterious ways, but the point is that this bug is not specific to inequality.


Line 691:     // subqueries (see IMPALA-5531).
We should be able to run queries with arbitrary correlated predicates if the subquery references relative nested collections.


Line 692:     if (expr instanceof BinaryPredicate && !correlatedPredicates.isEmpty() &&
Let's report which predicate makes the subquery unsupported.

Also the BinaryPredicate check is redundant with Expr.IS_NEQ_BINARY_PREDICATE.


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 2:

(5 comments)

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
Done


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 ToSq
Done


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
Done


Line 737:         AnalyzesOk(String.format("select id from functional.allcomplextypes t where id " +
> Just to be sure: Your original fix broke this test right?
Yes


Line 777:               "where t.int_struct_col.f1 = v.id)", cmpOp, aggFn),
> Why this change?
Forgot to revert. Done


-- 
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 <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has uploaded a new patch set (#4).

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................

IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

This commit fixes an issue where a query will return wrong results if it
has an aggregate subquery with a correlated inequality predicate.
Since the rewrite for this case is not currently supported, an
exception is now thrown during the analysis.

Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
3 files changed, 115 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/7706/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 5: Verified+1

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1147/

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

This commit fixes an issue where a query will return wrong results if it
has an aggregate subquery with a correlated inequality predicate.
Since the rewrite for this case is not currently supported, an
exception is now thrown during the analysis.

Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Reviewed-on: http://gerrit.cloudera.org:8080/7706
Reviewed-by: Dimitris Tsirogiannis <dt...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
3 files changed, 115 insertions(+), 24 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dimitris Tsirogiannis: Looks good to me, approved



-- 
To view, visit http://gerrit.cloudera.org:8080/7706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 1:

(4 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 681:             List<Expr> slotRefs = Lists.newArrayList();
> Might be easier/shorter:
Done


Line 690:     // Inequality correlated predicates are not currently supported in aggregate
> This bug is not specific to inequality predicates. I think it's really that
Done


Line 691:     // subqueries (see IMPALA-5531).
> We should be able to run queries with arbitrary correlated predicates if th
Done


Line 692:     if (expr instanceof BinaryPredicate && !correlatedPredicates.isEmpty() &&
> Let's report which predicate makes the subquery unsupported.
I am not sure the BinaryPredicate check is redundant. The first is applied on 'expr' which is the subquery expr while the latter is applied on the correlated predicates. Is this what you mean?


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 4: Code-Review+2

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has uploaded a new patch set (#2).

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................

IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

This commit fixes an issue where a query will return wrong results if it
has an aggregate subquery with a correlated inequality predicate.
Since the rewrite for this case is not currently supported, an
exception is now thrown during the analysis.

Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
---
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
3 files changed, 82 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/7706/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7706
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6ca7b60ef0543430d2f5a802285254ebb52db2ab
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-5531: Fix correctness issue in correlated aggregate subqueries
......................................................................


Patch Set 3:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7706/3/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 1417:   public static String toSqlHelper(List<Expr> exprs) {
> listToSql()?
Done


http://gerrit.cloudera.org:8080/#/c/7706/3/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java:

Line 399:     // Ensure that all the extracted correlated predicates can be added to the on-clause
> nit: we usually use caps for clauses and such, i.e. ON-clause
Done


Line 648:   private static void canRewriteCorrelatedSubquery(Expr expr) throws AnalysisException {
> validateCorrelatedSubqueryStmt()?
Done


Line 676:    * added to the on-clause of the join that results from the subquert rewrite; It throws
> ON-clause and typo: subquery. Also use "." instead of ";"
Done


Line 683:     Preconditions.checkNotNull(correlatedPredicates);
> Preconditions.checkState(inlineView.isAnalyzed());
Done


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5531: Fix correctness issue in correlated aggregate subqueries

Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
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 <dt...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes