8000 Traversal Bugfix (#11310) · arangodb/arangodb@f512398 · GitHub
[go: up one dir, main page]
Skip to content

Commit f512398

Browse files
markuspfmarkuspf
authored
Traversal Bugfix (#11310)
* Reset traversal on construction * Reset path enumerator properly * Cleanup * Add regression test for TraversalExecutor failure * Add back some asserts * Remove old produceRows * Add assertEqual to regression test to validate traversal result
1 parent 302578a commit f512398

File tree

5 files changed

+338
-42
lines changed

5 files changed

+338
-42
lines changed

‎arangod/Aql/TraversalExecutor.cpp

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,14 @@ std::vector<std::pair<Variable const*, RegisterId>> const& TraversalExecutorInfo
145145
}
146146

147147
TraversalExecutor::TraversalExecutor(Fetcher& fetcher, Infos& infos)
148-
: _infos(infos), _inputRow{CreateInvalidInputRowHint{}}, _traverser(infos.traverser()) {}
148+
: _infos(infos), _inputRow{CreateInvalidInputRowHint{}}, _traverser(infos.traverser()) {
149+
150+
// reset the traverser, so that no residual state is left in it. This is
151+
// important because the TraversalExecutor is sometimes reconstructed (in place)
152+
// with the same TraversalExecutorInfos as before. Those infos contain the traverser
153+
// which might contain state from a previous run.
154+
_traverser.done();
155+
}
149156

150157
TraversalExecutor::~TraversalExecutor() {
151158
auto opts = _traverser.options();
@@ -169,14 +176,7 @@ std::pair<ExecutionState, Result> TraversalExecutor::shutdown(int errorCode) {
169176
return {ExecutionState::DONE, TRI_ERROR_NO_ERROR};
170177
}
171178

172-
std::pair<ExecutionState, TraversalStats> TraversalExecutor::produceRows(OutputAqlItemRow& output) {
173-
// TODO: Remove me!
174-
TRI_ASSERT(false);
175-
return {ExecutionState::DONE, TraversalStats{}};
176-
}
177-
178179
auto TraversalExecutor::doOutput(OutputAqlItemRow& output) -> void {
179-
// TODO check whether _traverser.hasMore is obsolete here
180180
while (!output.isFull() && _traverser.hasMore() && _traverser.next()) {
181181
TRI_ASSERT(_inputRow.isInitialized());
182182

@@ -212,6 +212,7 @@ auto TraversalExecutor::doSkip(AqlCall& call) -> size_t {
212212
auto skip = size_t{0};
213213

214214
while (call.shouldSkip() && _traverser.hasMore() && _traverser.next()) {
215+
TRI_ASSERT(_inputRow.isInitialized());
215216
skip++;
216217
call.didSkip(1);
217218
}

‎arangod/Aql/TraversalExecutor.h

Lines changed: 0 additions & 8 deletions
8000
Original file line numberDiff line numberDiff line change
@@ -133,14 +133,6 @@ class TraversalExecutor {
133133
*/
134134
std::pair<ExecutionState, Result> shutdown(int errorCode);
135135

136-
/**
137-
* @brief produce the next Row of Aql Values.
138-
*
139-
* @return ExecutionState, and if successful exactly one new Row of AqlItems.
140-
*/
141-
[[nodiscard]] auto produceRows(OutputAqlItemRow& output)
142-
-> std::pair<ExecutionState, Stats>;
143-
144136
[[nodiscard]] auto produceRows(AqlItemBlockInputRange& input, OutputAqlItemRow& output)
145137
-> std::tuple<ExecutorState, Stats, AqlCall>;
146138
[[nodiscard]] auto skipRowsRange(AqlItemBlockInputRange& input, AqlCall& call)

‎arangod/Graph/PathEnumerator.cpp

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,27 +38,24 @@ using Traverser = arangodb::traverser::Traverser;
3838
using TraverserOptions = arangodb::traverser::TraverserOptions;
3939

4040
PathEnumerator::PathEnumerator(Traverser* traverser, TraverserOptions* opts)
41-
: _traverser(traverser),
42-
_isFirst(true),
43-
_opts(opts),
44-
_httpRequests(0) {}
41+
: _traverser(traverser), _isFirst(true), _opts(opts), _httpRequests(0) {}
4542

4643
PathEnumerator::~PathEnumerator() = default;
4744

4845
void PathEnumerator::setStartVertex(arangodb::velocypack::StringRef startVertex) {
4946
_enumeratedPath.edges.clear();
5047
_enumeratedPath.vertices.clear();
51-
_isFirst = true;
48+
_isFirst = true;
5249
_httpRequests = 0;
53-
50+
5451
_enumeratedPath.vertices.push_back(startVertex);
5552
TRI_ASSERT(_enumeratedPath.vertices.size() == 1);
5653
}
5754

5855
bool PathEnumerator::keepEdge(arangodb::graph::EdgeDocumentToken& eid,
5956
arangodb::velocypack::Slice edge,
60-
arangodb::velocypack::StringRef sourceVertex, size_t depth,
61-
size_t cursorId) {
57+
arangodb::velocypack::StringRef sourceVertex,
58+
size_t depth, size_t cursorId) {
6259
if (_opts->hasEdgeFilter(depth, cursorId)) {
6360
VPackSlice e = edge;
6461
if (edge.isString()) {
@@ -73,7 +70,8 @@ bool PathEnumerator::keepEdge(arangodb::graph::EdgeDocumentToken& eid,
7370
return _opts->destinationCollectionAllowed(edge, sourceVertex);
7471
}
7572

76-
graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nextVertex, uint64_t currentDepth) {
73+
graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nextVertex,
74+
uint64_t currentDepth) {
7775
if (currentDepth >= _cursors.size()) {
7876
_cursors.emplace_back(_opts->buildCursor(currentDepth));
7977
}
@@ -83,15 +81,14 @@ graph::EdgeCursor* PathEnumerator::getCursor(arangodb::velocypack::StringRef nex
8381
}
8482

8583
DepthFirstEnumerator::DepthFirstEnumerator(Traverser* traverser, TraverserOptions* opts)
86-
: PathEnumerator(traverser, opts),
87-
_activeCursors(0),
88-
_pruneNext(false) {}
84+
: PathEnumerator(traverser, opts), _activeCursors(0), _pruneNext(false) {}
8985

9086
DepthFirstEnumerator::~DepthFirstEnumerator() = default;
9187

9288
void DepthFirstEnumerator::setStartVertex(arangodb::velocypack::StringRef startVertex) {
9389
PathEnumerator::setStartVertex(startVertex);
9490

91+
_activeCursors = 0;
9592
_pruneNext = false;
9693
}
9794

@@ -114,7 +111,9 @@ bool DepthFirstEnumerator::next() {
114111
if (_enumeratedPath.edges.size() < _opts->maxDepth && !_pruneNext) {
115112
// We are not done with this path, so
116113
// we reserve the cursor for next depth
117-
graph::EdgeCursor* cursor = getCursor(arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()), _enumeratedPath.edges.size());
114+
graph::EdgeCursor* cursor =
115+
getCursor(arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()),
116+
_enumeratedPath.edges.size());
118117
incHttpRequests(cursor->httpRequests());
119118
++_activeCursors;
120119
} else {
@@ -131,7 +130,7 @@ bool DepthFirstEnumerator::next() {
131130
auto callback = [&](graph::EdgeDocumentToken&& eid, VPackSlice const& edge, size_t cursorId) {
132131
if (!keepEdge(eid, edge,
133132
arangodb::velocypack::StringRef(_enumeratedPath.vertices.back()),
134-
_enumeratedPath.edges.size(), cursorId)) {
133+
_enumeratedPath.edges.size(), cursorId)) {
135134
return;
136135
}
137136

@@ -257,13 +256,13 @@ bool DepthFirstEnumerator::shouldPrune() {
257256
if (!_opts->usesPrune()) {
258257
return false;
259258
}
260-
259+
261260
transaction::BuilderLeaser pathBuilder(_opts->trx());
262261
// evaluator->evaluate() might access these, so they have to live long
263262
// enough
264263
aql::AqlValue vertex, edge;
265264
aql::AqlValueGuard vertexGuard{vertex, true}, edgeGuard{edge, true};
266-
265+
267266
aql::PruneExpressionEvaluator* evaluator = _opts->getPruneEvaluator();
268267
TRI_ASSERT(evaluator != nullptr);
269268

@@ -281,4 +280,3 @@ bool DepthFirstEnumerator::shouldPrune() {
281280
}
282281
return evaluator->evaluate();
283282
}
284-

‎arangod/Graph/PathEnumerator.h

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ class PathEnumerator {
8282
//////////////////////////////////////////////////////////////////////////////
8383

8484
EnumeratedPath _enumeratedPath;
85-
85+
8686
//////////////////////////////////////////////////////////////////////////////
8787
/// @brief Number of HTTP requests made
8888
//////////////////////////////////////////////////////////////////////////////
@@ -96,7 +96,7 @@ class PathEnumerator {
9696
PathEnumerator(Traverser* traverser, TraverserOptions* opts);
9797

9898
virtual ~PathEnumerator();
99-
99+
100100
/// @brief set start vertex and reset
101101
/// note that the caller *must* guarantee that the string data pointed to by
102102
/// startVertex remains valid even after the call to reset()!!
@@ -113,19 +113,19 @@ class PathEnumerator {
113113
virtual aql::AqlValue lastVertexToAqlValue() = 0;
114114
virtual aql::AqlValue lastEdgeToAqlValue() = 0;
115115
virtual aql::AqlValue pathToAqlValue(arangodb::velocypack::Builder&) = 0;
116-
116+
117117
/// @brief return number of HTTP requests made, and reset it to 0
118-
size_t getAndResetHttpRequests() {
118+
size_t getAndResetHttpRequests() {
119119
size_t value = _httpRequests;
120120
_httpRequests = 0;
121121
return value;
122122
}
123-
123+
124124
void incHttpRequests(size_t requests) { _httpRequests += requests; }
125-
125+
126126
protected:
127127
graph::EdgeCursor* getCursor(arangodb::velocypack::StringRef nextVertex, uint64_t currentDepth);
128-
128+
129129
/// @brief The vector of EdgeCursors to walk through.
130130
std::vector<std::unique_ptr<graph::EdgeCursor>> _cursors;
131131
};
@@ -144,7 +144,7 @@ class DepthFirstEnumerator final : public PathEnumerator {
144144
DepthFirstEnumerator(Traverser* traverser, TraverserOptions* opts);
145145

146146
~DepthFirstEnumerator();
147-
147+
148148
/// @brief set start vertex and reset
149149
void setStartVertex(arangodb::velocypack::StringRef startVertex) override;
150150

@@ -159,7 +159,7 @@ class DepthFirstEnumerator final : public PathEnumerator {
159159

160160
private:
161161
bool shouldPrune();
162-
162+
163163
velocypack::Slice pathToSlice(arangodb::velocypack::Builder& result);
164164
};
165165
} // namespace traverser

0 commit comments

Comments
 (0)
0