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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 11 Apr 2018 21:44:28 +0000 Gerrit-HasComments: Yes
