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(Query): fix cascade with pagination #7440

Merged
merged 18 commits into from
Feb 19, 2021

Conversation

minhaj-shakeel
Copy link
Contributor

@minhaj-shakeel minhaj-shakeel commented Feb 17, 2021

Fixes DGRAPH-1901.
This PR fixes the behavior of the @cascade directive with pagination arguments.


This change is Reviewable

@github-actions github-actions bot added area/graphql Issues related to GraphQL support on Dgraph. area/querylang Issues related to the query language specification and implementation. labels Feb 17, 2021
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, 5 unresolved discussions (waiting on @manishrjain, @minhaj-shakeel, and @vvbalaji-dgraph)


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

		}

		// Remove pagination arguments from the query if @cascade is mentioned since

Also check what happens if a sort is applied and no first is applied, do we apply a first of 1000 automatically? Where does that happen?


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

	// Remove pagination arguments from the query if @cascade is mentioned since
	// pagination will be applied post processing the data.
	if len(args.Cascade.Fields) > 0 {

I see this reused, can we make a function for this?


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

			if parent == nil {
				// I'm root. We reach here if root had a function.
				newDestUidList := &pb.List{Uids: make([]uint64, 0, len(sg.DestUIDs.Uids))}

Add a comment about why this copy is being done. Say that DestUids for this level become the SrcUids for the next level. In updateUidMatrix with cascade we end up modifying the first list from the uidMatrix which ends up modifying the srcUids of the next level. So to avoid that we make a copy.

In fact, we only need to make a copy if Cascade is true for this level so add that optimization as well and do a copy only when Cascade has fields.


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

			// first time at the root here.

			//Apply pagination at the root after @cascade.

// Apply


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

	query := `
	{
		me(func: uid(0x01)) @cascade {

needs a bit more testing with three-level nodes and different scenarios.

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 2 files reviewed, 5 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)


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

Previously, pawanrawal (Pawan Rawal) wrote…

Also check what happens if a sort is applied and no first is applied, do we apply a first of 1000 automatically? Where does that happen?

Done. It still works fine.


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

Previously, pawanrawal (Pawan Rawal) wrote…

I see this reused, can we make a function for this?

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Add a comment about why this copy is being done. Say that DestUids for this level become the SrcUids for the next level. In updateUidMatrix with cascade we end up modifying the first list from the uidMatrix which ends up modifying the srcUids of the next level. So to avoid that we make a copy.

In fact, we only need to make a copy if Cascade is true for this level so add that optimization as well and do a copy only when Cascade has fields.

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

// Apply

Done.


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

Previously, pawanrawal (Pawan Rawal) wrote…

needs a bit more testing with three-level nodes and different scenarios.

Done.

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, all discussions resolved (waiting on @manishrjain and @vvbalaji-dgraph)

@minhaj-shakeel minhaj-shakeel merged commit f755cc0 into master Feb 19, 2021
@minhaj-shakeel minhaj-shakeel deleted the minhaj/cascade-with-pagination branch April 22, 2021 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph. area/querylang Issues related to the query language specification and implementation.
Development

Successfully merging this pull request may close these issues.

2 participants