Zoltan Borok-Nagy 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:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/Expr.java@1193
PS21, Line 1193:     Subquery subq = (Subquery) this;
> I think we can remove this when taking care of the other comments.
Done


http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:

http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1092
PS21, Line 1092:     Preconditions.checkState(isAnalyzed());
> Preconditions.checkState(isAnalyzed());
Done


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@427
PS21, Line 427:     ExprSubstitutionMap smap = new ExprSubstitutionMap();
> I don't think we should have to do this. At the very least we should have a
Removed these lines and updated ExistsPredicate as you recommended.


http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@464
PS21, Line 464:     // to the stmt's table refs later. Explicitly set the 
inline view's column labels
> Let's have:
Done


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 has a slightly different meaning than the original code.
This allows runtime scalars here, because their type is scalar now and not 
array type.

Maybe it is worth to check that the subquery doesn't return multiple columns.

Anyway, I removed this if statement and it seems that the FE tests don't miss 
it.


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@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?
Or, should I remove this part?



--
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 15:32:57 +0000
Gerrit-HasComments: Yes

Reply via email to