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

Fix retrieval of facets with cascade #4530

Merged
merged 5 commits into from
Jan 13, 2020
Merged

Conversation

ashish-goswami
Copy link
Contributor

@ashish-goswami ashish-goswami commented Jan 9, 2020

Fixes #4310
Currently we are not able to fetch fecets associated with scalar predicate when using
cascade directive.
While processing cascade directive, we update uidMatrix. Every uid in uidMatrix is checked if
it is present in destUIDs, if not we remove it from uidMatrix. We also remove entry from
facetsMatrix for same uid. This works fine when predicate is of uid/[uid] type.
When predicate is of scalar type, destUIDs and uidMatrix will be empty, hence we should not
update facetsMatrix as facets here are corresponding to valueMatrix.
This PR avoid updating facetsMatrix when a uid list in uidMatrix is empty in updateUidMatrix.


This change is Reviewable

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 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ashish-goswami)


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

}

func TestFacetsCascadeScalarPredicate(t *testing.T) {

Add another case where cascade removes one of the results. Facets should be retrieved for the other uid.


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

	js := processQueryNoErr(t, query)
	fmt.Println(js)

remove


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

	populateClusterWithFacets()
	query := `{
		q(func: uid(1, 23)) @cascade {

Can remove top level cascade or add another case where cascade is only applied on child.

Copy link
Contributor Author

@ashish-goswami ashish-goswami 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, 3 unresolved discussions (waiting on @pawanrawal)


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

Previously, pawanrawal (Pawan Rawal) wrote…

Add another case where cascade removes one of the results. Facets should be retrieved for the other uid.

updated TestFacetsCascadeUIDPredicate to have uid 24 also. This uid has name, but doesn't have friend, hence it should not be retrieved.


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

Previously, pawanrawal (Pawan Rawal) wrote…

remove

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can remove top level cascade or add another case where cascade is only applied on child.

Removed top level cascade.

@ashish-goswami ashish-goswami changed the title [WIP] Fix retrieval of facets with cascade Fix retrieval of facets with cascade Jan 13, 2020
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 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ashish-goswami ashish-goswami marked this pull request as ready for review January 13, 2020 17:00
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:

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@ashish-goswami ashish-goswami merged commit f38fc0c into master Jan 13, 2020
@ashish-goswami ashish-goswami deleted the ashish/cascade-facets branch January 13, 2020 18: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.

@cascade disables value edge facet fetching in PR #4267
3 participants