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

Fix variable expand between same symbol

Browse Source 
Summary:
This diff fixes the variable expand operator to work correctly then the start
and destination nodes use the same symbol or when the destination symbol is an
existing symbol.

Previously, the variable expand operator produced paths that were both
completely wrong (they shouldn't have been produced) and nonexistent (they
didn't even exist in the storage). Invalid data was produced because of a
wrong equality check that was introduced in D1703.

This issue was reported externally and the supplied test case was:
```
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
MATCH path = (pers:Person {id: 3})-[:KNOWS*2]->(pers) RETURN path;
```

Also, tests have been added so the behavior remains correct.

Reviewers: buda

Reviewed By: buda

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2793
This commit is contained in:
Matej Ferencevic 2020-06-29 14:28:54 +02:00
parent 0a2f2bfc90
commit 28590aea53
3 changed files with 361 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -533,7 +533,7 @@ bool CheckExistingNode(const VertexAccessor &new_node,
const TypedValue &existing_node = frame[existing_node_sym];
if (existing_node.IsNull()) return false;
ExpectType(existing_node_sym, existing_node, TypedValue::Type::Vertex);
return existing_node.ValueVertex() != new_node;
return existing_node.ValueVertex() == new_node;
}
template <class TEdges>
@ -834,11 +834,12 @@ class ExpandVariableCursor : public Cursor {
// if lower bound is zero we also yield empty paths
if (lower_bound_ == 0) {
auto &start_vertex = frame[self_.input_symbol_].ValueVertex();
if (!self_.common_.existing_node ||
CheckExistingNode(start_vertex, self_.common_.node_symbol,
frame)) {
if (!self_.common_.existing_node) {
frame[self_.common_.node_symbol] = start_vertex;
return true;
} else if (CheckExistingNode(start_vertex, self_.common_.node_symbol,
frame)) {
return true;
}
}
// if lower bound is not zero, we just continue, the next
@ -1016,11 +1017,9 @@ class ExpandVariableCursor : public Cursor {
? current_edge.first.From()
: current_edge.first.To();
if (self_.common_.existing_node &&
!CheckExistingNode(current_vertex, self_.common_.node_symbol, frame))
continue;
frame[self_.common_.node_symbol] = current_vertex;
if (!self_.common_.existing_node) {
frame[self_.common_.node_symbol] = current_vertex;
}
// Skip expanding out of filtered expansion.
frame[self_.filter_lambda_.inner_edge_symbol] = current_edge.first;
@ -1039,6 +1038,10 @@ class ExpandVariableCursor : public Cursor {
edges_it_.emplace_back(edges_.back().begin());
}
if (self_.common_.existing_node &&
!CheckExistingNode(current_vertex, self_.common_.node_symbol, frame))
continue;
// We only yield true if we satisfy the lower bound.
if (static_cast<int64_t>(edges_on_frame.size()) >= lower_bound_)
return true;

108
tests/qa/tests/memgraph_V1/features/match.feature
View File

@ -513,3 +513,111 @@ Feature: Match
| size(path) |
| 0 |
| 1 |
Scenario: Variable expand to existing symbol 1
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 3})-[:KNOWS*2]->(pers) RETURN path;
"""
Then the result should be empty
Scenario: Variable expand to existing symbol 2
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 3})-[:KNOWS*]->(pers) RETURN path
"""
Then the result should be:
| path |
| <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> |
Scenario: Variable expand to existing symbol 3
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 3})-[:KNOWS*0..]->(pers) RETURN path
"""
Then the result should be:
| path |
| <(:Person{id:3})> |
| <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> |
Scenario: Variable expand to existing symbol 4
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 3})-[:KNOWS*2..6]->(pers) RETURN path
"""
Then the result should be:
| path |
| <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> |
Scenario: Variable expand to existing symbol 5
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 3})-[:KNOWS*5..]->(pers) RETURN path
"""
Then the result should be empty
Scenario: Variable expand to existing symbol 6
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 1})-[:KNOWS*]->(pers) RETURN path
"""
Then the result should be
| path |
| <(:Person{id:1})-[:KNOWS]->(:Person{id:1})> |
Scenario: Variable expand to existing symbol 7
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 1})-[:KNOWS*0..]->(pers) RETURN path
"""
Then the result should be
| path |
| <(:Person{id:1})> |
| <(:Person{id:1})-[:KNOWS]->(:Person{id:1})> |
Scenario: Variable expand to existing symbol 8
Given an empty graph
And having executed:
"""
CREATE (p1:Person {id: 1})-[:KNOWS]->(p1);
"""
When executing query:
"""
MATCH path = (pers:Person {id: 1})-[:KNOWS*2..]->(pers) RETURN path
"""
Then the result should be empty

241
tests/unit/query_plan_match_filter_return.cpp
View File

@ -803,6 +803,247 @@ TEST_F(QueryPlanExpandVariable, NamedPath) {
expected_paths.begin()));
}
TEST_F(QueryPlanExpandVariable, ExpandToSameSymbol) {
auto test_expand = [&](int layer, EdgeAtom::Direction direction,
std::optional<size_t> lower,
std::optional<size_t> upper, bool reverse) {
auto e = Edge("r", direction);
auto node = NODE("n");
auto symbol = symbol_table.CreateSymbol("n", true);
node->identifier_->MapTo(symbol);
auto logical_op =
std::make_shared<ScanAll>(nullptr, symbol, storage::View::OLD);
auto n_from = ScanAllTuple{node, logical_op, symbol};
auto filter_op = std::make_shared<Filter>(
n_from.op_,
storage.Create<query::LabelsTest>(
n_from.node_->identifier_, std::vector<LabelIx>{storage.GetLabelIx(
dba.LabelToName(labels[layer]))}));
// convert optional ints to optional expressions
auto convert = [this](std::optional<size_t> bound) {
return bound ? LITERAL(static_cast<int64_t>(bound.value())) : nullptr;
};
return GetEdgeListSizes(
std::make_shared<ExpandVariable>(
filter_op, symbol, symbol, e, EdgeAtom::Type::DEPTH_FIRST,
direction, std::vector<storage::EdgeTypeId>{}, reverse,
convert(lower), convert(upper), /* existing = */ true,
ExpansionLambda{symbol_table.CreateSymbol("inner_edge", false),
symbol_table.CreateSymbol("inner_node", false),
nullptr},
std::nullopt, std::nullopt),
e);
};
// The graph is a double chain:
// chain 0: (v:0)-(v:1)-(v:2)
// X X
// chain 1: (v:0)-(v:1)-(v:2)
// Expand from chain 0 v:0 to itself.
//
// It has a total of 3 cycles:
// 1. C0 v:0 -> C0 v:1 -> C1 v:2 -> C1 v:1 -> C0 v:0
// 2. C0 v:0 -> C0 v:1 -> C0 v:2 -> C1 v:1 -> C0 v:0
// 3. C0 v:0 -> C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:0
//
// Each cycle can be in two directions, also, we have two starting nodes: one
// in chain 0 and the other in chain 1.
for (auto reverse : {false, true}) {
// Tests with both bounds set.
for (int lower_bound = 0; lower_bound < 10; ++lower_bound) {
for (int upper_bound = lower_bound; upper_bound < 10; ++upper_bound) {
map_int expected_directed;
map_int expected_undirected;
if (lower_bound == 0) {
expected_directed.emplace(0, 2);
expected_undirected.emplace(0, 2);
}
if (lower_bound <= 4 && upper_bound >= 4) {
expected_undirected.emplace(4, 12);
}
if (lower_bound <= 8 && upper_bound >= 8) {
expected_undirected.emplace(8, 24);
}
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, lower_bound,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, lower_bound,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, lower_bound,
upper_bound, reverse),
expected_undirected);
}
}
// Test only upper bound.
for (int upper_bound = 0; upper_bound < 10; ++upper_bound) {
map_int expected_directed;
map_int expected_undirected;
if (upper_bound >= 4) {
expected_undirected.emplace(4, 12);
}
if (upper_bound >= 8) {
expected_undirected.emplace(8, 24);
}
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, std::nullopt,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, std::nullopt,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, std::nullopt,
upper_bound, reverse),
expected_undirected);
}
// Test only lower bound.
for (int lower_bound = 0; lower_bound < 10; ++lower_bound) {
map_int expected_directed;
map_int expected_undirected;
if (lower_bound == 0) {
expected_directed.emplace(0, 2);
expected_undirected.emplace(0, 2);
}
if (lower_bound <= 4) {
expected_undirected.emplace(4, 12);
}
if (lower_bound <= 8) {
expected_undirected.emplace(8, 24);
}
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, lower_bound,
std::nullopt, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, lower_bound,
std::nullopt, reverse),
expected_directed);
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, lower_bound,
std::nullopt, reverse),
expected_undirected);
}
// Test no bounds.
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, std::nullopt,
std::nullopt, reverse),
(map_int{}));
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, std::nullopt,
std::nullopt, reverse),
(map_int{}));
EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, std::nullopt,
std::nullopt, reverse),
(map_int{{4, 12}, {8, 24}}));
}
// Expand from chain 0 v:1 to itself.
//
// It has a total of 6 cycles:
// 1. C0 v:1 -> C1 v:0 -> C1 v:1 -> C1 v:2 -> C0 v:1
// 2. C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:2 -> C0 v:1
// 3. C0 v:1 -> C0 v:0 -> C1 v:1 -> C1 v:2 -> C0 v:1
// 4. C0 v:1 -> C0 v:0 -> C1 v:1 -> C0 v:2 -> C0 v:1
// 5. C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:0 -> C0 v:1
// 6. C0 v:1 -> C1 v:2 -> C1 v:1 -> C0 v:2 -> C0 v:1
//
// Each cycle can be in two directions, also, we have two starting nodes: one
// in chain 0 and the other in chain 1.
for (auto reverse : {false, true}) {
// Tests with both bounds set.
for (int lower_bound = 0; lower_bound < 10; ++lower_bound) {
for (int upper_bound = lower_bound; upper_bound < 10; ++upper_bound) {
map_int expected_directed;
map_int expected_undirected;
if (lower_bound == 0) {
expected_directed.emplace(0, 2);
expected_undirected.emplace(0, 2);
}
if (lower_bound <= 4 && upper_bound >= 4) {
expected_undirected.emplace(4, 24);
}
if (lower_bound <= 8 && upper_bound >= 8) {
expected_undirected.emplace(8, 48);
}
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, lower_bound,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, lower_bound,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, lower_bound,
upper_bound, reverse),
expected_undirected);
}
}
// Test only upper bound.
for (int upper_bound = 0; upper_bound < 10; ++upper_bound) {
map_int expected_directed;
map_int expected_undirected;
if (upper_bound >= 4) {
expected_undirected.emplace(4, 24);
}
if (upper_bound >= 8) {
expected_undirected.emplace(8, 48);
}
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, std::nullopt,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, std::nullopt,
upper_bound, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, std::nullopt,
upper_bound, reverse),
expected_undirected);
}
// Test only lower bound.
for (int lower_bound = 0; lower_bound < 10; ++lower_bound) {
map_int expected_directed;
map_int expected_undirected;
if (lower_bound == 0) {
expected_directed.emplace(0, 2);
expected_undirected.emplace(0, 2);
}
if (lower_bound <= 4) {
expected_undirected.emplace(4, 24);
}
if (lower_bound <= 8) {
expected_undirected.emplace(8, 48);
}
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, lower_bound,
std::nullopt, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, lower_bound,
std::nullopt, reverse),
expected_directed);
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, lower_bound,
std::nullopt, reverse),
expected_undirected);
}
// Test no bounds.
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, std::nullopt,
std::nullopt, reverse),
(map_int{}));
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, std::nullopt,
std::nullopt, reverse),
(map_int{}));
EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, std::nullopt,
std::nullopt, reverse),
(map_int{{4, 24}, {8, 48}}));
}
}
namespace std {
template <>
struct hash<std::pair<int, int>> {