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