Support recursive queries

[ashwin95r]

This change is 

[ashishnegi]

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

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

	for _, pc := range sg.Children {

		if pc.uidMatrix == nil {

please put a comment that it can happen in recurse query.

---

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

		args.AfterUID = uint64(after)
	}
	if v, ok := gq.Args["depth"]; ok {

do we call fill for only root node ?
otherwise should we check, if gq.params.alias or something is recurse ?

---

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

			return err
		}
		args.RecurseDepth = int(from)

should we useuint64 ?
negative values are not allowed in depth.. ?
we are not going bottom up.. right ?

---

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

		algo.Sort(&o)
		sg.DestUIDs = &o
	} else if parent == nil && len(sg.SrcFunc) == 0 {

it looks like this was not required ?
was code readability/right flow was reason for this change ?

---

query/query_test.go, line 485 at r1 (raw file):

	query := `
		{
			recurse(id:0x01, depth: 2) {

i thought, depth 2 will get friends of friends ?
what happens at depth 0 ?

---

Comments from Reviewable

[ashwin95r]

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.

---

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

Previously, ashishnegi (Ashish Negi) wrote…

please put a comment that it can happen in recurse query.

Done.

---

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

Previously, ashishnegi (Ashish Negi) wrote…

do we call fill for only root node ?
otherwise should we check, if gq.params.alias or something is recurse ?

Fill can be called for any node. Yeah, for stricter handling, we can check the recurse keyword. Done.

---

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

Previously, ashishnegi (Ashish Negi) wrote…

should we useuint64 ?
negative values are not allowed in depth.. ?
we are not going bottom up.. right ?

Done.

---

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

Previously, ashishnegi (Ashish Negi) wrote…

it looks like this was not required ?
was code readability/right flow was reason for this change ?

This would be the right flow. Currently, It wouldn't make a difference as they do the same thing.

---

query/query_test.go, line 485 at r1 (raw file):

Previously, ashishnegi (Ashish Negi) wrote…

i thought, depth 2 will get friends of friends ?
what happens at depth 0 ?

We do get friends of friends, but since we don't fetch the attr, it's empty.

---

Comments from Reviewable

[manishrjain]

---

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

query/query_test.go, line 485 at r1 (raw file):

Previously, ashwin95r (Ashwin Ramesh) wrote…

We do get friends of friends, but since we don't fetch the attr, it's empty.

Went through this. Given we treat everything as an edge away, what we're currently doing makes sense.

---

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

	next chan bool, rch chan error) {

	reachMap := make(map[string]map[uint64]map[uint64]struct{})

How about just a map[string]struct{}, and use fmt.Sprintf("%s|%d|%d", ...) for key.

---

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

		start.Children = append(start.Children, temp)
		/*
			it := algo.NewListIterator(start.DestUIDs)

cleanup

---

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

		if numEdges > 1000000 {
			// If we've seen too many nodes, stop the query.
			rch <- ErrTooBig

return

---

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

	// Recurse number of times specified by the user.
	for i := uint64(0); i < depth; i++ {
		next <- false

next <- true

---

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

				if err == ErrTooBig {
					return err
				} else if err == ErrStop {

if err == ... Not else if.

---

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

				} else if err == ErrStop {
					break L
				} else {

No need for else here.

---

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

	}
	// Done expanding.
	next <- true

next <- false

---

Comments from Reviewable

[ashwin95r]

Review status: 3 of 4 files reviewed at latest revision, 8 unresolved discussions.

---

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

Previously, manishrjain (Manish R Jain) wrote…

How about just a map[string]struct{}, and use fmt.Sprintf("%s|%d|%d", ...) for key.

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

cleanup

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

return

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

next <- true

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

if err == ... Not else if.

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

No need for else here.

Done.

---

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

Previously, manishrjain (Manish R Jain) wrote…

next <- false

Done.

---

Comments from Reviewable

[ashishnegi]

Review status: 3 of 4 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

query/recurse.go, line 91 at r3 (raw file):

sg.DestUIDs = algo.MergeSorted(sg.uidMatrix)
shouldn't this be outside of for loop ?

---

query/recurse.go, line 113 at r3 (raw file):

				temp.SrcUIDs = sg.DestUIDs
				// If no UIDs are left after filtering, Ignore the node.
				if algo.ListLen(temp.SrcUIDs) == 0 {

temp.SrcUIDS is sg.SrcUIDs which remain same throughout the loop..
which we checked above as well.. so no need of this check.

---

query/recurse.go, line 140 at r3 (raw file):

			return
		}
		rch <- nil

usually, error channels are there to mark the done of execution..
here we are using it as done of one depth execution..
a comment here will be helpful.

---

query/recurse.go, line 151 at r3 (raw file):

	}
	expandErr := make(chan error, 2)
	next := make(chan bool, 2)

a comment about 2 will be helpful.
i think we need 2 because first next <- true and then ctx.Done() timesout, next <- false.
Is there any other reason as well ?
Does expandErr needs buffering of 2 ?

---

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

		if len(out) == 0 {
			rch <- ErrStop
		} else {

:)

---

Comments from Reviewable

[ashishnegi]

just some comments..

---

Review status: 3 of 4 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[ashwin95r]

Review status: 3 of 4 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

query/recurse.go, line 91 at r3 (raw file):

Previously, ashishnegi (Ashish Negi) wrote…

sg.DestUIDs = algo.MergeSorted(sg.uidMatrix)
shouldn't this be outside of for loop ?

Oops yes. Thanks for the catch.

---

query/recurse.go, line 113 at r3 (raw file):

Previously, ashishnegi (Ashish Negi) wrote…

temp.SrcUIDS is sg.SrcUIDs which remain same throughout the loop..
which we checked above as well.. so no need of this check.

Right. Since I moved the filtering step, it doesn't change now.

---

query/recurse.go, line 140 at r3 (raw file):

Previously, ashishnegi (Ashish Negi) wrote…

usually, error channels are there to mark the done of execution..
here we are using it as done of one depth execution..
a comment here will be helpful.

Done.

---

query/recurse.go, line 151 at r3 (raw file):

Previously, ashishnegi (Ashish Negi) wrote…

a comment about 2 will be helpful.
i think we need 2 because first next <- true and then ctx.Done() timesout, next <- false.
Is there any other reason as well ?
Does expandErr needs buffering of 2 ?

No, it actually doesn't but just for safety. Also, I fixed the case where it could have been 2 (In shortest path)

---

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

Previously, ashishnegi (Ashish Negi) wrote…

:)

Yeah Here!

---

Comments from Reviewable

[ashwin95r]

Merged.

[yinrong]

no doc?

[danielmai]

@yinrong Recurse query docs can be found here: https://docs.dgraph.io/query-language/#recurse-query