From d03fafcef6fae57c7e456372f64ea227fb84b316 Mon Sep 17 00:00:00 2001 From: Andi Date: Mon, 20 Nov 2023 11:52:17 +0100 Subject: [PATCH] Aggregations return empty result when used with group by (#1531) --- src/query/plan/operator.cpp | 8 +---- src/query/plan/operator.hpp | 2 -- .../memgraph_V1/features/aggregations.feature | 4 +-- .../features/aggregations.feature | 4 +-- .../unit/query_plan_accumulate_aggregate.cpp | 34 ++++++------------- 5 files changed, 13 insertions(+), 39 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 52e15e928..b68810ad7 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -3464,7 +3464,7 @@ class AggregateCursor : public Cursor { SCOPED_PROFILE_OP_BY_REF(self_); if (!pulled_all_input_) { - if (!ProcessAll(&frame, &context) && self_.AreAllAggregationsForCollecting()) return false; + if (!ProcessAll(&frame, &context) && !self_.group_by_.empty()) return false; pulled_all_input_ = true; aggregation_it_ = aggregation_.begin(); @@ -3824,12 +3824,6 @@ UniqueCursorPtr Aggregate::MakeCursor(utils::MemoryResource *mem) const { return MakeUniqueCursorPtr(mem, *this, mem); } -auto Aggregate::AreAllAggregationsForCollecting() const -> bool { - return std::all_of(aggregations_.begin(), aggregations_.end(), [](const auto &agg) { - return agg.op == Aggregation::Op::COLLECT_LIST || agg.op == Aggregation::Op::COLLECT_MAP; - }); -} - Skip::Skip(const std::shared_ptr &input, Expression *expression) : input_(input), expression_(expression) {} diff --git a/src/query/plan/operator.hpp b/src/query/plan/operator.hpp index ba844796a..7bb971752 100644 --- a/src/query/plan/operator.hpp +++ b/src/query/plan/operator.hpp @@ -1759,8 +1759,6 @@ class Aggregate : public memgraph::query::plan::LogicalOperator { Aggregate(const std::shared_ptr &input, const std::vector &aggregations, const std::vector &group_by, const std::vector &remember); - auto AreAllAggregationsForCollecting() const -> bool; - bool Accept(HierarchicalLogicalOperatorVisitor &visitor) override; UniqueCursorPtr MakeCursor(utils::MemoryResource *) const override; std::vector ModifiedSymbols(const SymbolTable &) const override; diff --git a/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature b/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature index cff138432..80b0ca69a 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature +++ b/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature @@ -425,6 +425,4 @@ Feature: Aggregations """ MATCH (subnet:Subnet) WHERE FALSE WITH subnet, count(subnet.ip) as ips RETURN id(subnet) as id """ - Then the result should be: - | id | - | null | + Then the result should be empty diff --git a/tests/gql_behave/tests/memgraph_V1_on_disk/features/aggregations.feature b/tests/gql_behave/tests/memgraph_V1_on_disk/features/aggregations.feature index cff138432..80b0ca69a 100644 --- a/tests/gql_behave/tests/memgraph_V1_on_disk/features/aggregations.feature +++ b/tests/gql_behave/tests/memgraph_V1_on_disk/features/aggregations.feature @@ -425,6 +425,4 @@ Feature: Aggregations """ MATCH (subnet:Subnet) WHERE FALSE WITH subnet, count(subnet.ip) as ips RETURN id(subnet) as id """ - Then the result should be: - | id | - | null | + Then the result should be empty diff --git a/tests/unit/query_plan_accumulate_aggregate.cpp b/tests/unit/query_plan_accumulate_aggregate.cpp index bbf3e0311..b1e9a62d0 100644 --- a/tests/unit/query_plan_accumulate_aggregate.cpp +++ b/tests/unit/query_plan_accumulate_aggregate.cpp @@ -250,30 +250,23 @@ TYPED_TEST(QueryPlanAggregateOps, WithData) { TYPED_TEST(QueryPlanAggregateOps, WithoutDataWithGroupBy) { { auto results = this->AggregationResults(true, false, {Aggregation::Op::COUNT}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); - EXPECT_EQ(results[0][0].ValueInt(), 0); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, false, {Aggregation::Op::SUM}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); - EXPECT_EQ(results[0][0].ValueInt(), 0); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, false, {Aggregation::Op::AVG}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, false, {Aggregation::Op::MIN}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, false, {Aggregation::Op::MAX}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, false, {Aggregation::Op::COLLECT_LIST}); @@ -666,30 +659,23 @@ TYPED_TEST(QueryPlanAggregateOps, WithDataDistinct) { TYPED_TEST(QueryPlanAggregateOps, WithoutDataWithDistinctAndWithGroupBy) { { auto results = this->AggregationResults(true, true, {Aggregation::Op::COUNT}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); - EXPECT_EQ(results[0][0].ValueInt(), 0); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, true, {Aggregation::Op::SUM}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); - EXPECT_EQ(results[0][0].ValueInt(), 0); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, true, {Aggregation::Op::AVG}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, true, {Aggregation::Op::MIN}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, true, {Aggregation::Op::MAX}); - EXPECT_EQ(results.size(), 1); - EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); + EXPECT_EQ(results.size(), 0); } { auto results = this->AggregationResults(true, true, {Aggregation::Op::COLLECT_LIST});