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

Support filtering on nonindexed predicate #4531

Merged
merged 7 commits into from Jan 14, 2020

Conversation

@animesh2049
Copy link
Member

animesh2049 commented Jan 9, 2020

Fixes #4305

This change is Reviewable

worker/task.go Outdated Show resolved Hide resolved
worker/task.go Outdated Show resolved Hide resolved
worker/task.go Outdated Show resolved Hide resolved
worker/task.go Outdated Show resolved Hide resolved
worker/task.go Outdated Show resolved Hide resolved
animesh2049 added 2 commits Jan 10, 2020
Copy link
Member Author

animesh2049 left a comment

Dismissed @golangcibot from 5 discussions.
Reviewable status: 0 of 4 files reviewed, all discussions resolved

Copy link
Contributor

mangalaman93 left a comment

clean PR. Check with @manishrjain whether this could have performance impact.

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

pawanrawal left a comment

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @animesh2049)


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

owner                          : [uid] .
noconflict_pred                : string @noconflict .
noindex_age                    : int .

Move noindex_name here as well.


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

			{
				me(func: uid(0x01)) {
					friend @filter(ge(survival_rate, 30)) {

Can we have a value which returns a result?


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

}

func TestFilterNonIndexedPredicateFail(t *testing.T) {

Can we change this test to execute le function at root and then that should still return an error? If we already have a test, then we don't need it.


worker/task.go, line 858 at r2 (raw file):

	if needsIndex(srcFn.fnType) && !schema.State().IsIndexed(q.Attr) {
		if q.UidList != nil && srcFn.fnType == compareAttrFn {
			// UidList is not nil means this is a filter. Filter predicate is not indexed, so

needsIndex can be modified to return false for compareAttrFn and q.UidList != nil


worker/task.go, line 1610 at r2 (raw file):

			}
			fc.eqTokens = append(fc.eqTokens, fc.ineqValue)
			if !schema.State().IsIndexed(attr) {

Can be refactored a bit to make clear the distinction between the logic for eq which can have multiple arguments vs other functions.


worker/task.go, line 1632 at r2 (raw file):

		// compare the value with all of them. Also eq would usually have less arguments, hence we
		// won't be fetching many index keys.
		if q.UidList != nil && fc.tokens == nil {

This should be !indexed so as to avoid the extra computation when it is it is indexed but doesn't have any tokens.

Copy link
Member Author

animesh2049 left a comment

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


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

Previously, pawanrawal (Pawan Rawal) wrote…

Move noindex_name here as well.

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can we have a value which returns a result?

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can we change this test to execute le function at root and then that should still return an error? If we already have a test, then we don't need it.

Done.


worker/task.go, line 1610 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Can be refactored a bit to make clear the distinction between the logic for eq which can have multiple arguments vs other functions.

This requires larger change. I have added a todo. Probably in some other PR.


worker/task.go, line 1632 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This should be !indexed so as to avoid the extra computation when it is it is indexed but doesn't have any tokens.

Done.

@animesh2049 animesh2049 marked this pull request as ready for review Jan 13, 2020
@animesh2049 animesh2049 requested review from manishrjain and dgraph-io/team as code owners Jan 13, 2020
Copy link
Member

manishrjain left a comment

Looks generally alright -- I don't see anything obvious in terms of perf impact, but good to double check that existing indices are still being used for queries -- do that manual testing. Defer to @pawanrawal for final review and approval.

Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @animesh2049 and @pawanrawal)


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

			}
			`,
			`{"data":{"me":[{"noindex_salary":501.230000},{"noindex_salary":589.040000},{"noindex_salary":459.470000}]}}`,

Look in the file. If the others are also overflowing 100 chars, then OK. Otherwise, bring it to 100 chars.


worker/task.go, line 858 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

needsIndex can be modified to return false for compareAttrFn and q.UidList != nil

Yeah, needsIndex is probably the right place.


worker/task.go, line 1610 at r2 (raw file):

Previously, animesh2049 (Animesh Chandra Pathak) wrote…

This requires larger change. I have added a todo. Probably in some other PR.

Please add a TODO


worker/task.go, line 923 at r3 (raw file):

	if srcFn.fnType == compareScalarFn && srcFn.isFuncAtRoot {
		span.Annotate(nil, "handleCompareScalarFunction")
		if err := qs.handleCompareScalarFunction(args); err != nil {

I'll get @pawanrawal to opine on this. Is this going to cause some race issues, perhaps?

Copy link
Member

pawanrawal left a comment

:lgtm: please address remaining comments and then this should be good to merge.

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


worker/task.go, line 923 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'll get @pawanrawal to opine on this. Is this going to cause some race issues, perhaps?

Should be ok as all the functions are being run sequentially in helpProcessTask and q, srcFn, out all are pointers.

@animesh2049 animesh2049 changed the title Animesh2049/filter on nonindexed predicate Support filtering on nonindexed predicate Jan 14, 2020
Copy link
Member Author

animesh2049 left a comment

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


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

Previously, manishrjain (Manish R Jain) wrote…

Look in the file. If the others are also overflowing 100 chars, then OK. Otherwise, bring it to 100 chars.

Done.

Copy link
Member Author

animesh2049 left a comment

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


worker/task.go, line 923 at r3 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should be ok as all the functions are being run sequentially in helpProcessTask and q, srcFn, out all are pointers.

Done.

@animesh2049 animesh2049 merged commit 916518f into master Jan 14, 2020
6 of 8 checks passed
6 of 8 checks passed
DeepSource: Go Analysis Failed: Issues detected that need attention.
Details
code-review/reviewable 3 files, 2 discussions left (manishrjain, pawanrawal)
Details
Blockade (dgraph) TeamCity build finished
Details
CI (dgraph) TeamCity build finished
Details
Docs-Preview (dgraph) TeamCity build finished
Details
Docs-Preview/dgraph-c47265f4c1-39381 Deployment ready!
Details
GolangCI No issues found!
Details
license/cla Contributor License Agreement is signed.
Details
@animesh2049 animesh2049 deleted the animesh2049/filter-on-nonindexed-predicate branch Jan 14, 2020
animesh2049 added a commit that referenced this pull request Jan 16, 2020
animesh2049 added a commit that referenced this pull request Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.