tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix aggregation functions on null and group-by inputs (#448)

Browse Source 
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.
This commit is contained in:
gvolfing 2022-07-19 12:16:32 +02:00 committed by GitHub
parent ff2f8031a9
commit eb0b3141d5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 86 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/query/plan/operator.cpp
View File

@ -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_)

65
tests/gql_behave/tests/memgraph_V1/features/aggregations.feature
View File

@ -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 |

25
tests/unit/query_plan_accumulate_aggregate.cpp
View File

@ -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