Csaba Ringhofer 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 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h File be/src/exec/scalar-check-node.h: http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h@44 PS1, Line 44: boost::scoped_ptr<RowBatch> child_row_batch_; If I understand correctly, having child_row_batch_ as member is usually needed because the row_batch in GetNext() can reach its capacity, so the remaining fetched rows will be copied in the next GetNext() call(s). Is this possible here? That would mean that the GetNext() was called with an already full row batch. If this is not possible, then this member could be replaced with a local variable in GetNext(). http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc File be/src/exec/scalar-check-node.cc: http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@56 PS1, Line 56: if (child_row_batch_->num_rows() == 0) { Is it possible/ok for this condition to be false? See my comment in scalar-check-node.h http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@63 PS1, Line 63: one "zero or one" or "maximum one" would be more exact http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@78 PS1, Line 78: child_row_batch_->TransferResourceOwnership(row_batch); Just an idea, not sure if a good one: it may be better to deep copy the row, which would make it possible to release the resources referenced by child_row_batch_ earlier. http://gerrit.cloudera.org:8080/#/c/9005/1/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/1/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@178 PS1, Line 178: // Runtime check needed to check if subquery returns scalar This is not just a check, but can be a transformation, as array types are replaced. This should be mentioned in the comment. +nit: missing . http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java: http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java@179 PS1, Line 179: // Runtime check needed to check if subquery returns scalar This is not just a check, but can be a transformation, as array types are replaced. This should be mentioned in the comment. +nit: missing . http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@90 PS1, Line 90: // If true, a ScalarCheckNode is needed to check this statement's result at runtime nit: missing . http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java File fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java: http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java@32 PS1, Line 32: // LIMIT 2 is enough to check if the child returns a scalar value : // Also, an optimized plan is generated with LIMIT 2 nit: 2 missing . -- 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: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 11 Jan 2018 16:42:33 +0000 Gerrit-HasComments: Yes
