Fix handling of depth parameter for shortest path query. #4347

pawanrawal · 2019-12-04T05:58:56Z

Fixes #4169. This change doesn't fix handling of depth for k-shortest path query, it only does so for shortest path query with numpaths as 1. A separate change is required for fixing depth for shortest path queries with numpaths > 1.

This change is

…e is 0.

…th filter.

animesh2049

Reviewable status: 0 of 6 files reviewed, 7 unresolved discussions (waiting on @animesh2049 and @pawanrawal)

a discussion (no related file):
Djikstra logic seems okay to me. Just have some minor comment.

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

	heap.Push(&pq, srcNode)

	numHops := -1

Don't we want numHops to be initialized with 0? But I guess runKShortestPaths will be fixed in another PR.

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

	}
	pq := make(priorityQueue, 0)
	heap.Init(&pq)

Queue is empty. Do we really need to call heap.Init ?

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

		uid:  sg.Params.From,
		cost: 0,
		hop:  0,

Just a question, what does node.hop mean?

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

						attr:  neighbour.attr,
						facet: neighbour.facet,
					},

Can we update map entry outside if-else block? Code will look like this

var node *queueItem
if !ok {
	// This is the first time we're seeing this node. So
	// create a new node and add it to the heap and map.
	node := &queueItem{
		uid:  toUID,
		cost: nodeCost,
	}
	heap.Push(&pq, node)
} else {
	// We've already seen this node. So, just update the cost
	// and fix the priority in the heap and map.
	node := dist[toUID].node
	node.cost = nodeCost
	heap.Fix(&pq, node.index)
}

dist[toUID] = nodeInfo{
	parent: item.uid,
	node:   node,
	mapItem: mapItem{
		cost:  nodeCost,
		attr:  neighbour.attr,
		facet: neighbour.facet,
	},
}

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

	cur := sg.Params.To
	totalWeight = dist[cur].cost
	for i := 0; cur != sg.Params.From && i < len(dist); i++ {

Can we iterate for numHops times and check?
Loop will look something like

for i := numHops; i >= 0; i--

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

	}

	result = append(result, cur)

After above changes, we probably won't need this line.

martinmr

Make sure you track the issue for numpaths > 1 either in github or JIRA.

Reviewed 2 of 6 files at r1, 4 of 4 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @animesh2049 and @pawanrawal)

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

ExploreDepth *uint64

Will the value of this pointer be accessed concurrently at all?

query/query3_test.go, line 707 at r2 (raw file):

`{"data":{"path":[]}}`,

you use this multiple times so you could use a var called emptyPath or similar.

query/query3_test.go, line 747 at r2 (raw file):

Quoted 20 lines of code…

		// {
		// 	"1",
		// 	"10",
		// 	directPath,
		// },
		// {
		// 	"2",
		// 	"10",
		// 	directPath,
		// },
		// {
		// 	"3",
		// 	"10",
		// 	shortestPath,
		// },
		// {
		// 	"10",
		// 	"10",
		// 	shortestPath,
		// },

Why is this commented out?

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Just a question, what does node.hop mean?

I think it's the number of hops from the source node.

pawanrawal

Sure, created a JIRA ticket for it @martinmr.

Reviewable status: 4 of 6 files reviewed, 10 unresolved discussions (waiting on @animesh2049 and @martinmr)

a discussion (no related file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Djikstra logic seems okay to me. Just have some minor comment.

Done.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

ExploreDepth *uint64
Will the value of this pointer be accessed concurrently at all?

Nope, I don't see it being accessed concurrently for the same subgraph.

query/query3_test.go, line 707 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

`{"data":{"path":[]}}`,
you use this multiple times so you could use a var called emptyPath or similar.

Done.

query/query3_test.go, line 747 at r2 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

		// {
		// 	"1",
		// 	"10",
		// 	directPath,
		// },
		// {
		// 	"2",
		// 	"10",
		// 	directPath,
		// },
		// {
		// 	"3",
		// 	"10",
		// 	shortestPath,
		// },
		// {
		// 	"10",
		// 	"10",
		// 	shortestPath,
		// },

Why is this commented out?

These are test cases for k-shortest path as the second parameter is numpaths here... Will un-comment them when I have a fix for them. Added a comment.

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Don't we want numHops to be initialized with 0? But I guess runKShortestPaths will be fixed in another PR.

Yes, will fix k-shortest path in another PR.

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Queue is empty. Do we really need to call heap.Init ?

Nope, good point. Removed.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

I think it's the number of hops from the source node.

Added a comment to the struct definition.

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Can we update map entry outside if-else block? Code will look like this

var node *queueItem
if !ok {
	// This is the first time we're seeing this node. So
	// create a new node and add it to the heap and map.
	node := &queueItem{
		uid:  toUID,
		cost: nodeCost,
	}
	heap.Push(&pq, node)
} else {
	// We've already seen this node. So, just update the cost
	// and fix the priority in the heap and map.
	node := dist[toUID].node
	node.cost = nodeCost
	heap.Fix(&pq, node.index)
}

dist[toUID] = nodeInfo{
	parent: item.uid,
	node:   node,
	mapItem: mapItem{
		cost:  nodeCost,
		attr:  neighbour.attr,
		facet: neighbour.facet,
	},
}

done

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

Can we iterate for numHops times and check?
Loop will look something like
for i := numHops; i >= 0; i-- 

Done.

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

After above changes, we probably won't need this line.

Done.

… out while numHops < maxHops.

pawanrawal

Reviewable status: 4 of 6 files reviewed, 10 unresolved discussions (waiting on @animesh2049 and @martinmr)

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

Previously, pawanrawal (Pawan Rawal) wrote…

Done.

Actually reverted because it won't be correct. Length of the shortest path could be greater than number of hops. So if we expandOut only twice (i.e. call processGraph twice), the shortest path be of length 3.

animesh2049

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @martinmr and @pawanrawal)

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

