You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2018/08/07 00:56:11 UTC
[4/5] impala git commit: IMPALA-7397: fix DCHECK in
impala::AggregationNode::Close
IMPALA-7397: fix DCHECK in impala::AggregationNode::Close
As part of IMPALA-110, all of the logic of performing aggregations was
refactored out of the aggregation ExecNode and into Aggregators. Each
Aggregator manages its own memory, so a DCHECK was added in
AggregationNode::Close to ensure that no allocations were
made from the regular ExecNode mem pools.
This DCHECK is violated if the node was assigned conjuncts that
allocate mem in Prepare - even though the conjuncts are evaluated in
the Aggregator, we still initialize them in ExecNode::Prepare.
This patch solves the problem by creating a copy of the TPlanNode
without the conjuncts to pass to ExecNode. In the future, when
TAggregator is introduced, we can get rid of this by directly
assigning conjuncts to Aggregators.
Note that this doesn't affect StreamingAggregationNode, which never
has conjuncts assigned to it, but this patch fixes an incorrect DCHECK
that enforces this.
Testing:
- Added a regression test for this case.
Change-Id: I65ae00ac23a62ab9f4a7ff06ac9ad5cece80c39d
Reviewed-on: http://gerrit.cloudera.org:8080/11132
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/70bf9ea2
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/70bf9ea2
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/70bf9ea2
Branch: refs/heads/master
Commit: 70bf9ea29636a6fbecfd7a003cc746d3a0046edb
Parents: cf5de09
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
Authored: Mon Aug 6 19:06:40 2018 +0000
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Aug 6 23:45:08 2018 +0000
----------------------------------------------------------------------
be/src/exec/aggregation-node.cc | 6 +++++-
be/src/exec/streaming-aggregation-node.cc | 2 +-
.../functional-query/queries/QueryTest/aggregation.test | 8 ++++++++
3 files changed, 14 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/70bf9ea2/be/src/exec/aggregation-node.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/aggregation-node.cc b/be/src/exec/aggregation-node.cc
index 2c95590..9f76768 100644
--- a/be/src/exec/aggregation-node.cc
+++ b/be/src/exec/aggregation-node.cc
@@ -38,7 +38,11 @@ AggregationNode::AggregationNode(
: ExecNode(pool, tnode, descs) {}
Status AggregationNode::Init(const TPlanNode& tnode, RuntimeState* state) {
- RETURN_IF_ERROR(ExecNode::Init(tnode, state));
+ // The conjuncts will be evaluated in the Aggregator, so don't pass them to the
+ // ExecNode. TODO: remove this once we assign conjuncts directly to Aggregators.
+ TPlanNode tnode_no_conjuncts(tnode);
+ tnode_no_conjuncts.__set_conjuncts(std::vector<TExpr>());
+ RETURN_IF_ERROR(ExecNode::Init(tnode_no_conjuncts, state));
if (tnode.agg_node.grouping_exprs.empty()) {
aggregator_.reset(new NonGroupingAggregator(this, pool_, tnode, state->desc_tbl()));
} else {
http://git-wip-us.apache.org/repos/asf/impala/blob/70bf9ea2/be/src/exec/streaming-aggregation-node.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/streaming-aggregation-node.cc b/be/src/exec/streaming-aggregation-node.cc
index c1e9184..7280849 100644
--- a/be/src/exec/streaming-aggregation-node.cc
+++ b/be/src/exec/streaming-aggregation-node.cc
@@ -33,7 +33,7 @@ namespace impala {
StreamingAggregationNode::StreamingAggregationNode(
ObjectPool* pool, const TPlanNode& tnode, const DescriptorTbl& descs)
: ExecNode(pool, tnode, descs), child_eos_(false) {
- DCHECK(conjunct_evals_.empty()) << "Preaggs have no conjuncts";
+ DCHECK(tnode.conjuncts.empty()) << "Preaggs have no conjuncts";
DCHECK(!tnode.agg_node.grouping_exprs.empty()) << "Streaming preaggs do grouping";
DCHECK(limit_ == -1) << "Preaggs have no limits";
}
http://git-wip-us.apache.org/repos/asf/impala/blob/70bf9ea2/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
index 88dd97c..7aba138 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/aggregation.test
@@ -1370,3 +1370,11 @@ from x
---- TYPES
DOUBLE,DOUBLE,DOUBLE,DOUBLE
====
+---- QUERY
+# IMPALA-7397: conjunct that makes allocations in Prepare (extract) assigned to agg node
+select timestamp_col, count(int_col) from alltypesagg group by timestamp_col, int_col
+having extract(hour from timestamp_col) = int_col
+---- TYPES
+TIMESTAMP,BIGINT
+---- RESULTS
+====