Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11852 )
Change subject: IMPALA-3652: Fix resource transfer in subplans with limits ...................................................................... Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/aggregation-node-base.cc File be/src/exec/aggregation-node-base.cc: http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/aggregation-node-base.cc@77 PS1, Line 77: for (auto& agg : aggs_) RETURN_IF_ERROR(agg->Reset(state, row_batch)); > I think there are cases where previous batches reference memory owned by th Done http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/analytic-eval-node.cc File be/src/exec/analytic-eval-node.cc: http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/analytic-eval-node.cc@812 PS1, Line 812: Status AnalyticEvalNode::Reset(RuntimeState* state, RowBatch* row_batch) { > I feel like we need some of the logic from the eos branch on line 783 here Done http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/partitioned-hash-join-node.cc@200 PS1, Line 200: Status PartitionedHashJoinNode::Reset(RuntimeState* state, RowBatch* row_batch) { > I think it's possible that returned rows could reference memory from in-mem Done http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/partitioned-hash-join-node.cc@211 PS1, Line 211: if (output_unmatched_batch_ != nullptr) { > I don't understand why this is true - couldn't the previous GetNext() call Done http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/subplan-node.cc File be/src/exec/subplan-node.cc: http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/subplan-node.cc@91 PS1, Line 91: transferre > nit: transferred Done http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/11852/1/be/src/exec/topn-node.cc@219 PS1, Line 219: row_batch->tuple_data_pool()->AcquireData(tuple_pool_.get(), false); > I think we'd need some very weird query shape with limits on subqueries at Done http://gerrit.cloudera.org:8080/#/c/11852/1/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch.test File testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch.test: http://gerrit.cloudera.org:8080/#/c/11852/1/testdata/workloads/functional-query/queries/QueryTest/nested-types-tpch.test@195 PS1, Line 195: > Do these options take effect? If batch_size is part of the test vector that Done -- To view, visit http://gerrit.cloudera.org:8080/11852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3968a379fcbb5d30fcec304995d3e44933dbbc77 Gerrit-Change-Number: 11852 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 06 Nov 2018 21:25:17 +0000 Gerrit-HasComments: Yes
