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 1: (15 comments) Thanks for all the 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 nee You are right, but I still keep it because I've changed the implementation and now we need to have a member. I renamed it to scalar_row_batch_ btw. ScalarCheckNode is now a blocking node and deep-copies its child's row in Open(), then passes it to its parent in GetNext(). http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.h@47 PS1, Line 47: child_row_idx_ > This member is not used anywhere. Probably it should be removed. Done 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@31 PS1, Line 31: NULL > Prefer nullptr in new code. Removed it from here since it is a boost::scoped_ptr with a default constructor. But maybe I can add "= nullptr" in the class definition to make the initialization explicit. http://gerrit.cloudera.org:8080/#/c/9005/1/be/src/exec/scalar-check-node.cc@32 PS1, Line 32: child_row_idx_(0), > We've mostly been switching to initialising member variables to constants i Removed those members. 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) { > I think this needs to be a while loop that resets the batch after each iter I followed this logic and moved this part to Open() since now it is a blocking node and we want to call Close() on the child asap. 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 Done 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); > I think it's the start of a good idea. I originally suggested modelling thi Thanks for the idea, I followed this logic. So I deep copy the child's row in Open(), or raise an error if there are more rows. Also, I close the child right after reading its row. Then, deep copy the single scalar row in GetNext() into the parent's row batch. 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 r Replaced the comment to make it more informative. 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 r Replaced the comment. 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 . Done http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@735 PS1, Line 735: * @throws ImpalaException > We don't generally have @throws declarations in Java comments Oops, eclipse added it automagically. http://gerrit.cloudera.org:8080/#/c/9005/1/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@746 PS1, Line 746: selectNode > scalarCheckNode? Yes! 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 . Done http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test File testdata/workloads/functional-query/queries/QueryTest/subquery.test: http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test@839 PS1, Line 839: ---- QUERY > Maybe add a test that sets num_nodes=1 to make sure that the single-node ve Done http://gerrit.cloudera.org:8080/#/c/9005/1/testdata/workloads/functional-query/queries/QueryTest/subquery.test@932 PS1, Line 932: ==== > The case when the subquery returns 0 row could be tested too. Done -- 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-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 12 Jan 2018 16:36:14 +0000 Gerrit-HasComments: Yes
