Tim Armstrong 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: (7 comments) 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 aren't implemented by this node so we'd get incorrect results in that case. We should actually convince ourselves that it's not possible to assign conjuncts to this plan node (I honestly don't know if it is - might be a question for Alex). I think the same thing applies for a limit of 0 - I'm not sure if we can get a limit applied. 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? 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? It would be a lot more user friendly if we could report which scalar subquery failed. It might be feasible to plumb information (the original SQL text? a line number?) through from analysis so it can be reported here. Actually, if we just plumbed the string through from analysis, we could make this a generic node like Vuk suggested that doesn't know anything about the SQl semantics it's implementing. 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? 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 error-handling path. It seems like this part should be a separate branch from the error handling that starts in l175. 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 single node". 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 testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test -- 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 <borokna...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Jan 2018 23:38:31 +0000 Gerrit-HasComments: Yes
