[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 13: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 13
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sat, 02 Dec 2017 04:09:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 13:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 13
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sat, 02 Dec 2017 00:27:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 13: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 13
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sat, 02 Dec 2017 00:26:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Hello Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#13).

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..

IMPALA-1422: support a constant on LHS of IN predicates.

Currently, constant expressions for the LHS of the IN predicate
are not supported. This patch adds this support as a rewrite in
StmtRewriter (where subqueries are rewritten to joins). Since
there is a nested-loop variant of left semijoin, support for IN
is handled by not erring out. NOT IN is handled by a rewrite to
corresponding NOT EXISTS predicate. Support for NOT IN with a
correlated subquery is not included in this change.

Re-organized the frontend subquery analysis tests to expand coverage.

Testing:
- added frontend subquery analysis tests
- added e2e tests

Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
---
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
A 
testdata/workloads/functional-query/queries/QueryTest/subquery-in-constant-lhs.test
M tests/query_test/test_queries.py
7 files changed, 932 insertions(+), 203 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8322/13
--
To view, visit http://gerrit.cloudera.org:8080/8322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 13
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8322/12/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test:

http://gerrit.cloudera.org:8080/#/c/8322/12/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2277
PS12, Line 2277: (select int_col from functional.tinyinttable where
> Make this one more complicated or add another test like this:
done. we're getting a HJ for the inner and a nested-loop for the outer. the lhs 
constant is pushed down into the HJ of the nested join.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Sat, 02 Dec 2017 00:04:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8322/12/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test:

http://gerrit.cloudera.org:8080/#/c/8322/12/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test@2277
PS12, Line 2277: (select int_col from functional.tinyinttable where
Make this one more complicated or add another test like this:

select * from functional.alltypes t where 1 in
(select int_col from functional.alltypessmall t where
 bigint_col in (select bigint_col from functional.alltypestiny where id = t.id))

I want to be sure that the nested correlated subquery resolves to the right "t" 
and that the rewrite using a HJ works correctly



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:17:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Hello Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#12).

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..

IMPALA-1422: support a constant on LHS of IN predicates.

Currently, constant expressions for the LHS of the IN predicate
are not supported. This patch adds this support as a rewrite in
StmtRewriter (where subqueries are rewritten to joins). Since
there is a nested-loop variant of left semijoin, support for IN
is handled by not erring out. NOT IN is handled by a rewrite to
corresponding NOT EXISTS predicate. Support for NOT IN with a
correlated subquery is not included in this change.

Re-organized the frontend subquery analysis tests to expand coverage.

Testing:
- added frontend subquery analysis tests
- added e2e tests

Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
---
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
A 
testdata/workloads/functional-query/queries/QueryTest/subquery-in-constant-lhs.test
M tests/query_test/test_queries.py
7 files changed, 908 insertions(+), 203 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8322/12
--
To view, visit http://gerrit.cloudera.org:8080/8322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 12
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 11:

updates alias generation to use outer-block's alias generators.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 11
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 01 Dec 2017 21:37:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Hello Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#10).

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..

IMPALA-1422: support a constant on LHS of IN predicates.

Currently, constant expressions for the LHS of the IN predicate
are not supported. This patch adds this support as a rewrite in
StmtRewriter (where subqueries are rewritten to joins). Since
there is a nested-loop variant of left semijoin, support for IN
is handled by not erring out. NOT IN is handled by a rewrite to
corresponding NOT EXISTS predicate. Support for NOT IN with a
correlated subquery is not included in this change.

Re-organized the frontend subquery analysis tests to expand coverage.

Testing:
- added frontend subquery analysis tests
- added e2e tests

Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
---
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
A 
testdata/workloads/functional-query/queries/QueryTest/subquery-in-constant-lhs.test
M tests/query_test/test_queries.py
8 files changed, 921 insertions(+), 203 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8322/10
--
To view, visit http://gerrit.cloudera.org:8080/8322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 10
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 9:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8322/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@368
PS9, Line 368: String wrapperAlias = 
rhsQuery.getTableAliasGenerator().getNextAlias();
> Please address this alias generation issue as discussed.
done. I create generators externally now since I can't create the outer-block 
before I've created this view. Then I pass them to the outer-block after its 
created.


http://gerrit.cloudera.org:8080/#/c/8322/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@401
PS9, Line 401:
> remove, no point in having a blank line here
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 01 Dec 2017 06:50:47 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 9:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8322/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@368
PS9, Line 368: String wrapperAlias = 
rhsQuery.getTableAliasGenerator().getNextAlias();
Please address this alias generation issue as discussed.


http://gerrit.cloudera.org:8080/#/c/8322/9/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@401
PS9, Line 401:
remove, no point in having a blank line here



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 30 Nov 2017 22:04:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Hello Dimitris Tsirogiannis, Alex Behm,

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#9).

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..

IMPALA-1422: support a constant on LHS of IN predicates.

Currently, constant expressions for the LHS of the IN predicate
are not supported. This patch adds this support as a rewrite in
StmtRewriter (where subqueries are rewritten to joins). Since
there is a nested-loop variant of left semijoin, support for IN
is handled by not erring out. NOT IN is handled by a rewrite to
corresponding NOT EXISTS predicate. Support for NOT IN with a
correlated subquery is not included in this change.

Re-organized the frontend subquery analysis tests to expand coverage.

Testing:
- added frontend subquery analysis tests
- added e2e tests

Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
---
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
A 
testdata/workloads/functional-query/queries/QueryTest/subquery-in-constant-lhs.test
M tests/query_test/test_queries.py
7 files changed, 903 insertions(+), 203 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/22/8322/9
--
To view, visit http://gerrit.cloudera.org:8080/8322
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d69889a3c72e90be9d4ccf47d2816819ae32acb
Gerrit-Change-Number: 8322
Gerrit-PatchSet: 9
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Vuk Ercegovac (Code Review)]

Vuk Ercegovac has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 8:

(24 comments)

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

http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@326
PS8, Line 326:*condition using the LHS constant. The rewrite handles 
group by,
> Not clear what "handles" here means. It doesn't really do anything special
thanks for the suggestion.. re-worded.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@327
PS8, Line 327:*analytic functions, limit, and uncorrelated subqueries. 
Correlated subqueries
> Move last sentence into a TODO like this:
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@330
PS8, Line 330:*TODO: handle correlated NOT IN case
> remove (see above comment)
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@340
PS8, Line 340:*So, if C is equal to any x_i, the expression is false. 
Similarly, if any
> Nice description. I'm wondering if we directly translate this insight into
yup, simplified.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@355
PS8, Line 355: Preconditions.checkArgument(rhs.contains(Subquery.class));
> not needed because L356-357 performs the same check
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@398
PS8, Line 398: newSubquery.analyze(rhsQuery.getAnalyzer());
> This seems wrong. It might happen to work but pretty sure this will eventua
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@714
PS8, Line 714: if (isCorrelatedPredicate(subqueryStmt.getWhereClause(), 
subqueryTupleIds)) {
> containsCorrelatedPredicate()?
yes, good catch. further simplified.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java:

http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@277
PS8, Line 277:   // NOT IN subquery with a correlated non-equi predicate is 
ok if the subquery only
> [NOT] IN subquery
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@292
PS8, Line 292: AnalyzesOk(String.format("select 1 from 
functional.allcomplextypes t " +
> What about queries without complex types and non-constant LHS in the IN pre
there's several above, for example L220 has a non-constant LHS and the types 
are not complex. L92 loops over a bunch of scalar types (and non-constant LHS).


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@299
PS8, Line 299:   AnalysisError(String.format("select * from 
functional.alltypestiny t where id %s " +
> These tests are pretty rough to follow, perhaps we should spent some time b
makes sense. the cut in this patch is just to get the tests to exercise the 
various branches more consistently where applicable.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@437
PS8, Line 437:   // Tests for constant on the left hand side of [NOT] IN.
> Since we are doing a double rewrite here, it's interesting to test nested s
separated these out. I looked at making constant LHS just another dimension of 
the existing tests but I think a more comprehensive clean up here is better as 
a separate change. for now, I've added several more tests.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@476
PS8, Line 476:
> You can omit this part if you like. Error message matching is prefix based.
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@482
PS8, Line 482:   "SELECT int_col FROM functional.alltypestiny b 
WHERE b.id = a.id ORDER BY int_col ASC LIMIT 5");
> long line, you can omit this line in the expected error message
Done


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@520
PS8, Line 520: + "(select * from functional.tinyinttable x where t.id = 
x.int_col)");
> please move "+" to previous line (easier to read and more consistent with e
Done

[Impala-ASF-CR] IMPALA-1422: support a constant on LHS of IN predicates.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8322 )

Change subject: IMPALA-1422: support a constant on LHS of IN predicates.
..


Patch Set 8:

(24 comments)

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

http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@326
PS8, Line 326:*condition using the LHS constant. The rewrite handles 
group by,
Not clear what "handles" here means. It doesn't really do anything special to 
group by, limit, etc.

Maybe instead say something like: The inline view ensures that the filter is 
logically evaluated against the RHS result even if the RHS contains 
analytics/aggregation/limit. Adding the filter directly to any clause within 
the RHS statement is generally incorrect.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@327
PS8, Line 327:*analytic functions, limit, and uncorrelated subqueries. 
Correlated subqueries
Move last sentence into a TODO like this:

TODO: Correlated NOT IN subqueries require that column resolution be extended 
to handle ...


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@330
PS8, Line 330:*TODO: handle correlated NOT IN case
remove (see above comment)


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@340
PS8, Line 340:*So, if C is equal to any x_i, the expression is false. 
Similarly, if any
Nice description. I'm wondering if we directly translate this insight into the 
WHERE condition to make it easier to understand:

WHERE C IS NULL OR tmp.x IS NULL OR C = tmp.x

That way we can also shrink/remove the last paragraph in this comment block.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@355
PS8, Line 355: Preconditions.checkArgument(rhs.contains(Subquery.class));
not needed because L356-357 performs the same check


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@398
PS8, Line 398: newSubquery.analyze(rhsQuery.getAnalyzer());
This seems wrong. It might happen to work but pretty sure this will eventually 
lead to trouble.
Each query block needs to get its own analyzer and the hierarchy of Analyzers 
needs to be fixed.

I'm thinking we should reset the rhsQuery and not analyze newSubquery here. The 
caller will analyze the new ExistsPredicate with the Analyzer of the enclosing 
stmt (the correct analyzer).


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@714
PS8, Line 714: if (isCorrelatedPredicate(subqueryStmt.getWhereClause(), 
subqueryTupleIds)) {
containsCorrelatedPredicate()?


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java:

http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@277
PS8, Line 277:   // NOT IN subquery with a correlated non-equi predicate is 
ok if the subquery only
[NOT] IN subquery


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@292
PS8, Line 292: AnalyzesOk(String.format("select 1 from 
functional.allcomplextypes t " +
What about queries without complex types and non-constant LHS in the IN 
predicate? Not sure that case has coverage now.


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@299
PS8, Line 299:   AnalysisError(String.format("select * from 
functional.alltypestiny t where id %s " +
These tests are pretty rough to follow, perhaps we should spent some time 
brainstorming how we can clean up these tests (after this patch of course)


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@437
PS8, Line 437:   // Tests for constant on the left hand side of [NOT] IN.
Since we are doing a double rewrite here, it's interesting to test nested 
subqueries. Can you add 1-2 tests for that?


Since you already did the work of separating these tests, it seems good to add 
them into a new @Test, something like TestConstantLhsInSubqueries()


http://gerrit.cloudera.org:8080/#/c/8322/8/fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java@476
PS8, Line 476:
You can omit this part if you like. Error message matching is prefix based.

You can use this to clean up other AnalysisError() calls. Imo it's not always 
useful to include the whole failed