K-Shortest path query fix #5410

all-seeing-code · 2020-05-09T17:31:58Z

Motivation

This PR fixes:

DGRAPH-794 -- Depth parameter has an off-by-one bug and in certain cases all paths are not returned even if depth is set to maximum.
DGRAPH-1371 -- Cyclical paths are not removed from the output.

For detailed discussion please refer here.

Components affected by this PR

K-shortest path queries with k > 1

Does this PR break backwards compatibility?

No. But it is a breaking fix in a sense that because of bug, code wasn't filtering out cyclical paths in all cases. This PR ensures that cyclical paths are filtered out. Hence code-output can change in some cases.

Testing

Added following tests in query3_test.go:

For numpaths>1 check if correct paths are returned for different values of depth parameter. Also test that the paths returned become constant after a certain depth when the graph becomes saturated.
Check that cyclical paths are not returned.

Fixes

Fixes DGRAPH-794
Fixes DGRAPH-1371

This change is

Docs Preview:

…esult

gja

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @anurags92 and @pawanrawal)

query/shortest.go, line 337 at r1 (raw file):

			newRoute := item.path
			newRoute.totalWeight = item.cost
			kroutes = append(kroutes, newRoute)

if len(newRoute) <= depth ? <- To be discussed

ashish-goswami

Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @anurags92 and @pawanrawal)

query/shortest.go, line 73 at r2 (raw file):

	sort.Slice(tmp, func(i, j int) bool {
		return tmp[i].uid < tmp[j].uid
	})

Slice sorting and the searching is expensive here. Just a linear search would work fine.

query/shortest.go, line 383 at r2 (raw file):

		default:
			if len(kroutes) == numPaths {
				break

This will break out of switch case and not for loop. Use a labelled for loop here.

pawanrawal

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92)

