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

Add ability to expand only on certain types. #3920

Merged
merged 6 commits into from
Oct 17, 2019
Merged

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Sep 4, 2019

Instead of doing expand(all), this change introduces expand(Type1, Type2) syntax to
expand only on certain types.


This change is Reviewable

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ 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.

@martinmr
Copy link
Contributor Author

martinmr commented Sep 4, 2019

There are some issues with the query code that came up while making this change.

  • Language tags are only expanded when using expand(_all_). I am not sure if this is the desired behavior.
  • The following test is failing because the uid of the node is shown. This does not happen when using expand(_all_). Removing the uid part from the query makes the test pass but I am not sure why there's a difference between doing expand(_all_) and expand(<list of all the types>)
func TestTypeExpandMultipleExplicitTypes(t *testing.T) {
	query := `{
		q(func: eq(make, "Toyota")) {
			expand(CarModel, Object) {
                           uid
                         }
		}
	}`
	js := processQueryNoErr(t, query)
	require.JSONEq(t, `{"data": {"q":[
		{"name": "Car", "make":"Toyota","model":"Prius", "year":2009}]}}`, js)
}

@pawanrawal could you take a look at the second item. I am not sure why the uid of the node is being returned along all the other predicates.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

The structure looks sound but have some questions pertaining to the use of pointer semantics in the parseTypeList function.

It appears that the lex.ItemIterator is never mutated. Is the reason you are using pointers here due to the fact that the cost of copying is high or just a code pattern in this project?


Reviewed with ❤️ by PullRequest

@pawanrawal
Copy link
Contributor

I had a look at it but couldn't find immediately why. I suppose what could be happening here is that the uid is being somehow added at the same level as other predicates instead of a child. It would require time for me to debug and I am busy with other things right now. If you can't figure it out, file it as a bug and then I or someone else could have a proper look later.

@martinmr
Copy link
Contributor Author

martinmr commented Sep 6, 2019

No worries. I was hoping it was something obvious I was missing. I'll take a closer look soon.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

I found the root cause of the weird issue I was seeing (the parser code was incorrectly swallowing a { character. PR should be ready to review now.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion


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

			}
			sg := req.Subgraphs[idx]

remove this line

@martinmr martinmr marked this pull request as ready for review October 4, 2019 00:38
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:

Would be get to merge the other PRs to master and merge master into this branch first.

Reviewed 2 of 7 files at r1, 4 of 5 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)


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

Previously, martinmr (Martin Martinez Rivera) wrote…

remove this line

Why?

Copy link
Contributor Author

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

I will merge the other PRs first since this is a new feature.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)


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

Previously, pawanrawal (Pawan Rawal) wrote…

Why?

Extra line I added. I just wrote a comment so I wouldn't forget.

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: One comment about the test case.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @martinmr)


query/query4_test.go, line 345 at r3 (raw file):

		q(func: eq(make, "Toyota")) {
			expand(Object) {
				uid

Remove this.


query/query4_test.go, line 357 at r3 (raw file):

		q(func: eq(make, "Toyota")) {
			expand(CarModel, Object) {
				uid

At least one example with expand(Type) which has a child node whose uid can be retrieved.

Copy link
Contributor Author

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


query/query4_test.go, line 345 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Remove this.

Done.


query/query4_test.go, line 357 at r3 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

At least one example with expand(Type) which has a child node whose uid can be retrieved.

Done.

@martinmr martinmr merged commit 05024f6 into master Oct 17, 2019
@martinmr martinmr deleted the martinmr/expand-types branch October 17, 2019 20:40
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.

3 participants