feat(Query): Support variables inside uid_in function #5320

all-seeing-code · 2020-04-28T13:41:40Z

Motivation

This PR adds support for queries which use variable inside uid_in function:

{
  getJeunet as q(func: eq(name@fr, "Jean-Pierre Jeunet")) 
  caro(func: eq(name@en, "Marc Caro")) {
    name@en
    director.film @filter(uid_in(~director.film, uid(getJeunet) )) {
      name@en
    }
  }
}

DGRAPH-549 -- Add support of Value Variables for uid_in
Github Issue Add support of Value Variables for uid_in. #3066

Components affected by this PR

Nested uid_in function calls.

Does this PR break backwards compatibility?

No.

Testing

Added following tests in query1_test.go:

Extended uid_in tests to work with variables
Added TestUidInWithParseErrors to check that program gracefully returns error in case of malformed input and does not crash

Fixes

Fixes DGRAPH-549
Fixes #3066

This change is

Docs Preview:

all-seeing-code

@MichelDiz No, currently it will only support one variable inside uid function. I have added tests (TestUidInWithErrors) specifically which checks the behavior which you asked for and throw an error. I have added a TODO to look into this later.

@jarifibrahim Added tests and description.

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

gql/parser.go, line 1728 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

needs test at the parser level as well.

Yes, have added more tests and edge case handling to prevent panics.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

I think you can remove this special logic and simplify. Simply create a new array to store the args and set sg.SrcFunc.Args to point to that array after the for loop. That way the first item doesn't have to be handled differently.

Done.

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

Previously, parasssh wrote…

add comment about the base 10.

Done.

parasssh

Can you estimate how much additional work is it for the request by @MichelDiz above? Also, run this by Pawan. Add / Move UT to the parser_test.go.

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

martinmr

It's fine to do what @MichelDiz suggested but it probably should be done in another PR.

