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 19: (2 comments) http://gerrit.cloudera.org:8080/#/c/9005/11/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java File fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java: http://gerrit.cloudera.org:8080/#/c/9005/11/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@175 PS11, Line 175: if (!operand.type_.isNumericType() && !operand.type_.isNull()) { > Thanks! I don't think the type of an expression should not depend on its enclosing context. That breaks a fundamental invariant and may cause all sorts of problems. If the alternative is to always analyze the limit clause in Expr.isScalarSubquery(), then I strongly prefer that variant, even though it's not perfectly clean either. http://gerrit.cloudera.org:8080/#/c/9005/19/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java File fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java: http://gerrit.cloudera.org:8080/#/c/9005/19/fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java@66 PS19, Line 66: if(!queryStmt.returnsSingleRow()) subquery_.type_ = new ArrayType(subquery_.type_); We really should't be setting the type of an expression from the outside. That's the job of expression analysis itself. One alternative could be to keep a the ArrayType or a flag "isScalarSubquery" internally in Subquery and then use that flag or type to properly respond to Expr.isScalarSubquery(). -- 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: 19 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: Thu, 29 Mar 2018 21:44:17 +0000 Gerrit-HasComments: Yes
