From eb0b3141d51e679cecf6d85588696b039677b9bf Mon Sep 17 00:00:00 2001 From: gvolfing <107616712+gvolfing@users.noreply.github.com> Date: Tue, 19 Jul 2022 12:16:32 +0200 Subject: [PATCH] Fix aggregation functions on `null` and group-by inputs (#448) The `sum()` and `count()` functions were giving results different from the openCypher specification on null `input.` The aggregation functions also had a problem when they were used in a group-by context and were giving results that were not compliant with the openCypher specification. --- src/query/plan/operator.cpp | 10 ++- .../memgraph_V1/features/aggregations.feature | 65 +++++++++++++++++++ .../unit/query_plan_accumulate_aggregate.cpp | 25 ++++--- 3 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 74463ead0..a4ae9da66 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -2600,13 +2600,13 @@ namespace { * when there are */ TypedValue DefaultAggregationOpValue(const Aggregate::Element &element, utils::MemoryResource *memory) { switch (element.op) { - case Aggregation::Op::COUNT: - return TypedValue(0, memory); - case Aggregation::Op::SUM: case Aggregation::Op::MIN: case Aggregation::Op::MAX: case Aggregation::Op::AVG: return TypedValue(memory); + case Aggregation::Op::COUNT: + case Aggregation::Op::SUM: + return TypedValue(0, memory); case Aggregation::Op::COLLECT_LIST: return TypedValue(TypedValue::TVector(memory)); case Aggregation::Op::COLLECT_MAP: @@ -2628,9 +2628,7 @@ class AggregateCursor : public Cursor { pulled_all_input_ = true; aggregation_it_ = aggregation_.begin(); - // in case there is no input and no group_bys we need to return true - // just this once - if (aggregation_.empty() && self_.group_by_.empty()) { + if (aggregation_.empty()) { auto *pull_memory = context.evaluation_context.memory; // place default aggregation values on the frame for (const auto &elem : self_.aggregations_) diff --git a/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature b/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature index 09776a466..796f598eb 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature +++ b/tests/gql_behave/tests/memgraph_V1/features/aggregations.feature @@ -73,6 +73,16 @@ Feature: Aggregations | n | | 5 | + Scenario: Count test 07: + Given an empty graph + When executing query: + """ + RETURN count(null) + """ + Then the result should be: + | count(null) | + | 0 | + Scenario: Sum test 01: Given an empty graph And having executed @@ -114,6 +124,16 @@ Feature: Aggregations | 4 | 0 | | 4 | 1 | + Scenario: Sum test 04: + Given an empty graph + When executing query: + """ + RETURN sum(null) + """ + Then the result should be: + | sum(null) | + | 0 | + Scenario: Avg test 01: Given an empty graph And having executed @@ -155,6 +175,16 @@ Feature: Aggregations | 2.0 | 0 | | 4.0 | 1 | + Scenario: Avg test 04: + Given an empty graph + When executing query: + """ + RETURN avg(null) + """ + Then the result should be: + | avg(null) | + | null | + Scenario: Min test 01: Given an empty graph And having executed @@ -196,6 +226,16 @@ Feature: Aggregations | 1 | 0 | | 4 | 1 | + Scenario: Min test 04: + Given an empty graph + When executing query: + """ + RETURN min(null) + """ + Then the result should be: + | min(null) | + | null | + Scenario: Max test 01: Given an empty graph And having executed @@ -237,6 +277,16 @@ Feature: Aggregations | 3 | 0 | | 4 | 1 | + Scenario: Max test 04: + Given an empty graph + When executing query: + """ + RETURN max(null) + """ + Then the result should be: + | max(null) | + | null | + Scenario: Collect test 01: Given an empty graph And having executed @@ -279,3 +329,18 @@ Feature: Aggregations | n | | {a_key: 13, b_key: 11, c_key: 12} | + Scenario: Combined aggregations - some evauluates to null: + Given an empty graph + And having executed + """ + CREATE (f) + CREATE (n {property: 1}) + """ + When executing query: + """ + MATCH (n) RETURN count(n) < n.property, count(n.property), count(n), avg(n.property), min(n.property), max(n.property), sum(n.property) + """ + Then the result should be: + | count(n) < n.property | count(n.property) | count(n) | avg(n.property) | min(n.property) | max(n.property) | sum(n.property) | + | false | 1 | 1 | 1.0 | 1 | 1 | 1 | + | null | 0 | 1 | null | null | null | 0 | diff --git a/tests/unit/query_plan_accumulate_aggregate.cpp b/tests/unit/query_plan_accumulate_aggregate.cpp index 04bd1cc44..08f1965f0 100644 --- a/tests/unit/query_plan_accumulate_aggregate.cpp +++ b/tests/unit/query_plan_accumulate_aggregate.cpp @@ -217,31 +217,40 @@ TEST_F(QueryPlanAggregateOps, WithData) { TEST_F(QueryPlanAggregateOps, WithoutDataWithGroupBy) { { auto results = AggregationResults(true, {Aggregation::Op::COUNT}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); + EXPECT_EQ(results[0][0].ValueInt(), 0); } { auto results = AggregationResults(true, {Aggregation::Op::SUM}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Int); + EXPECT_EQ(results[0][0].ValueInt(), 0); } { auto results = AggregationResults(true, {Aggregation::Op::AVG}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); } { auto results = AggregationResults(true, {Aggregation::Op::MIN}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); } { auto results = AggregationResults(true, {Aggregation::Op::MAX}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Null); } { auto results = AggregationResults(true, {Aggregation::Op::COLLECT_LIST}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::List); } { auto results = AggregationResults(true, {Aggregation::Op::COLLECT_MAP}); - EXPECT_EQ(results.size(), 0); + EXPECT_EQ(results.size(), 1); + EXPECT_EQ(results[0][0].type(), TypedValue::Type::Map); } } @@ -260,7 +269,7 @@ TEST_F(QueryPlanAggregateOps, WithoutDataWithoutGroupBy) { // max EXPECT_TRUE(results[0][3].IsNull()); // sum - EXPECT_TRUE(results[0][4].IsNull()); + EXPECT_EQ(results[0][4].ValueInt(), 0); // avg EXPECT_TRUE(results[0][5].IsNull()); // collect list