Support filtering by facets on values.

[martinmr]

Previously, only uid nodes could use facet filtering. This PR allows filtering to also happen in value nodes. The syntax is the same.

Fixes #4034

---

This change is 

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

@martinmr you can click here to see the review status or cancel the code review job.

[pullrequest]

Added some minor comments on readability, and one comment on error handling. No major blockers though.

---

Reviewed with ❤️ by PullRequest

[manishrjain]

Got a few comments. Address carefully. Test thoroughly.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @campoy, @martinmr, @pawanrawal, and @pullrequest[bot])

---

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

func retrieveValuesAndFacets(args funcArgs, pl *posting.List) ([]types.Val, []*api.Facet, error) {
	q := args.q
	listType := schema.State().IsList(q.Attr)

Calculate once and use here. Perhaps, funcArgs could have this info??

---

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

	if q.FacetsFilter == nil {
		// Retrieve values.
		if q.ExpandAll {

Does this mean that expand and list retrieval won't work if we do have a filter??

Add some tests to ensure that we can still retrieve all the values and langs, etc. even if there's a filter.

Test might be to:

1. Have a facet, which is true for all values (dummy=true)
2. Filter on that facet, which basically is a passthrough (eq(dummy, true)).
3. Ensure that you can still retrieve all other facets (ask for key1 and key2).

---

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

Previously, pullrequest[bot] wrote…

why do we not have to handle the error case, when we did before?
Note that empty slice isn't the same as a nil slice.

if err != nil {
  fs = []*api.Facet{}
}

Please handle error.

---

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

Previously, pullrequest[bot] wrote…

nit: should be picked instead of pick. It makes more sense, since it describes if it was picked or not.

^^ yes.

[martinmr]

Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @campoy, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

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

Previously, pullrequest[bot] wrote…

^ there's a typo at line 345: his function has small boiletplate... should be boilerplate

Done.

---

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

Previously, pullrequest[bot] wrote…

Let's have facets instead of fcs so its easier to understand

Can't do. facets is the name of a package.

---

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

Previously, manishrjain (Manish R Jain) wrote…

Calculate once and use here. Perhaps, funcArgs could have this info??

funcArgs is not a good place because it holds the entire query, not just information about a particular subgraph so it could confuse people further down the line. I added the listType as an argument.

---

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

Previously, pullrequest[bot] wrote…

nit: facets instead of fcs

Can't do.

---

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

Previously, pullrequest[bot] wrote…

This comment makes it seem like there are still no facet filtering on values.
I think the code speaks for its self here. Let's remove this line.

Rephrased comment.

---

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

Previously, manishrjain (Manish R Jain) wrote…

Does this mean that expand and list retrieval won't work if we do have a filter??

Add some tests to ensure that we can still retrieve all the values and langs, etc. even if there's a filter.

Test might be to:

1. Have a facet, which is true for all values (dummy=true)
2. Filter on that facet, which basically is a passthrough (eq(dummy, true)).
3. Ensure that you can still retrieve all other facets (ask for key1 and key2).

I knew I had to implement this logic but I missed it somehow. I have added the equivalent logic to the case where there's no filtering on facets to properly handle language tags and added additional tests.

I also added a test like the one you suggested.

---

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

Previously, manishrjain (Manish R Jain) wrote…

Please handle error.

Done.

[pawanrawal]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @campoy, @manishrjain, @martinmr, and @pullrequest[bot])

---

query/query_facets_test.go, line 881 at r2 (raw file):

	{
		me(func: has(name)) {
			name@en @facets(eq(origin, "french"))

1. Can you please add some tests for AND, OR which use different facets as well?
   Like

@facets(eq(origin, "french") AND ge(age, 10))

2. Could you also add the same test but with name@hi to ensure that it doesn't return any result?

---

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

	}

	// Retrieve the posting that matches the language preferences.

Do the facet filters work for list type and expand all? Why don't we just get the postings upfront (for all the 3 cases) and decide if we want to add the value based on the facet filtering?

I don't see tests for

1. List type + facet filtering
2. Expand all + facet filtering

[martinmr]

Reviewable status: 2 of 4 files reviewed, 8 unresolved discussions (waiting on @campoy, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

query/query_facets_test.go, line 881 at r2 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

1. Can you please add some tests for AND, OR which use different facets as well?
   Like

@facets(eq(origin, "french") AND ge(age, 10))

2. Could you also add the same test but with name@hi to ensure that it doesn't return any result?

Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

Do the facet filters work for list type and expand all? Why don't we just get the postings upfront (for all the 3 cases) and decide if we want to add the value based on the facet filtering?

I don't see tests for

1. List type + facet filtering
2. Expand all + facet filtering

As discussed in person, it's hard to retrieve the values first because once you do, it's hard to associate the values with their posting.

Expand all does not work because directives are not supported with expand all. We don't have to support this use case.

I added a new test for the list type.

[pawanrawal]

Just add a couple of more test cases mentioned in the comments below and then this should be good to go.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @campoy, @manishrjain, @martinmr, and @pullrequest[bot])

---

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

	{
		me(func: has(name)) {
			alt_name @facets(eq(origin, "french"))

To make sure that filtering is working properly, could we add the alt_name to a couple of other uids as well. Right now its only there for uid 1.

---

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

	{
		me(func: has(name)) {
			name @facets(eq(origin, "french") AND eq(dummy, false))

Could you please add another case for a AND filter which returns actual results.

[pullrequest]

✖️ This code review was cancelled. See Details

[martinmr]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @campoy, @manishrjain, @pawanrawal, and @pullrequest[bot])

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

To make sure that filtering is working properly, could we add the alt_name to a couple of other uids as well. Right now its only there for uid 1.

Done.

---

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

Previously, pawanrawal (Pawan Rawal) wrote…

Could you please add another case for a AND filter which returns actual results.

Done.