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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
......................................................................


Patch Set 23:

(3 comments)

Code looks pretty good now. I'll do another pass over everything focusing on 
the tests.

http://gerrit.cloudera.org:8080/#/c/9005/21/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/9005/21/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1025
PS21, Line 1025:     if (isCorrelated && 
!item.getExpr().contains(Expr.IS_BUILTIN_AGG_FN)) {
> This allows runtime scalars here, because their type is scalar now and not
Right. That could be an indication that this check is useless or that we have a 
gap in test coverage. I'm worried about the latter. One of two outcomes is 
acceptable:
* add a test that hits this branch
* convince ourselves that the type is always scalar here, and then add a 
Preconditions check


http://gerrit.cloudera.org:8080/#/c/9005/23/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:

http://gerrit.cloudera.org:8080/#/c/9005/23/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@82
PS23, Line 82:       if (node instanceof StatementBase) {
No need for this. We're always going to have a StatementBase


http://gerrit.cloudera.org:8080/#/c/9005/23/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@917
PS23, Line 917:     //TODO: make it work again or delete
              : //    testToSql("select * from functional.alltypes where not 
(id < 10 and " +
              : //            "(int_col in (select int_col from 
functional.alltypestiny)) and " +
              : //            "(string_col = (select max(string_col) from 
functional.alltypestiny)))",
              : //        "SELECT * FROM functional.alltypes WHERE NOT (id < 10 
AND " +
              : //            "(int_col IN (SELECT int_col FROM 
functional.alltypestiny)) AND " +
              : //        "(string_col = (SELECT max(string_col) FROM 
functional.alltypestiny)))");
> So, is it OK if I just leave this TODO here?
Please file an improvement JIRA for this case, and remove the test.

The issue here is that we push the NOT into the condition, so the ANDs turn 
into ORs and then we no longer support the subquery.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 23
Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogian...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com>
Gerrit-Comment-Date: Wed, 11 Apr 2018 21:44:28 +0000
Gerrit-HasComments: Yes

Reply via email to