I added some comments but they are all related to the tests. Once those are fixed it .

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @anurags92, @manishrjain, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

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

		},
		{
			description: "query with uid variable where school uid for <31> is <5001> and <25> is <5000>",

the description should be more general. Don't mention specific uids. It should say what general case is being tested.
Same for the other tests that contain specific numbers.

The descriptions of the tests in TestUidInWithErrors are good examples.

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

	}
}
func TestUidInWithErrors(t *testing.T) {

All these are parser tests so they should be moved there.

Also, the parser test should include tests to verify valid queries.

all-seeing-code

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

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

Previously, martinmr (Martin Martinez Rivera) wrote…

the description should be more general. Don't mention specific uids. It should say what general case is being tested.
Same for the other tests that contain specific numbers.

The descriptions of the tests in TestUidInWithErrors are good examples.

Done.

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

Previously, martinmr (Martin Martinez Rivera) wrote…

All these are parser tests so they should be moved there.

Also, the parser test should include tests to verify valid queries.

Moved the TestUidInWithErrors to parser_test.go
For the second point -- wouldn't the working queries be sufficient?

all-seeing-code

Ok.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

…work after this feature

pawanrawal

Reviewed 2 of 4 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @anurags92, @manishrjain, @martinmr, @parasssh, and @vvbalaji-dgraph)

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

			query: `{
				me(func: uid(1, 23, 24 )) {
					friend @filter(uid_in(school, uid(5000))) {

Its a bit weird that we don't support this case as it's the same as

friend @filter(uid_in(school, 5000))

We support passing a constant uid using the uid function at other places.
Should we support it?

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

			srcFuncArgs := sg.SrcFunc.Args[:0]
			if l.Uids != nil {
				for _, uid := range l.Uids.Uids {

Could you just range over using l.Uids.GetUids() without checking l.Uids != nil as it takes care of nil

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

					arg := gql.Arg{
						Value:        strconv.FormatUint(uid, 10),
						IsValueVar:   false,

Don't need to pass in false values, they are the default.

query/query1_test.go, line 1061 at r3 (raw file):

			description: "query at top level with nested UID variable",
			query: `{
				uidVar as q(func: uid(5001, 5000))

This can be a var query so that its not returned in the results.

query/query1_test.go, line 1142 at r3 (raw file):

		},
		{
			description: "query inside root with nested uid variable uid variable which resolves to two uids",

extra uid variable can be removed

query/query1_test.go, line 1192 at r3 (raw file):

		}
	}`
	expectedErr := errors.New("rpc error: code = Unknown desc = : Function 'uid_in' requires atleast 1 argument, but got 0 ([])")

hmm, this shouldn't be a runtime error, if the uid variable returns a null set we should just consider the intersection to be []. It would be weird if for the user some of the queries failed and some passed depending on which node they were starting with.

all-seeing-code

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

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

Previously, pawanrawal (Pawan Rawal) wrote…

Its a bit weird that we don't support this case as it's the same as
friend @filter(uid_in(school, 5000))
We support passing a constant uid using the uid function at other places.
Should we support it?

Will create a JIRA to extend support.

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

Previously, pawanrawal (Pawan Rawal) wrote…

Could you just range over using l.Uids.GetUids() without checking l.Uids != nil as it takes care of nil

Okay, thanks. Didnt know that.

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

Previously, pawanrawal (Pawan Rawal) wrote…

Don't need to pass in false values, they are the default.

Cool, makes sense.

query/query1_test.go, line 1061 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This can be a var query so that its not returned in the results.

Yes, thanks for pointing this out.

query/query1_test.go, line 1142 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

extra uid variable can be removed

Ok.

query/query1_test.go, line 1192 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

hmm, this shouldn't be a runtime error, if the uid variable returns a null set we should just consider the intersection to be []. It would be weird if for the user some of the queries failed and some passed depending on which node they were starting with.

Agree, need to be changed.

all-seeing-code

Reviewable status: 0 of 5 files reviewed, 11 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

gql/parser.go, line 1745 at r4 (raw file):

					function.Args = append(function.Args, Arg{Value: nestedFunc.NeedsVar[0].Name})
				default:
					return nil, itemInFunc.Errorf("Only val/count/len/uid allowed as function "+

Created a JIRA ticket since this change might allow nested uid inside other functions too. https://dgraph.atlassian.net/browse/DGRAPH-1918

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

friend @filter(uid_in(school, 5000))
This JIRA ticket should be able to handle the above requirement. https://dgraph.atlassian.net/browse/DGRAPH-1919

gql/parser_test.go, line 4349 at r4 (raw file):

}

func TestEqUidFunctionErr(t *testing.T) {

This JIRA ticket should investigate this: https://dgraph.atlassian.net/browse/DGRAPH-1918

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

Previously, anurags92 (Anurag) wrote…

Okay, thanks. Didnt know that.

Done.Done

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

Previously, anurags92 (Anurag) wrote…

Cool, makes sense.

Done.

query/query1_test.go, line 1061 at r3 (raw file):

Previously, anurags92 (Anurag) wrote…

Yes, thanks for pointing this out.

Done.

query/query1_test.go, line 1192 at r3 (raw file):

Previously, anurags92 (Anurag) wrote…

Agree, need to be changed.

Done.

pawanrawal

Reviewed 1 of 4 files at r4, 4 of 4 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

all-seeing-code

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @jarifibrahim, @manishrjain, @martinmr, @parasssh, @pawanrawal, and @vvbalaji-dgraph)

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

Previously, anurags92 (Anurag) wrote…

friend @filter(uid_in(school, 5000))
This JIRA ticket should be able to handle the above requirement. https://dgraph.atlassian.net/browse/DGRAPH-1919

Done.

gql/parser_test.go, line 4349 at r4 (raw file):

Previously, anurags92 (Anurag) wrote…

This JIRA ticket should investigate this: https://dgraph.atlassian.net/browse/DGRAPH-1918

Done.

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

Previously, anurags92 (Anurag) wrote…

Moved the TestUidInWithErrors to parser_test.go
For the second point -- wouldn't the working queries be sufficient?

Done.

…nc#5320)

all-seeing-code added 8 commits April 24, 2020 19:34

Support multiple uids in UID_IN

f9f9bcb

Code clean-up

a1800a1

Code clean-up again

4c98de9

Code clean-up again and again

f75c486

Add support for parsing inside square brackets

b57f05a

Remove print statements

ebd4598

Remove extra line.

bcc15ea

Testing uid nested inside functions

495638d

all-seeing-code requested a review from manishrjain as a code owner April 28, 2020 13:41

all-seeing-code requested a review from a team April 28, 2020 13:41

all-seeing-code changed the title ~~Anurags92/nested uid fn~~ Add nested uid fn Apr 28, 2020

all-seeing-code added 8 commits April 28, 2020 19:17

Add sorting to uids list

0b5480c

Added tests for uid_in function

aae6da1

Add tests in gql parser for uid_in extention to accept multiple uids

8ddb8c8

Add parser tests

4bb7941

remove repeat test cases

705a3b3

Rename parseIneqArgs

b4502cb

Merge branch 'anurags92/ValVarinUID_IN' into anurags92/NestedUIDFn

51abb26

Add helpful print statements

335bb36

all-seeing-code requested a review from pawanrawal as a code owner April 29, 2020 06:19

all-seeing-code linked an issue Apr 30, 2020 that may be closed by this pull request

Add support of Value Variables for uid_in. #3066

Closed

all-seeing-code added 3 commits May 18, 2020 12:40

Merge master

87c4222

Merge branch 'master' into anurags92/NestedUIDFn

db9fdf3

Added nested uid functionality (dirty, needs clean-up)

e95725f

all-seeing-code requested a review from vvbalaji-dgraph as a code owner June 2, 2020 15:59

all-seeing-code added 5 commits June 2, 2020 22:10

Remove unnecessary snippets

12a3f62

More clean-up

68b7b50

Remove unused prints

192a95d

Merge branch 'master' into anurags92/NestedUIDFn

a4711e7

Add special case for handling uid_in in query processing

7828950

Add error handling

aa21da9

all-seeing-code commented Jun 30, 2020

View reviewed changes

Minor corrections in test suite

c1f6e74

parasssh reviewed Jun 30, 2020

View reviewed changes

martinmr approved these changes Jun 30, 2020

View reviewed changes

all-seeing-code commented Jul 1, 2020

View reviewed changes

all-seeing-code added 2 commits July 1, 2020 13:41

Added more tests for uid_in and removed an old test which should not …

33804ee

…work after this feature

Added mroe tests to parser

64cfc74

pawanrawal suggested changes Jul 2, 2020

View reviewed changes

all-seeing-code commented Jul 2, 2020

View reviewed changes

all-seeing-code added 4 commits July 3, 2020 00:52

Added comments

23f96be

Merge branch 'master' into anurags92/NestedUIDFn

137c2f0

Addressed few comments on review

8ecc85c

Minor edit

0d128f8

all-seeing-code requested review from parasssh and jarifibrahim July 6, 2020 07:28

all-seeing-code added 5 commits July 8, 2020 16:30

Merge branch 'master' into anurags92/NestedUIDFn

94c7aef

Merge branch 'master' into anurags92/NestedUIDFn

12037d4

Change error message

3864f0f

Change error handling for zero arguments

7b1f69e

Changed test case and removed a comment

13f156b

all-seeing-code commented Jul 14, 2020

View reviewed changes

pawanrawal approved these changes Jul 14, 2020

View reviewed changes

Add more test cases

623d732

all-seeing-code commented Jul 14, 2020

View reviewed changes

all-seeing-code merged commit e9c3f05 into master Jul 14, 2020

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

feat(Query): Support uid variables inside uid_in function (hypermodei…

44736b1

…nc#5320)

all-seeing-code mentioned this pull request Jul 20, 2020

Add support of Value Variables for uid_in. #3066

Closed

all-seeing-code deleted the anurags92/NestedUIDFn branch July 27, 2020 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(Query): Support variables inside uid_in function #5320

feat(Query): Support variables inside uid_in function #5320

Uh oh!

all-seeing-code commented Apr 28, 2020 •

edited

Loading

Uh oh!

all-seeing-code left a comment

Uh oh!

parasssh left a comment

Uh oh!

martinmr left a comment

Uh oh!

all-seeing-code left a comment

Uh oh!

all-seeing-code left a comment

Uh oh!

pawanrawal left a comment

Uh oh!

all-seeing-code left a comment

Uh oh!

all-seeing-code left a comment

Uh oh!

pawanrawal left a comment

Uh oh!

all-seeing-code left a comment

Uh oh!

Uh oh!

feat(Query): Support variables inside uid_in function #5320

feat(Query): Support variables inside uid_in function #5320

Uh oh!

Conversation

all-seeing-code commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Components affected by this PR

Does this PR break backwards compatibility?

Testing

Fixes

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

parasssh left a comment

Choose a reason for hiding this comment

Uh oh!

martinmr left a comment

Choose a reason for hiding this comment

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

pawanrawal left a comment

Choose a reason for hiding this comment

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

pawanrawal left a comment

Choose a reason for hiding this comment

Uh oh!

all-seeing-code left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

all-seeing-code commented Apr 28, 2020 •

edited

Loading