8000 Bug fix/graph node keep all flags on clone (#14635) (#14642) · arangodb/arangodb@f890a13 · GitHub
[go: up one dir, main page]

Skip to content

Commit f890a13

Browse files
mchackiKVS85
andauthored
Bug fix/graph node keep all flags on clone (#14635) (#14642)
Co-authored-by: Vadim <vadim@arangodb.com>
1 parent 4a42fe6 commit f890a13

File tree

10 files changed

+427
-4
lines changed

10 files changed

+427
-4
lines changed

CHANGELOG

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
v3.8.1 (XXXX-XX-XX)
22
-------------------
33

4-
* Fix wrong assertion in fuerte and move it to where the TLA+ model says
5-
it should be. This fixes a unit test failure occurring on newer Macs
6-
with a certain clang version.
4+
* Bug-Fix: In more complex queries there was a code-path where a (Disjoint-)
5+
Smart graph access was not properly optimized.
6+
7+
* Fix wrong assertion in fuerte and move it to where the TLA+ model says i
8+
should be. This fixes a unit test failure occurring on newer Macs with a
9+
certain clang version.
710

811
* When creating Pregel memory-mapped files, create them with O_TMPFILE attribute
912
on Linux so that files are guaranteed to vanish even if a process dies.

arangod/Aql/GraphNode.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -618,6 +618,11 @@ void GraphNode::toVelocyPackHelper(VPackBuilder& nodes, unsigned flags,
618618
_options->toVelocyPackIndexes(nodes);
619619
}
620620

621+
void GraphNode::graphCloneHelper(ExecutionPlan&, GraphNode& clone, bool) const {
622+
clone._isSmart = _isSmart;
623+
clone._isDisjoint = _isDisjoint;
624+
}
625+
621626
CostEstimate GraphNode::estimateCost() const {
622627
CostEstimate estimate = _dependencies.at(0)->getCost();
623628
size_t incoming = estimate.estimatedNrItems;

arangod/Aql/GraphNode.h

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,21 @@ class GraphNode : public ExecutionNode {
202202
public:
203203
graph::Graph const* graph() const noexcept;
204204

205+
#ifdef ARANGODB_USE_GOOGLE_TESTS
206+
// Internal helpers used in tests to modify enterprise detections.
207+
// These should not be used in production, as their detection
208+
// is implemented in constructors.
209+
void setIsSmart(bool target) {
210+
_isSmart = target;
211+
}
212+
213+
void setIsDisjoint(bool target) {
214+
_isDisjoint = target;
215+
}
216+
#endif
217+
protected:
218+
void graphCloneHelper(ExecutionPlan& plan, GraphNode& clone, bool withProperties) const;
219+
205220
private:
206221
void addEdgeCollection(aql::Collections const& collections, std::string const& name, TRI_edge_direction_e dir);
207222
void addEdgeCollection(aql::Collection& collection, TRI_edge_direction_e dir);

arangod/Aql/KShortestPathsNode.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -457,6 +457,7 @@ ExecutionNode* KShortestPathsNode::clone(ExecutionPlan* plan, bool withDependenc
457457
void KShortestPathsNode::kShortestPathsCloneHelper(ExecutionPlan& plan,
458458
KShortestPathsNode& c,
459459
bool withProperties) const {
460+
graphCloneHelper(plan, c, withProperties);
460461
if (usesPathOutVariable()) {
461462
auto pathOutVariable = _pathOutVariable;
462463
if (withProperties) {

arangod/Aql/ShortestPathNode.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ ExecutionNode* ShortestPathNode::clone(ExecutionPlan* plan, bool withDependencie
327327

328328
void ShortestPathNode::shortestPathCloneHelper(ExecutionPlan& plan, ShortestPathNode& c,
329329
bool withProperties) const {
330+
graphCloneHelper(plan, c, withProperties);
330331
if (isVertexOutVariableUsedLater()) {
331332
auto vertexOutVariable = _vertexOutVariable;
332333
if (withProperties) {

arangod/Aql/TraversalNode.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -587,6 +587,7 @@ ExecutionNode* TraversalNode::clone(ExecutionPlan* plan, bool withDependencies,
587587

588588
void TraversalNode::traversalCloneHelper(ExecutionPlan& plan, TraversalNode& c,
589589
bool const withProperties) const {
590+
graphCloneHelper(plan, c, withProperties);
590591
if (isVertexOutVariableAccessed()) {
591592
auto vertexOutVariable = _vertexOutVariable;
592593
if (withProperties) {
@@ -616,7 +617,6 @@ void TraversalNode::traversalCloneHelper(ExecutionPlan& plan, TraversalNode& c,
616617

617618
c._conditionVariables.reserve(_conditionVariables.size());
618619
for (auto const& it : _conditionVariables) {
619-
//#warning TODO: check if not cloning variables breaks anything
620620
if (withProperties) {
621621
c._conditionVariables.emplace(it->clone());
622622
} else {
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
////////////////////////////////////////////////////////////////////////////////
2+
/// DISCLAIMER
3+
///
4+
/// Copyright 2021-2021 ArangoDB GmbH, Cologne, Germany
5+
///
6+
/// Licensed under the Apache License, Version 2.0 (the "License");
7+
/// you may not use this file except in compliance with the License.
8+
/// You may obtain a copy of the License at
9+
///
10+
/// http://www.apache.org/licenses/LICENSE-2.0
11+
///
12+
/// Unless required by applicable law or agreed to in writing, software
13+
/// distributed under the License is distributed on an "AS IS" BASIS,
14+
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
/// See the License for the specific language governing permissions and
16+
/// limitations under the License.
17+
///
18+
/// Copyright holder is ArangoDB GmbH, Cologne, Germany
19+
///
20+
/// @author Michael Hackstein
21+
/// @author Copyright 2021, ArangoDB GmbH, Cologne, Germany
22+
////////////////////////////////////////////////////////////////////////////////
23+
24+
#include "gtest/gtest.h"
25+
26+
#include "Aql/Ast.h"
27+
#include "Aql/KShortestPathsNode.h"
28+
#include "Aql/Query.h"
29+
#include "Graph/BaseOptions.h"
30+
#include "Graph/ShortestPathOptions.h"
31+
#include "Mocks/Servers.h"
32+
33+
#include <memory>
34+
35+
using namespace arangodb;
36+
using namespace arangodb::aql;
37+
using namespace arangodb::graph;
38+
using namespace arangodb::tests::mocks;
39+
40+
namespace arangodb {
41+
namespace tests {
42+
namespace aql {
43+
class KShortestPathsNodeTest : public ::testing::Test {
44+
MockAqlServer _server{};
45+
std::unique_ptr<Query> _query{_server.createFakeQuery()};
46+
std::unique_ptr<Query> _otherQuery{_server.createFakeQuery()};
47+
std::string _startNode{"v/123"};
48+
AstNode* _source{nullptr};
49+
AstNode* _target{nullptr};
50+
AstNode* _direction{nullptr};
51+
AstNode* _graph{nullptr};
52+
53+
public:
54+
KShortestPathsNodeTest() {
55+
auto ast = _query->ast();
56+
_source = ast->createNodeValueString(_startNode.c_str(), _startNode.length());
57+
_target = ast->createNodeValueString(_startNode.c_str(), _startNode.length());
58+
_direction = ast->createNodeDirection(0, 1);
59+
AstNode* edges = ast->createNodeArray(0);
60+
_graph = ast->createNodeCollectionList(edges, _query->resolver());
61+
}
62+
63+
ExecutionPlan* plan() const { return _query->plan(); }
64+
ExecutionPlan* otherPlan(bool emptyQuery = false) {
65+
if (emptyQuery) {
66+
// Let us start a new blank query
67+
_otherQuery = _server.createFakeQuery();
68+
}
69+
return _otherQuery->plan();
70+
}
71+
72+
KShortestPathsNode createNode(ExecutionNodeId id,
73+
std::unique_ptr<ShortestPathOptions> opts) const {
74+
return KShortestPathsNode{plan(),
75+
id,
76+
&_query->vocbase(),
77+
ShortestPathType::Type::KShortestPaths,
78+
_direction,
79+
_source,
80+
_target,
81+
_graph,
82+
std::move(opts)};
83+
};
84+
85+
std::unique_ptr<ShortestPathOptions> makeOptions() const {
86+
return std::make_unique<ShortestPathOptions>(*_query);
87+
}
88+
};
89+
90+
TEST_F(KShortestPathsNodeTest, clone_should_preserve_isSmart) {
91+
ExecutionNodeId id(12);
92+
auto opts = makeOptions();
93+
KShortestPathsNode original = createNode(id, std::move(opts));
94+
ASSERT_EQ(original.id(), id);
95+
for (bool keepPlan : std::vector<bool>{false, true}) {
96+
for (bool value : std::vector<bool>{false, true}) {
97+
auto p = keepPlan ? plan() : otherPlan(true);
98+
original.setIsSmart(value);
99+
auto clone =
100+
ExecutionNode::castTo<KShortestPathsNode*>(original.clone(p, false, !keepPlan));
101+
if (keepPlan) {
102+
EXPECT_NE(clone->id(), original.id()) << "Clone did keep the id";
103+
} else {
104+
EXPECT_EQ(clone->id(), original.id()) << "Clone did not keep the id";
105+
}
106+
EXPECT_EQ(original.isSmart(), value);
107+
EXPECT_EQ(clone->isSmart(), value);
108+
}
109+
}
110+
}
111+
112+
TEST_F(KShortestPathsNodeTest, clone_should_preserve_isDisjoint) {
113+
ExecutionNodeId id(12);
114+
auto opts = makeOptions();
115+
KShortestPathsNode original = createNode(id, std::move(opts));
116+
ASSERT_EQ(original.id(), id);
117+
for (bool keepPlan : std::vector<bool>{false, true}) {
118+
for (bool value : std::vector<bool>{false, true}) {
119+
auto p = keepPlan ? plan() : otherPlan(true);
120+
original.setIsDisjoint(value);
121+
auto clone =
122+
ExecutionNode::castTo<KShortestPathsNode*>(original.clone(p, false, !keepPlan));
123+
if (keepPlan) {
124+
EXPECT_NE(clone->id(), original.id()) << "Clone did keep the id";
125+
} else {
126+
EXPECT_EQ(clone->id(), original.id()) << "Clone did not keep the id";
127+
}
128+
EXPECT_EQ(original.isDisjoint(), value);
129+
EXPECT_EQ(clone->isDisjoint(), value);
130+
}
131+
}
132+
}
133+
134+
} // namespace aql
135+
} // namespace tests
136+
} // namespace arangodb

tests/Aql/ShortestPathNodeTest.cpp

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
////////////////////////////////////////////////////////////////////////////////
2+
/// DISCLAIMER
3+
///
4+
/// Copyright 2021-2021 ArangoDB GmbH, Cologne, Germany
5+
///
6+
/// Licensed under the Apache License, Version 2.0 (the "License");
7+
/// you may not use this file except in compliance with the License.
8+
/// You may obtain a copy of the License at
9+
///
10+
/// http://www.apache.org/licenses/LICENSE-2.0
11+
///
12+
/// Unless required by applicable law or agreed to in writing, software
13+
/// distributed under the License is distributed on an "AS IS" BASIS,
14+
/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
/// See the License for the specific language governing permissions and
16+
/// limitations under the License.
17+
///
18+
/// Copyright holder is ArangoDB GmbH, Cologne, Germany
19+
///
20+
/// @author Michael Hackstein
21+
/// @author Copyright 2021, ArangoDB GmbH, Cologne, Germany
22+
////////////////////////////////////////////////////////////////////////////////
23+
24+
#include "gtest/gtest.h"
25+
26+
#include "Aql/Ast.h"
27+
#include "Aql/Query.h"
28+
#include "Aql/ShortestPathNode.h"
29+
#include "Graph/BaseOptions.h"
30+
#include "Graph/ShortestPathOptions.h"
31+
#include "Mocks/Servers.h"
32+
33+
#include <memory>
34+
35+
using namespace arangodb;
36+
using namespace arangodb::aql;
37+
using namespace arangodb::graph;
38+
using namespace arangodb::tests::mocks;
39+
40+
namespace arangodb {
41+
namespace tests {
42+
namespace aql {
43+
class ShortestPathNodeTest : public ::testing::Test {
44+
MockAqlServer _server{};
45+
std::unique_ptr<Query> _query{_server.createFakeQuery()};
46+
std::unique_ptr<Query> _otherQuery{_server.createFakeQuery()};
47+
std::string _startNode{"v/123"};
48+
AstNode* _source{nullptr};
49+
AstNode* _target{nullptr};
50+
AstNode* _direction{nullptr};
51+
AstNode* _graph{nullptr};
52+
53+
public:
54+
ShortestPathNodeTest() {
55+
auto ast = _query->ast();
56+
_source = ast->createNodeValueString(_startNode.c_str(), _startNode.length());
57+
_target = ast->createNodeValueString(_startNode.c_str(), _startNode.length());
58+
_direction = ast->createNodeDirection(0, 1);
59+
AstNode* edges = ast->createNodeArray(0);
60+
_graph = ast->createNodeCollectionList(edges, _query->resolver());
61+
}
62+
63+
ExecutionPlan* plan() const { return _query->plan(); }
64+
ExecutionPlan* otherPlan(bool emptyQuery = false) {
65+
if (emptyQuery) {
66+
// Let us start a new blank query
67+
_otherQuery = _server.createFakeQuery();
68+
}
69+
return _otherQuery->plan();
70+
}
71+
72+
ShortestPathNode createNode(ExecutionNodeId id,
73+
std::unique_ptr<ShortestPathOptions> opts) const {
74+
return ShortestPathNode{
75+
plan(), id, &_query->vocbase(), _direction, _source,
76+
_target, _graph, std::move(opts)};
77+
};
78+
79+
std::unique_ptr<ShortestPathOptions> makeOptions() const {
80+
return std::make_unique<ShortestPathOptions>(*_query);
81+
}
82+
};
83+
84+
TEST_F(ShortestPathNodeTest, clone_should_preserve_isSmart) {
85+
ExecutionNodeId id(12);
86+
auto opts = makeOptions();
87+
ShortestPathNode original = createNode(id, std::move(opts));
88+
ASSERT_EQ(original.id(), id);
89+
for (bool keepPlan : std::vector<bool>{false, true}) {
90+
for (bool value : std::vector<bool>{false, true}) {
91+
auto p = keepPlan ? plan() : otherPlan(true);
92+
original.setIsSmart(value);
93+
auto clone =
94+
ExecutionNode::castTo<ShortestPathNode*>(original.clone(p, false, !keepPlan));
95+
if (keepPlan) {
96+
EXPECT_NE(clone->id(), original.id()) << "Clone did keep the id";
97+
} else {
98+
EXPECT_EQ(clone->id(), original.id()) << "Clone did not keep the id";
99+
}
100+
EXPECT_EQ(original.isSmart(), value);
101+
EXPECT_EQ(clone->isSmart(), value);
102+
}
103+
}
104+
}
105+
106+
TEST_F(ShortestPathNodeTest, clone_should_preserve_isDisjoint) {
107+
ExecutionNodeId id(12);
108+
auto opts = makeOptions();
109+
ShortestPathNode original = createNode(id, std::move(opts));
110+
ASSERT_EQ(original.id(), id);
111+
for (bool keepPlan : std::vector<bool>{false, true}) {
112+
for (bool value : std::vector<bool>{false, true}) {
113+
auto p = keepPlan ? plan() : otherPlan(true);
114+
original.setIsDisjoint(value);
115+
auto clone =
116+
ExecutionNode::castTo<ShortestPathNode*>(original.clone(p, false, !keepPlan));
117+
if (keepPlan) {
118+
EXPECT_NE(clone->id(), original.id()) << "Clone did keep the id";
119+
} else {
120+
EXPECT_EQ(clone->id(), original.id()) << "Clone did not keep the id";
121+
}
122+
EXPECT_EQ(original.isDisjoint(), value);
123+
EXPECT_EQ(clone->isDisjoint(), value);
124+
}
125+
}
126+
}
127+
128+
} // namespace aql
129+
} // namespace tests
130+
} // namespace arangodb

0 commit comments

Comments
 (0)
0