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 3: (10 comments) Thanks all! http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.h File be/src/exec/scalar-check-node.h: http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.h@29 PS3, Line 29: ScalarCheckNode > small naming nit, but it seems that this class checks that the cardinality I agree that it's a better name, so renamed it to CardinalityCheckNode. Also, to make it more general, it can be parameterized by the max allowed cardinality. http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc File be/src/exec/scalar-check-node.cc: http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc@33 PS3, Line 33: Status ScalarCheckNode::Prepare(RuntimeState* state) { > Can you DCHECK that this doesn't have any conjuncts assigned to it. Those a I added a DCHECK for conjuncts. The limit of this node is set by the constructor of the plan node and it equals to the allowed cardinality. I added a DCHECK_EQ here to check if limit and max_cardinality is the same. http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc@56 PS3, Line 56: if (rows_collected > 1) { > Does conditional fit on one line? Now that we want to display the associated SQL statement as well, it doesn't. http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc@57 PS3, Line 57: scalar > what guarantees a scalar here? At runtime nothing, it is checked at parse-time. However, as you suggested I renamed it to CardinalityCheckNode. http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc@57 PS3, Line 57: scalar > It would be a lot more user friendly if we could report which scalar subque The subquery gets rewritten at one point of the analysis. E.g. it rewrites "id = 1 or id = 2" to "id IN (1, 2)" Therefore I needed to hack a little to plumb the original SQL statement. I wonder if there's a better or usual way to do that. http://gerrit.cloudera.org:8080/#/c/9005/3/be/src/exec/scalar-check-node.cc@59 PS3, Line 59: if (child_batch.num_rows() == 1) { > Does conditional fit on one line? yes http://gerrit.cloudera.org:8080/#/c/9005/3/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/3/fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java@177 PS3, Line 177: if (operand instanceof Subquery && !operand.type_.isScalarType()) { > Handling this case here doesn't feel quite right, since it's kind-of an err I restructured the branches http://gerrit.cloudera.org:8080/#/c/9005/3/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/3/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@740 PS3, Line 740: // Scalar checking requires to merge the output of the child fragment > Maybe this would be better explained as "The scalar check must execute on a yeah, it does sound better. http://gerrit.cloudera.org:8080/#/c/9005/3/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/3/fe/src/main/java/org/apache/impala/planner/ScalarCheckNode.java@26 PS3, Line 26: conjuncts and a limit clause. > what conjuncts and what limit? perhaps phrase this to be more specific. Oops, it was a leftover from copy pasting SelectNode. http://gerrit.cloudera.org:8080/#/c/9005/3/testdata/workloads/functional-query/queries/QueryTest/subquery.test File testdata/workloads/functional-query/queries/QueryTest/subquery.test: http://gerrit.cloudera.org:8080/#/c/9005/3/testdata/workloads/functional-query/queries/QueryTest/subquery.test@839 PS3, Line 839: ---- QUERY > We should also add planner tests to test the explain plan output. E.g. see Added some. -- 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: 3 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: Vuk Ercegovac <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 15 Jan 2018 17:20:12 +0000 Gerrit-HasComments: Yes
