Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for multiple uids in uid_in function #5292

Merged
merged 20 commits into from
Apr 29, 2020

Conversation

all-seeing-code
Copy link
Contributor

@all-seeing-code all-seeing-code commented Apr 24, 2020

Currently uid_in functions checks membership of only one uid across a given predicate edge. This PR will extend the support of membership checks to multiple uids in a single query fragment. Particularly we can run queries like this which checks for the presense of two uids (0x4e472a and 0x4e472b):

q(func: eq(name, "Jane Doe") ){
                name
                friend @filter( uid_in(memberOf, [0x4e472a, 0x4e472b]) ) {
                    name
                }
 }

Fixes DGRAPH-1292


This change is Reviewable

@all-seeing-code all-seeing-code self-assigned this Apr 24, 2020
@all-seeing-code all-seeing-code linked an issue Apr 24, 2020 that may be closed by this pull request
Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some tests similar to others that we have in the query package to make sure this works as expected.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anurags92, @manishrjain, and @MichelDiz)


worker/task.go, line 1831 at r1 (raw file):

				return nil, err
			}
			fc.uidsPresent = append(fc.uidsPresent, uidParsed)

This list probably needs to be sorted before interesting. See if your tests pass with the code as it is.

Copy link
Contributor

@MichelDiz MichelDiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anurags92 and @manishrjain)

Copy link
Contributor Author

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added testing in parser and query packages

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)


worker/task.go, line 1831 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This list probably needs to be sorted before interesting. See if your tests pass with the code as it is.

Done.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good, just needs some small changes.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @anurags92 and @manishrjain)


gql/parser_test.go, line 4903 at r2 (raw file):

			}
		}
		require.True(t, found, "vars not matched: %v", tc.vars)

Shouldn't this found be checked for each variable? That is it should be reset for each variable and checked in the loop above?


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

	query := `
	{
		me(func: UID(1, 23, 24)) @filter(uid_in(school, 5000, 5001)) {

Lets add a couple of tests (one at root and one as a child) where the input to uid_in is not already sorted.


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

	}`
	js := processQueryNoErr(t, query)
	require.JSONEq(t, `{"data": {"me":[{"name":"Michonne"},{"name":"Rick Grimes"},{"name":"Glenn Rhee"}]}}`, js)

Can you also add a starting node for which the uid_in filter fails? Right now it seems to pass for all 1, 23 and 24.


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

	{
		me(func: uid(1, 23, 24 )) {
			friend @filter(uid_in(school, [5000, 5001])) {

We don't need to have separate cases for [5000, 5001] and 5000, 5001 since they are just different syntax for the same query. If anything, we can add those tests in gql/parser.go to verify they have the same args.

Copy link
Contributor Author

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @MichelDiz, and @pawanrawal)


gql/parser_test.go, line 4903 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Shouldn't this found be checked for each variable? That is it should be reset for each variable and checked in the loop above?

Yes, nice catch. I have updated that in another function from where I copied this.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Lets add a couple of tests (one at root and one as a child) where the input to uid_in is not already sorted.

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can you also add a starting node for which the uid_in filter fails? Right now it seems to pass for all 1, 23 and 24.

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

We don't need to have separate cases for [5000, 5001] and 5000, 5001 since they are just different syntax for the same query. If anything, we can add those tests in gql/parser.go to verify they have the same args.

Done.

Copy link
Contributor

@pawanrawal pawanrawal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @anurags92 and @manishrjain)


gql/parser_test.go, line 4905 at r3 (raw file):

			}
		}
		require.True(t, found, "vars not matched: %v", tc.vars)

This require.True should be in the loop above which goes over tc.vars.

Copy link
Contributor Author

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @manishrjain and @pawanrawal)


gql/parser_test.go, line 4905 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This require.True should be in the loop above which goes over tc.vars.

Done.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Got a couple of comments. Good to go!

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


query/query1_test.go, line 1049 at r4 (raw file):

	}`
	js := processQueryNoErr(t, query)
	require.JSONEq(t, `{"data": {"me":[{"name":"Michonne"},{"name":"Rick Grimes"},{"name":"Glenn Rhee"}]}}`, js)

I'd check if other lines in this file are violating the 100 char limit.


worker/task.go, line 1835 at r4 (raw file):

			fc.uidsPresent = append(fc.uidsPresent, uidParsed)
		}
		sort.Slice(fc.uidsPresent, func(i, j int) bool { return fc.uidsPresent[i] < fc.uidsPresent[j] })

100 chars.

Copy link
Contributor Author

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 4 files reviewed, 3 unresolved discussions (waiting on @anurags92, @manishrjain, and @pawanrawal)


query/query1_test.go, line 1049 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd check if other lines in this file are violating the 100 char limit.

Yes, a lot of tests are violating the 100 char limit mainly because the expected JSON strings are kept in one single line.

@all-seeing-code all-seeing-code merged commit 3c12533 into master Apr 29, 2020
@all-seeing-code all-seeing-code deleted the anurags92/ValVarinUID_IN branch May 18, 2020 06:52
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants