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 30: (1 comment) Change looks good to me. I'll +2 once we have decided how to proceed with my last comment :) 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); > I think it's surprising that QueryStmt.setLimit() wipes the offset (at leas It's not safe to rely on our tests to find all issues. We should convince ourselves that the modified setLimit() has the desired semantics for that other caller in L566. I investigated this and convinced myself that the new setLimit() has the correct semantics. In fact, your change fixes an existing, previously unknown bug. Let's separate the bugfix from this huge change. I filed IMPALA-6934 and assigned it to you. Please let me know if you cannot work on it As for this change, you could either wait for the bugfix to be merged, or you could undo your offset fix and comment out the offset tests noting that they will be enabled when IMPALA-6934 is fixed. Either approach works for me. -- 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 21:51:34 +0000 Gerrit-HasComments: Yes
