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 30: (3 comments) http://gerrit.cloudera.org:8080/#/c/9005/29/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/29/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@484 PS29, Line 484: if (isRuntimeScalar) subqueryStmt.setLimit(2); > Sorry I pasted the wrong query here. This is the correct one: I think it's surprising that QueryStmt.setLimit() wipes the offset (at least it surprised me :) ). I modified it to keep the offset expr. QueryStmt.setLimit() is only used at two places (including this one) and based on the test results it seems I haven't introduced a regression by changing its behavior. Note that after calling setLimit() subqueryStmt will have an un-analyzed limit element. Maybe it's not a big issue, because we already had a setLimit() call in this method. Added new tests to 'PlannerTest/subquery-rewrite.test' and 'QueryTest/subquery.test'. http://gerrit.cloudera.org:8080/#/c/9005/29/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@809 PS29, Line 809: } > Unsupported correlated subquery with runtime scalar check: Done http://gerrit.cloudera.org:8080/#/c/9005/29/fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java File fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java: http://gerrit.cloudera.org:8080/#/c/9005/29/fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java@45 PS29, Line 45: Preconditions.checkState(child.getLimit() == 2); > Is this needed? Should we make this a Preconditions check? Based on the tests it seems we always invoke setLimit(2) in StmtRewriter.mergeExpr(), so I made it a precondition check as suggested. -- 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: 30 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, 25 Apr 2018 13:32:58 +0000 Gerrit-HasComments: Yes
