Skip to content

fix(worker): fix between filter for non indexed predicates #6715

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

Merged
merged 9 commits into from
Oct 14, 2020

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Oct 13, 2020

Fixes DGRAPH-2528.
Fixes DGRAPH-2544.

This PR extends the support for between filter on non-indexed predicates so that it could be used outside the root also. For example, for the given schema

type CarModel {
	make
	model
	year
}
model                          : string @index(term) @lang .
make                           : string @index(term) .
year                           : int .

The below query uses between filter on year predicate which is non-indexed, should be valid.

queryCareModel(func: type(CarModel)) @filter(between(year,2009,2010)){
				make
				model
				year
			}

This PR also fixes the eq filter for more than one argument on the non-indexed predicate which earlier used to compare with just the first of the given arguments for eq filter. For eg the below query was not giving proper results earlier.

queryCarModel(func: type(CarModel)) @filter(eq(year,2008,2009)){
				make
				model
				year
			}

This change is Reviewable

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 14 rules errored during the review.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 12 rules errored during the review.

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.

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @minhaj-shakeel, @pawanrawal, and @vvbalaji-dgraph)


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

		`
		{
			me(func: type(CarModel)) @filter(between(year,2009,2010)){

Also, add a test where between is used in a filter on a child node.

{

  me(users) {
    car @filter(between(....)) {
      make
       model
    }
  }

}

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

						}
					case "between":
						if types.CompareVals("ge", val, srcFn.eqTokens[0]) &&

Can we add a function for this check and use it both here and at the root?


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

							types.CompareVals("le", val, srcFn.eqTokens[1]) {
							uidList.Uids = append(uidList.Uids, q.UidList.Uids[i])
							break

not needed


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

						if types.CompareVals(srcFn.fname, val, srcFn.eqTokens[0]) {
							uidList.Uids = append(uidList.Uids, q.UidList.Uids[i])
							break

not needed

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel 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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


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

Previously, pawanrawal (Pawan Rawal) wrote…

Also, add a test where between is used in a filter on a child node.

{

  me(users) {
    car @filter(between(....)) {
      make
       model
    }
  }

}

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can we add a function for this check and use it both here and at the root?

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

not needed

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

not needed

Done.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 15 rules errored during the review.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 11 rules errored during the review.

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.

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


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

			`Test Between on Indexed Predicate`,
			`{
				me(func :has(newname))  @filter(eq(newname,"P1","P3")){

these queries are not using between, did you intend to use between instead of eq.

Copy link
Contributor Author

@minhaj-shakeel minhaj-shakeel 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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


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

Previously, pawanrawal (Pawan Rawal) wrote…

these queries are not using between, did you intend to use between instead of eq.

Sorry for the mistake, I have corrected it.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

LGTM. 5 rules errored during the review.

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.

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

@minhaj-shakeel minhaj-shakeel merged commit 3d6abdf into master Oct 14, 2020
@minhaj-shakeel minhaj-shakeel deleted the minhaj/dgraph-between branch November 23, 2020 04:38
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.

2 participants