query/query3_test.go, line 683 at r3 (raw file):

	query := `
	{
		A as shortest(from: 51, to:55, numpaths: 5) {

Seems like the output of this one and the test below is the same. Could you give this query various inputs like numpaths 5, 6, 10, 100 etc. and see it still returns the same result.

Could you have a test which checks this for various values of depth as well along with a fixed value of numpaths say 5. Have cases for depth 0, 1, 2, 3, 4, 5, 10 and 100.

query/shortest.go, line 378 at r3 (raw file):

			return nil, ctx.Err()
		default:
			if len(kroutes) == numPaths {

Is this useful here? We already check this when we append to kroutes above. Does anything fail if you remove this?

query/shortest.go, line 389 at r3 (raw file):

				continue
			}
			if len(*item.path.route) > 0 && item.path.indexOf(toUid) != -1 {

This needs a comment on what it does and why is it needed. Also mention about the associated test case here.

ashish-goswami

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92)

query/shortest.go, line 66 at r3 (raw file):

func (r *route) indexOf(uid uint64) int {

You can pass a pointer to route that could avoid unnecessary copy.

query/query3_test.go

2. Code clean-up

all-seeing-code

Reviewable status: 0 of 3 files reviewed, 8 unresolved discussions (waiting on @ashish-goswami, @gja, and @pawanrawal)

query/query3_test.go, line 683 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Seems like the output of this one and the test below is the same. Could you give this query various inputs like numpaths 5, 6, 10, 100 etc. and see it still returns the same result.

Could you have a test which checks this for various values of depth as well along with a fixed value of numpaths say 5. Have cases for depth 0, 1, 2, 3, 4, 5, 10 and 100.

Addressed. Also added the tests which were earlier commented out. They work fine as well.

query/query3_test.go, line 593 at r4 (raw file):

Previously, gja (Tejas Dinkar) wrote…

can these be compressed a bit to not use so much vertical space
me: [
{"name": "x"},
{"name": "y"}
..
]

Yes, thanks. I have manually tried to minimize vertical spaces. But except for the name key, other params are nested. So could achieve only partial benefits.

query/shortest.go, line 337 at r1 (raw file):

Previously, gja (Tejas Dinkar) wrote…

if len(newRoute) <= depth ? <- To be discussed

As discussed in the discuss post, continuing with the current implementation of depth.

query/shortest.go, line 73 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

Slice sorting and the searching is expensive here. Just a linear search would work fine.

Done.

query/shortest.go, line 383 at r2 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

This will break out of switch case and not for loop. Use a labelled for loop here.

no applicable anymore.

query/shortest.go, line 66 at r3 (raw file):

Previously, ashish-goswami (Ashish Goswami) wrote…

func (r *route) indexOf(uid uint64) int {
You can pass a pointer to route that could avoid unnecessary copy.

Done.

query/shortest.go, line 378 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Is this useful here? We already check this when we append to kroutes above. Does anything fail if you remove this?

Removed.

query/shortest.go, line 389 at r3 (raw file):

// Skip neighbour if the cost is greater than the maximum weight allowed.

query/shortest.go, line 389 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This needs a comment on what it does and why is it needed. Also mention about the associated test case here.

Added. But didn't mention the test cases. It is being checked in multiple ways.

pawanrawal

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, and @gja)

query/query3_test.go, line 1240 at r5 (raw file):

			emptyPath,
		},
		//The test cases below are for k-shortest path queries with varying depths. They don't pass

This comment can be removed

manishrjain

Just ensure that the documentation explains this "changed" behavior.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, @gja, and @pawanrawal)

manishrjain

Do mention that this is a breaking fix -- if the user's results might change now.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, @gja, and @pawanrawal)

all-seeing-code · 2020-05-14T18:16:42Z

Just ensure that the documentation explains this "changed" behavior.

Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @anurags92, @ashish-goswami, @gja, and @pawanrawal)

Added a documentation JIRA Ticket https://dgraph.atlassian.net/browse/DGRAPH-1369

* Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254)

…5596) * K-Shortest path query fix (#5410) * Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254) * Edit test cases to remove flaky behavior (#5464) (cherry picked from commit bae2b1b) * Added details of Shortest path queries in documentation (#5533) (cherry picked from commit abb57c0)

…5548) * K-Shortest path query fix (#5410) * Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254) * Edit test cases to remove flaky behavior (#5464) (cherry picked from commit bae2b1b) * Added details of Shortest path queries in documentation (#5533) (cherry picked from commit abb57c0)

* Bug fix for depth parameter * Handle cycles in K - Shortest path queries

all-seeing-code added 2 commits May 3, 2020 15:49

Add print statements for debug

d77e15a

Add length check on kroutes so that it adds more paths to the final r…

17ed5fd

…esult

all-seeing-code requested a review from pawanrawal as a code owner May 9, 2020 17:31

all-seeing-code added 6 commits May 10, 2020 01:02

Commit out leftover prints

a1f4670

Remove print statements

0451374

Remove more print statements

5fabbf6

Merge branch 'master' into anurags92/KshortestPathQuery

d6f8b45

Bug fix for depth parameter

13f4c78

Code clean-up

23493a0

gja suggested changes May 11, 2020

View reviewed changes

all-seeing-code added 3 commits May 12, 2020 16:15

Add print statements for debugging

7aaa3cc

Handle cycles in KShortest path queries

243f4ef

Code clean-up

70712a1

ashish-goswami suggested changes May 13, 2020

View reviewed changes

all-seeing-code added 6 commits May 13, 2020 15:00

Do linear search for index

b9e01c2

Merge branch 'master' into anurags92/KshortestPathQuery

da58050

Add tests for depth param in K-shortest queries

e180624

Add more test cases to check Kshortest path queries

59cdccc

Modify test case to limit it to depth=2

13e1087

Add weights to the query to make the order deterministic

33ede43

pawanrawal suggested changes May 14, 2020

View reviewed changes

ashish-goswami suggested changes May 14, 2020

View reviewed changes

Addressed code clean-up comments

daa10f8

gja approved these changes May 14, 2020

View reviewed changes

query/query3_test.go Outdated Show resolved Hide resolved

1. Add more test cases in tabular fashion.

7a7063c

2. Code clean-up

all-seeing-code commented May 14, 2020

View reviewed changes

pawanrawal approved these changes May 14, 2020

View reviewed changes

Minor code clean-up

a88251f

manishrjain reviewed May 14, 2020

View reviewed changes

Remove non-deterministic test case

4414f8c

Merge branch 'master' into anurags92/KshortestPathQuery

a3de73d

all-seeing-code mentioned this pull request May 15, 2020

alpha breakdown-shortest search #4861

Closed

all-seeing-code merged commit 6332254 into master May 15, 2020

lgalatin mentioned this pull request May 15, 2020

Shortest path query returns paths with repeated nodes. #5426

Closed

all-seeing-code added a commit that referenced this pull request May 29, 2020

K-Shortest path query fix (#5410)

b33b284

* Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254)

all-seeing-code mentioned this pull request May 29, 2020

release/v20.03 K shortest paths queries fix and documentation update #5548

Merged

all-seeing-code added a commit that referenced this pull request Jun 8, 2020

K-Shortest path query fix (#5410)

b091e93

* Bug fix for depth parameter * Handle cycles in K - Shortest path queries (cherry picked from commit 6332254)

all-seeing-code mentioned this pull request Jun 8, 2020

release/v1.2 K shortest paths queries fix and documentation update #5596

Merged

dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020

K-Shortest path query fix (dgraph-io#5410)

d9fe1d0

* Bug fix for depth parameter * Handle cycles in K - Shortest path queries

all-seeing-code deleted the anurags92/KshortestPathQuery branch September 24, 2020 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

K-Shortest path query fix #5410

K-Shortest path query fix #5410

all-seeing-code commented May 9, 2020 •

edited

Loading

gja left a comment

ashish-goswami left a comment

pawanrawal left a comment

ashish-goswami left a comment

all-seeing-code left a comment

pawanrawal left a comment

manishrjain left a comment

manishrjain left a comment

all-seeing-code commented May 14, 2020 •

edited by manishrjain

Loading

K-Shortest path query fix #5410

K-Shortest path query fix #5410

Conversation

all-seeing-code commented May 9, 2020 • edited Loading

Motivation

Components affected by this PR

Does this PR break backwards compatibility?

Testing

Fixes

gja left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

ashish-goswami left a comment

Choose a reason for hiding this comment

all-seeing-code left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

all-seeing-code commented May 14, 2020 • edited by manishrjain Loading

all-seeing-code commented May 9, 2020 •

edited

Loading

all-seeing-code commented May 14, 2020 •

edited by manishrjain

Loading