diff --git a/CHANGELOG b/CHANGELOG index 815cb63f9d..e8563d83ba 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,9 @@ v3.5.3 (XXXX-XX-XX) ------------------- +* Fixed issue #10371: K_SHORTEST_PATHS LIMIT 1 can not return the shortest path. + Now the shortest path is returned as the first one in such queries. + * Improve killability of some types of cluster AQL queries. Previously, several cluster queries, especially those containing a `DistributeNode` in their execution plans, did not respond to a kill instruction. diff --git a/arangod/Graph/KShortestPathsFinder.cpp b/arangod/Graph/KShortestPathsFinder.cpp index c02b4d58fc..04e11e33f8 100644 --- a/arangod/Graph/KShortestPathsFinder.cpp +++ b/arangod/Graph/KShortestPathsFinder.cpp @@ -70,27 +70,36 @@ bool KShortestPathsFinder::computeShortestPath(VertexRef const& start, VertexRef std::unordered_set const& forbiddenVertices, std::unordered_set const& forbiddenEdges, Path& result) { - bool found = false; Ball left(start, FORWARD); Ball right(end, BACKWARD); VertexRef join; result.clear(); - while (!left._frontier.empty() && !right._frontier.empty() && !found) { + auto currentBest = boost::optional{}; + + // We will not improve anymore if we have found a best path and the smallest + // combined distance between left and right is bigger than that path + while (!left._frontier.empty() && !right._frontier.empty() && + !(currentBest.has_value() && + (left._closest + right._closest > currentBest.value()))) { _options.isQueryKilledCallback(); // Choose the smaller frontier to expand. if (left._frontier.size() < right._frontier.size()) { - found = advanceFrontier(left, right, forbiddenVertices, forbiddenEdges, join); + advanceFrontier(left, right, forbiddenVertices, forbiddenEdges, join, currentBest); } else { - found = advanceFrontier(right, left, forbiddenVertices, forbiddenEdges, join); + advanceFrontier(right, left, forbiddenVertices, forbiddenEdges, join, currentBest); } } - if (found) { + + if (currentBest.has_value()) { reconstructPath(left, right, join, result); + return true; + } else { + // No path found + return false; } - return found; } void KShortestPathsFinder::computeNeighbourhoodOfVertexCache(VertexRef vertex, @@ -178,10 +187,11 @@ void KShortestPathsFinder::computeNeighbourhoodOfVertex(VertexRef vertex, Direct } } -bool KShortestPathsFinder::advanceFrontier(Ball& source, Ball const& target, +void KShortestPathsFinder::advanceFrontier(Ball& source, Ball const& target, std::unordered_set const& forbiddenVertices, std::unordered_set const& forbiddenEdges, - VertexRef& join) { + VertexRef& join, + boost::optional& currentBest) { VertexRef vr; DijkstraInfo *v, *w; std::vector* neighbours; @@ -190,7 +200,7 @@ bool KShortestPathsFinder::advanceFrontier(Ball& source, Ball const& target, TRI_ASSERT(v != nullptr); TRI_ASSERT(vr == v->_vertex); if (!success) { - return false; + return; } computeNeighbourhoodOfVertexCache(vr, source._direction, neighbours); @@ -217,14 +227,17 @@ bool KShortestPathsFinder::advanceFrontier(Ball& source, Ball const& target, } } v->_done = true; + source._closest = v->_weight; w = target._frontier.find(v->_vertex); if (w != nullptr && w->_done) { - join = v->_vertex; - return true; + // The total weight of the found path + double totalWeight = v->_weight + w->_weight; + if (!currentBest.has_value() || totalWeight < currentBest.value()) { + join = v->_vertex; + currentBest = v->_weight + w->_weight; + } } - - return false; } void KShortestPathsFinder::reconstructPath(Ball const& left, Ball const& right, diff --git a/arangod/Graph/KShortestPathsFinder.h b/arangod/Graph/KShortestPathsFinder.h index 36986cb2d5..4bc84114fe 100644 --- a/arangod/Graph/KShortestPathsFinder.h +++ b/arangod/Graph/KShortestPathsFinder.h @@ -34,6 +34,8 @@ #include +#include + namespace arangodb { namespace velocypack { @@ -141,9 +143,9 @@ class KShortestPathsFinder : public ShortestPathFinder { void setWeight(double weight) { _weight = weight; } DijkstraInfo(VertexRef const& vertex, Edge const&& edge, VertexRef const& pred, double weight) - : _vertex(vertex), _edge(std::move(edge)), _pred(pred), _weight(weight), _done(false) {} + : _vertex(vertex), _edge(std::move(edge)), _pred(pred), _weight(weight), _done(false) {} explicit DijkstraInfo(VertexRef const& vertex) - : _vertex(vertex), _weight(0), _done(true) {} + : _vertex(vertex), _weight(0), _done(true) {} }; typedef ShortestPathPriorityQueue Frontier; @@ -154,10 +156,13 @@ class KShortestPathsFinder : public ShortestPathFinder { VertexRef _centre; Direction _direction; Frontier _frontier; + // The distance of the last node that has been fully expanded + // from _centre + double _closest; Ball() {} Ball(VertexRef const& centre, Direction direction) - : _centre(centre), _direction(direction) { + : _centre(centre), _direction(direction), _closest(0) { _frontier.insert(centre, std::make_unique(centre)); } ~Ball() {} @@ -193,7 +198,7 @@ class KShortestPathsFinder : public ShortestPathFinder { std::vector _paths; explicit FoundVertex(VertexRef const& vertex) - : _vertex(vertex), _hasCachedOutNeighbours(false), _hasCachedInNeighbours(false) {} + : _vertex(vertex), _hasCachedOutNeighbours(false), _hasCachedInNeighbours(false) {} }; // Contains the vertices that were found while searching // for a shortest path between start and end together with @@ -246,9 +251,10 @@ class KShortestPathsFinder : public ShortestPathFinder { void computeNeighbourhoodOfVertex(VertexRef vertex, Direction direction, std::vector& steps); - bool advanceFrontier(Ball& source, Ball const& target, + void advanceFrontier(Ball& source, Ball const& target, std::unordered_set const& forbiddenVertices, - std::unordered_set const& forbiddenEdges, VertexRef& join); + std::unordered_set const& forbiddenEdges, + VertexRef& join, boost::optional& currentBest); private: bool _pathAvailable; diff --git a/tests/Graph/GraphTestTools.h b/tests/Graph/GraphTestTools.h index 0bd00476b5..f19ac5d6b4 100644 --- a/tests/Graph/GraphTestTools.h +++ b/tests/Graph/GraphTestTools.h @@ -50,6 +50,8 @@ #include "../Mocks/StorageEngineMock.h" #include "IResearch/common.h" +#include "boost/optional.hpp" + using namespace arangodb; using namespace arangodb::aql; using namespace arangodb::graph; @@ -184,9 +186,18 @@ struct MockGraphDatabase { EXPECT_TRUE(vertices->type()); } + struct EdgeDef { + EdgeDef(size_t from, size_t to) : _from(from), _to(to){}; + EdgeDef(size_t from, size_t to, double weight) + : _from(from), _to(to), _weight(weight){}; + size_t _from; + size_t _to; + boost::optional _weight; + }; + // Create a collection with name of edges given by void addEdgeCollection(std::string name, std::string vertexCollection, - std::vector> edgedef) { + std::vector edgedef) { std::shared_ptr edges; auto createJson = velocypack::Parser::fromJson("{ \"name\": \"" + name + "\", \"type\": 3 }"); @@ -205,10 +216,18 @@ struct MockGraphDatabase { for (auto& p : edgedef) { // std::cout << "edge: " << vertexCollection << " " << p.first << " -> " // << p.second << std::endl; - auto docJson = velocypack::Parser::fromJson( - "{ \"_from\": \"" + vertexCollection + "/" + std::to_string(p.first) + - "\"" + ", \"_to\": \"" + vertexCollection + "/" + - std::to_string(p.second) + "\" }"); + // This is moderately horrible + auto docJson = + p._weight.has_value() + ? velocypack::Parser::fromJson( + "{ \"_from\": \"" + vertexCollection + "/" + + std::to_string(p._from) + "\"" + ", \"_to\": \"" + + vertexCollection + "/" + std::to_string(p._to) + + "\", \"cost\": " + std::to_string(p._weight.value()) + "}") + : velocypack::Parser::fromJson( + "{ \"_from\": \"" + vertexCollection + "/" + + std::to_string(p._from) + "\"" + ", \"_to\": \"" + + vertexCollection + "/" + std::to_string(p._to) + "\" }"); docs.emplace_back(docJson); } @@ -282,12 +301,13 @@ struct MockGraphDatabase { spos.emplace_back(spo); return spo; } -}; +}; // namespace graph -bool checkPath(ShortestPathOptions* spo, ShortestPathResult result, std::vector vertices, +bool checkPath(ShortestPathOptions* spo, ShortestPathResult result, + std::vector vertices, std::vector> edges, std::string& msgs); -} -} -} +} // namespace graph +} // namespace tests +} // namespace arangodb #endif diff --git a/tests/Graph/KShortestPathsFinder.cpp b/tests/Graph/KShortestPathsFinder.cpp index f6a3b299d2..9d358fb44d 100644 --- a/tests/Graph/KShortestPathsFinder.cpp +++ b/tests/Graph/KShortestPathsFinder.cpp @@ -212,6 +212,52 @@ TEST_F(KShortestPathsFinderTest, many_edges_between_two_nodes) { ASSERT_TRUE(false == finder->getNextPathShortestPathResult(result)); } +class KShortestPathsFinderTestWeights : public ::testing::Test { + protected: + GraphTestSetup s; + MockGraphDatabase gdb; + + arangodb::aql::Query* query; + arangodb::graph::ShortestPathOptions* spo; + + KShortestPathsFinder* finder; + + KShortestPathsFinderTestWeights() : gdb("testVocbase") { + gdb.addVertexCollection("v", 10); + gdb.addEdgeCollection("e", "v", + {{1, 2, 10}, + {1, 3, 10}, + {1, 10, 100}, + {2, 4, 10}, + {3, 4, 20}, + {7, 3, 10}, + {8, 3, 10}, + {9, 3, 10}}); + + query = gdb.getQuery("RETURN 1"); + spo = gdb.getShortestPathOptions(query); + spo->weightAttribute = "cost"; + + finder = new KShortestPathsFinder(*spo); + } + + ~KShortestPathsFinderTestWeights() { delete finder; } +}; + +TEST_F(KShortestPathsFinderTestWeights, diamond_path) { + auto start = velocypack::Parser::fromJson("\"v/1\""); + auto end = velocypack::Parser::fromJson("\"v/4\""); + ShortestPathResult result; + std::string msgs; + + finder->startKShortestPathsTraversal(start->slice(), end->slice()); + + ASSERT_TRUE(finder->getNextPathShortestPathResult(result)); + auto cpr = checkPath(spo, result, {"1", "2", "4"}, + {{}, {"v/1", "v/2"}, {"v/2", "v/4"}}, msgs); + ASSERT_TRUE(cpr) << msgs; +} + } // namespace graph } // namespace tests } // namespace arangodb