Previously, pawanrawal (Pawan Rawal) wrote…

Done.

With the current logic, source node (sg.Params.From won't be included in result). Is that the desired behavior?

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

		item := heap.Pop(&pq).(*queueItem)
		if item.uid == sg.Params.To {
			totalWeight = item.cost

I think we are not using this value of totalWeight. Deep source also points this out.

manishrjain

Reviewed 2 of 6 files at r1, 2 of 4 files at r2, 1 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @martinmr and @pawanrawal)

query/shortest.go, line 557 at r4 (raw file):

				// We've already seen this node. So, just update the cost
				// and fix the priority in the heap and map.
				node = dist[toUID].node

We don't enforce capital ID in Dgraph.

martinmr

There's a comment from Animesh that hasn't been marked as done but once that is done it

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pawanrawal)

pawanrawal

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @animesh2049 and @manishrjain)

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

With the current logic, source node (sg.Params.From won't be included in result). Is that the desired behavior?

It is included because we break out of loop after.

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

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

I think we are not using this value of totalWeight. Deep source also points this out.

Changed it

query/shortest.go, line 557 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

We don't enforce capital ID in Dgraph.

Done.

pawanrawal added 12 commits November 14, 2019 16:43

Add some test cases based on the issue and start working on the fix.

ea30432

Add a couple of more test cases.

ecae59e

Refactor shortest path function a bit.

a9570bc

Fix existing query tests.

df65575

Skip a test, make some other parallel.

1dff0de

Remove a TODO which isn't correct.

6493b84

Remove some more indentation and make it simpler.

cb4a039

Add some more comments.

1d6ff70

Change ExploreDepth to be a pointer and return empty path if its valu…

5e82755

…e is 0.

Call updateUidMatrix in shortest path query processing to fix test wi…

22a00cd

…th filter.

Add test cases for k shortest path but comment them out for now.

2b6692b

Fix k shortest path, depth: 0 case.

3a2857e

pawanrawal marked this pull request as ready for review December 4, 2019 08:03

pawanrawal requested a review from animesh2049 December 4, 2019 08:05

animesh2049 suggested changes Dec 4, 2019

View reviewed changes

Bring back item.hop in shortest path query and add a test for it.

080d67d

pawanrawal requested a review from martinmr December 4, 2019 13:58

Move numHops++ inside the if block.

1b59eb3

martinmr suggested changes Dec 4, 2019

View reviewed changes

pawanrawal added 2 commits December 5, 2019 09:23

Address review comments by Animesh and Martin.

6fd1762

Add a comment about commented out test cases.

b4654e3

pawanrawal commented Dec 5, 2019

View reviewed changes

Keep on poping from the heap till we run out of elements. Only expand…

71d63f9

… out while numHops < maxHops.

pawanrawal commented Dec 5, 2019

View reviewed changes

animesh2049 suggested changes Dec 5, 2019

View reviewed changes

Remove unnecessary assignment to totalWeight

049170d

manishrjain approved these changes Dec 5, 2019

View reviewed changes

martinmr approved these changes Dec 6, 2019

View reviewed changes

pawanrawal commented Dec 9, 2019

View reviewed changes

pawanrawal merged commit 50802e1 into master Dec 11, 2019

pawanrawal deleted the pawanrawal/fix-4169 branch December 11, 2019 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of depth parameter for shortest path query. #4347

Fix handling of depth parameter for shortest path query. #4347

pawanrawal commented Dec 4, 2019 •

edited

Loading

animesh2049 left a comment

martinmr left a comment

pawanrawal left a comment

pawanrawal left a comment

animesh2049 left a comment

manishrjain left a comment

martinmr left a comment

pawanrawal left a comment

Fix handling of depth parameter for shortest path query. #4347

Fix handling of depth parameter for shortest path query. #4347

Conversation

pawanrawal commented Dec 4, 2019 • edited Loading

animesh2049 left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

animesh2049 left a comment

Choose a reason for hiding this comment

manishrjain left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

pawanrawal commented Dec 4, 2019 •

edited

Loading