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 7: (2 comments) The backend code is looking good to me, comments are mainly about test coverage. I'm not confident about the frontend code, I'd like input from Alex or Dimitris about whether the approach is right. http://gerrit.cloudera.org:8080/#/c/9005/6/be/src/exec/cardinality-check-node.cc File be/src/exec/cardinality-check-node.cc: http://gerrit.cloudera.org:8080/#/c/9005/6/be/src/exec/cardinality-check-node.cc@89 PS6, Line 89: CopyRows > No, in this case RowBatch::DeepCopyTo fails. Yeah, I think we can just assume that the frontend will only generate plans with max_cardinality <= 1 for now and deal with additional complications later if we need the additional generality. We can't test the other cases now so seems best to be clear that it doesn't work. http://gerrit.cloudera.org:8080/#/c/9005/7/be/src/exec/cardinality-check-node.cc File be/src/exec/cardinality-check-node.cc: http://gerrit.cloudera.org:8080/#/c/9005/7/be/src/exec/cardinality-check-node.cc@116 PS7, Line 116: Status CardinalityCheckNode::Reset(RuntimeState* state) { I don't think we have test coverage for this code yet, since it's only executed if the node is inside a subplan. We do support this, e.g. the following query puts a cardinality check inside a subplan: select a.id from functional.allcomplextypes a where id < (select item from a.int_array_col); It looks like these planner tests and query tests have a number of subqueries that get put inside subplans: ./testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test ./testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test Your code does look like it handles subplans correctly, just want to make sure we have the test coverage. -- 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: 7 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: Tue, 16 Jan 2018 21:54:33 +0000 Gerrit-HasComments: Yes
