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

Skip to content

Commit a33e95c

Browse files
authored
Bug fix/graph node keep all flags on clone (#14635)
1 parent 8b95339 commit a33e95c

File tree

10 files changed

+423
-1
lines changed

10 files changed

+423
-1
lines changed

CHANGELOG

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
devel
22
-----
33

4+
* Bug-Fix: In more complex queries there was a code-path where a (Disjoint-) Smart
5+
graph access was not properly optimized.
6+
47
* Improve log messages for Pregel runs by giving them more context.
58

69
* Add ReplicatedLogs column family.

arangod/Aql/GraphNode.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -614,6 +614,11 @@ void GraphNode::doToVelocyPack(VPackBuilder& nodes, unsigned flags) const {
614614
_options->toVelocyPackIndexes(nodes);
615615
}
616616

617+
void GraphNode::graphCloneHelper(ExecutionPlan&, GraphNode& clone, bool) const {
618+
clone._isSmart = _isSmart;
619+
clone._isDisjoint = _isDisjoint;
620+
}
621+
617622
CostEstimate GraphNode::estimateCost() const {
618623
CostEstimate estimate = _dependencies.at(0)->getCost();
619624
size_t incoming = estimate.estimatedNrItems;

arangod/Aql/GraphNode.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,9 +197,23 @@ class GraphNode : public ExecutionNode {
197197

198198
graph::Graph const* graph() const noexcept;
199199

200+
#ifdef ARANGODB_USE_GOOGLE_TESTS
201+
// Internal helpers used in tests to modify enterprise detections.
202+
// These should not be used in production, as their detection
203+
// is implemented in constructors.
204+
void setIsSmart(bool target) {
205+
_isSmart = target;
206+
}
207+
208+
void setIsDisjoint(bool target) {
209+
_isDisjoint = target;
210+
}
211+
#endif
200212
protected:
201213
void doToVelocyPack(arangodb::velocypack::Builder& nodes, unsigned flags) const override;
202214

215+
void graphCloneHelper(ExecutionPlan& plan, GraphNode& clone, bool withProperties) const;
216+
203217
private:
204218
void addEdgeCollection(aql::Collections const& collections, std::string const& name, TRI_edge_direction_e dir);
205219
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
@@ -452,6 +452,7 @@ ExecutionNode* KShortestPathsNode::clone(ExecutionPlan* plan, bool withDependenc
452452
void KShortestPathsNode::kShortestPathsCloneHelper(ExecutionPlan& plan,
453453
KShortestPathsNode& c,
454454
bool withProperties) const {
455+
graphCloneHelper(plan, c, withProperties);
455456
if (usesPathOutVariable()) {
456457
auto pathOutVariable = _pathOutVariable;
457458
if (withProperties) {

arangod/Aql/ShortestPathNode.cpp

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

324324
void ShortestPathNode::shortestPathCloneHelper(ExecutionPlan& plan, ShortestPathNode& c,
325325
bool withProperties) const {
326+
graphCloneHelper(plan, c, withProperties);
326327
if (isVertexOutVariableUsedLater()) {
327328
auto vertexOutVariable = _vertexOutVariable;
328329
if (withProperties) {

arangod/Aql/TraversalNode.cpp

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

705705
void TraversalNode::traversalCloneHelper(ExecutionPlan& plan, TraversalNode& c,
706706
bool const withProperties) const {
707+
graphCloneHelper(plan, c, withProperties);
707708
if (isVertexOutVariableAccessed()) {
708709
auto vertexOutVariable = _vertexOutVariable;
709710
if (withProperties) {
@@ -733,7 +734,6 @@ void TraversalNode::traversalCloneHelper(ExecutionPlan& plan, TraversalNode& c,
733734

734735
c._conditionVariables.reserve(_conditionVariables.size());
735736
for (auto const& it : _conditionVariables) {
736-
//#warning TODO: check if not cloning variables breaks anything
737737
if (withProperties) {
738738
c._conditionVariables.emplace(it->clone());
739739
} 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 358A 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