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 21: (6 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()) { > Yeah, now I vaguely remember from those formal languages courses that the t That's true. I see the isRuntimeScalar flag more like a hint on how we should evaluate this expression, rather than part of type analysis. 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: // Callers should be sure to analyze the limit clause of stmt I think we can remove this when taking care of the other comments. 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: // limit 1 clause Preconditions.checkState(isAnalyzed()); 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: newSubquery.getStatement().setIsRuntimeScalar( I don't think we should have to do this. At the very least we should have a comment explaining why this is necessary. I looked into this issue and concluded that you probably added this code to work around a bug with expr substitution in ExistsPredicate. The bug is that in ExistsPredicate we keep a member "subquery_", but that member is not properly updated during expr substitution. The easiest fix is to remove the "subquery_" member in ExistsPredicate and consistently use "children_.get(0)" wherever we used "subquery_" before. http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@464 PS21, Line 464: SelectStmt subqueryStmt = (SelectStmt)expr.getSubquery().getStatement(); Let's have: boolean isScalarSubquery = expr.getSubquery().isScalarSubquery(); and use that variable consistently in this function. It's weird to call isScalarSubquery() once we start modifying the subquery statement. It's easy to avoid that. Might also be convenient/consistent to have a isRuntimeScalar boolean variable here. http://gerrit.cloudera.org:8080/#/c/9005/21/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1025 PS21, Line 1025: if (!subquery.getType().isScalarType()) { This has a slightly different meaning than the original code. I could not find a test that covers this branch. Does this check even make sense? Why would we not be able to allow runtime scalar here? -- 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: 21 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: Tue, 10 Apr 2018 22:22:03 +0000 Gerrit-HasComments: Yes
