From b33b28467b79c3e8d1a07a8bb790d48fc44decb7 Mon Sep 17 00:00:00 2001 From: Anurag Date: Fri, 15 May 2020 15:27:35 +0530 Subject: [PATCH] K-Shortest path query fix (#5410) * Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254bdbeb8d2f80cd12cc41da1fdbd60cebd1) --- query/common_test.go | 2 +- query/query3_test.go | 413 ++++++++++++++++++++++++++++++++++++++++--- query/shortest.go | 21 ++- 3 files changed, 404 insertions(+), 32 deletions(-) diff --git a/query/common_test.go b/query/common_test.go index a3892a5c9ed..4f9cf7886ba 100644 --- a/query/common_test.go +++ b/query/common_test.go @@ -686,7 +686,7 @@ func populateCluster() { <510> <511> . <510> <512> . - <51> <52> (weight=10) . + <51> <52> (weight=11) . <51> <53> (weight=1) . <51> <54> (weight=10) . diff --git a/query/query3_test.go b/query/query3_test.go index 6c223b857d2..967452055c2 100644 --- a/query/query3_test.go +++ b/query/query3_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + "github.com/dgraph-io/dgraph/testutil" "github.com/stretchr/testify/require" "google.golang.org/grpc/metadata" ) @@ -561,6 +562,298 @@ func TestKShortestPathWeighted1MinMaxWeight(t *testing.T) { `, js) } +func TestKShortestPathDepth(t *testing.T) { + // Shortest path between 1 and 1000 is the path 1 => 31 => 1001 => 1000 + // but if the depth is less than 3 then there is no direct path between + // 1 and 1000. Also if depth >=5 there is another path + // 1 => 31 => 1001 => 1003 => 1002 => 1000 + query := ` + query test ($depth: int, $numpaths: int) { + path as shortest(from: 1, to: 1000, depth: $depth, numpaths: $numpaths) { + follow + } + me(func: uid(path)) { + name + } + }` + + emptyPath := `{"data": {"me":[]}}` + + onePath := `{ + "data": { + "me": [ + {"name": "Michonne"}, + {"name": "Andrea"}, + {"name": "Bob"}, + {"name": "Alice"} + ], + "_path_": [ + { + "follow": { + "follow": { + "follow": { + "uid": "0x3e8" + }, + "uid": "0x3e9" + }, + "uid": "0x1f" + }, + "uid": "0x1", + "_weight_": 3 + } + ] + } + }` + twoPaths := `{ + "data": { + "me": [ + {"name": "Michonne"}, + {"name": "Andrea"}, + {"name": "Bob"}, + {"name": "Alice"} + ], + "_path_": [ + { + "follow": { + "follow": { + "follow": { + "uid": "0x3e8" + }, + "uid": "0x3e9" + }, + "uid": "0x1f" + }, + "uid": "0x1", + "_weight_": 3 + }, + { + "follow": { + "follow": { + "follow": { + "follow": { + "follow": { + "uid": "0x3e8" + }, + "uid": "0x3ea" + }, + "uid": "0x3eb" + }, + "uid": "0x3e9" + }, + "uid": "0x1f" + }, + "uid": "0x1", + "_weight_": 5 + } + ] + } + }` + tests := []struct { + depth, numpaths, output string + }{ + { + "2", + "4", + emptyPath, + }, + { + "3", + "4", + onePath, + }, + { + "4", + "4", + onePath, + }, + { + "5", + "4", + twoPaths, + }, + { + "6", + "4", + twoPaths, + }, + } + + t.Parallel() + for _, tc := range tests { + t.Run(fmt.Sprintf("depth_%s_numpaths_%s", tc.depth, tc.numpaths), func(t *testing.T) { + js, err := processQueryWithVars(t, query, map[string]string{"$depth": tc.depth, + "$numpaths": tc.numpaths}) + require.NoError(t, err) + require.JSONEq(t, tc.output, js) + }) + } +} + +func TestKShortestPathTwoPaths(t *testing.T) { + query := ` + { + A as shortest(from: 51, to:55, numpaths: 2, depth:2) { + connects + } + me(func: uid(A)) { + name + } + }` + js := processQueryNoErr(t, query) + require.JSONEq(t, `{ + "data": { + "me": [ + { + "name": "A" + }, + { + "name": "D" + }, + { + "name": "E" + } + ], + "_path_": [ + { + "connects": { + "connects": { + "uid": "0x37" + }, + "uid": "0x36" + }, + "uid": "0x33", + "_weight_": 2 + }, + { + "connects": { + "connects": { + "connects": { + "uid": "0x37" + }, + "uid": "0x36" + }, + "uid": "0x34" + }, + "uid": "0x33", + "_weight_": 3 + } + ] + } + }`, js) +} + +// There are 5 paths between 51 to 55 under "connects" predicate. +// This tests checks if the algorithm finds only 5 paths and doesn't add +// cyclical paths when forced to search for 6 or more paths. +func TestKShortestPathAllPaths(t *testing.T) { + for _, q := range []string{ + `{A as shortest(from: 51, to:55, numpaths: 5) {connects @facets(weight)} + me(func: uid(A)) {name}}`, + `{A as shortest(from: 51, to:55, numpaths: 6) {connects @facets(weight)} + me(func: uid(A)) {name}}`, + `{A as shortest(from: 51, to:55, numpaths: 10) {connects @facets(weight)} + me(func: uid(A)) {name}}`, + } { + js := processQueryNoErr(t, q) + expected := `{ + "data": { + "me": [ + {"name": "A"}, + {"name": "C"}, + {"name": "D"}, + {"name": "E"} + ], + "_path_": [ + { + "connects": { + "connects": { + "connects": { + "uid": "0x37" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 1, + "uid": "0x35" + }, + "connects|weight": 1, + "uid": "0x33", + "_weight_": 3 + }, + { + "connects": { + "connects": { + "uid": "0x37" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 10, + "uid": "0x33", + "_weight_": 11 + }, + { + "connects": { + "connects": { + "connects": { + "connects": { + "uid": "0x37" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 10, + "uid": "0x34" + }, + "connects|weight": 10, + "uid": "0x35" + }, + "connects|weight": 1, + "uid": "0x33", + "_weight_": 22 + }, + { + "connects": { + "connects": { + "connects": { + "uid": "0x37" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 10, + "uid": "0x34" + }, + "connects|weight": 11, + "uid": "0x33", + "_weight_": 22 + }, + { + "connects": { + "connects": { + "connects": { + "connects": { + "uid": "0x37" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 1, + "uid": "0x35" + }, + "connects|weight": 10, + "uid": "0x34" + }, + "connects|weight": 11, + "uid": "0x33", + "_weight_": 23 + } + ] + } + }` + testutil.CompareJSON(t, expected, js) + } +} func TestTwoShortestPath(t *testing.T) { query := ` @@ -746,7 +1039,6 @@ func TestShortestPathWithUidVariableNoMatchForFrom(t *testing.T) { require.JSONEq(t, `{"data":{}}`, js) } -// TODO - Later also extend this to k-shortest path. func TestShortestPathWithDepth(t *testing.T) { // Shortest path between A and B is the path A => C => D => B but if the depth is less than 3 // then the direct path between A and B should be returned. @@ -783,9 +1075,9 @@ func TestShortestPathWithDepth(t *testing.T) { "connects": { "uid": "0x34" }, - "connects|weight": 10, + "connects|weight": 11, "uid": "0x33", - "_weight_": 10 + "_weight_": 11 } ] } @@ -835,6 +1127,83 @@ func TestShortestPathWithDepth(t *testing.T) { emptyPath := `{"data":{"path":[]}}` + allPaths := `{ + "data": { + "path": [ + {"uid": "0x33","name": "A"}, + {"uid": "0x35","name": "C"}, + {"uid": "0x36","name": "D"}, + {"uid": "0x34","name": "B"} + ], + "_path_": [ + { + "connects": { + "connects": { + "connects": { + "uid": "0x34" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 1, + "uid": "0x35" + }, + "connects|weight": 1, + "uid": "0x33", + "_weight_": 3 + }, + { + "connects": { + "connects": { + "uid": "0x34" + }, + "connects|weight": 10, + "uid": "0x35" + }, + "connects|weight": 1, + "uid": "0x33", + "_weight_": 11 + }, + { + "connects": { + "uid": "0x34" + }, + "connects|weight": 11, + "uid": "0x33", + "_weight_": 11 + }, + { + "connects": { + "connects": { + "uid": "0x34" + }, + "connects|weight": 1, + "uid": "0x36" + }, + "connects|weight": 10, + "uid": "0x33", + "_weight_": 11 + }, + { + "connects": { + "connects": { + "connects": { + "uid": "0x34" + }, + "connects|weight": 10, + "uid": "0x35" + }, + "connects|weight": 10, + "uid": "0x36" + }, + "connects|weight": 10, + "uid": "0x33", + "_weight_": 30 + } + ] + } + }` + tests := []struct { depth, numpaths, output string }{ @@ -863,33 +1232,27 @@ func TestShortestPathWithDepth(t *testing.T) { "1", shortestPath, }, + //The test cases below are for k-shortest path queries with varying depths. { "0", "10", emptyPath, }, - // The test cases below are for k-shortest path queries with varying depths. They don't pass - // right now and hence are commented out... - // { - // "1", - // "10", - // directPath, - // }, - // { - // "2", - // "10", - // directPath, - // }, - // { - // "3", - // "10", - // shortestPath, - // }, - // { - // "10", - // "10", - // shortestPath, - // }, + { + "1", + "10", + directPath, + }, + { + "2", + "10", + allPaths, + }, + { + "10", + "10", + allPaths, + }, } t.Parallel() diff --git a/query/shortest.go b/query/shortest.go index d05c3dab6f0..35505af5e35 100644 --- a/query/shortest.go +++ b/query/shortest.go @@ -63,6 +63,15 @@ var errFacet = errors.Errorf("Skip the edge") type priorityQueue []*queueItem +func (r *route) indexOf(uid uint64) int { + for i, val := range *r.route { + if val.uid == uid { + return i + } + } + return -1 +} + func (h priorityQueue) Len() int { return len(h) } func (h priorityQueue) Less(i, j int) bool { return h[i].cost < h[j].cost } @@ -303,7 +312,7 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { } heap.Push(&pq, srcNode) - numHops := -1 + numHops := 0 maxHops := math.MaxInt32 if sg.Params.ExploreDepth != nil { maxHops = int(*sg.Params.ExploreDepth) @@ -340,7 +349,7 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { break } } - if item.hop > numHops && numHops < maxHops { + if item.hop > numHops-1 && numHops < maxHops { // Explore the next level by calling processGraph and add them // to the queue. if !stopExpansion { @@ -364,9 +373,6 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { case <-ctx.Done(): return nil, ctx.Err() default: - if stopExpansion { - continue - } } neighbours := adjacencyMap[item.uid] for toUid, info := range neighbours { @@ -375,7 +381,10 @@ func runKShortestPaths(ctx context.Context, sg *SubGraph) ([]*SubGraph, error) { if item.cost+cost > maxWeight { continue } - + // Skip neighbour if it present in current path to remove cyclical paths + if len(*item.path.route) > 0 && item.path.indexOf(toUid) != -1 { + continue + } curPath := pathPool.Get().(*[]pathInfo) if curPath == nil { return nil, errors.Errorf("Sync pool returned a nil pointer")