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

Simplify type definitions. #4017

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Simplify type definitions. #4017

merged 5 commits into from
Oct 1, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Sep 18, 2019

Type definitons are now simpler as the type of the fields is not
required anymore. The new format is:

type Person {
  name
  address
  ssn
}

This change is Reviewable

Type definitons are now simpler as the type of the fields is not
required anymore. The new format is:

```
type Person {
  name
  address
  ssn
}
```
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 martinmr marked this pull request as ready for review September 18, 2019 00:56
@martinmr martinmr requested review from manishrjain, pawanrawal and a team as code owners September 18, 2019 00:56
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.

Mostly looks good. Please also consider cases of export/import of schema across current and the next version and make sure that works well. Also you were mentioning something about backups, would they be affected by this change?

Reviewed 3 of 9 files at r1.
Reviewable status: 3 of 9 files reviewed, 3 unresolved discussions (waiting on @manishrjain, @martinmr, and @pawanrawal)


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

	js := processQueryNoErr(t, query)
	require.JSONEq(t, `{"data": {"types":[{"name":"Person",
		"fields":[{"name":"name", "type":"default"}, {"name":"pet", "type":"default"}]}]}}`,

Isn't name of type string as defined outside the types. Why is it being return as default?


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

	query := `schema(type: [Person, Animal]) {}`
	js := processQueryNoErr(t, query)
	require.JSONEq(t, `{"data": {"types":[{"name":"Animal",

Can we have a test which returns scalar fields of type uid and [uid] as well?


schema/parse_test.go, line 400 at r1 (raw file):

	reset()
	result, err := Parse(`
		type Person {

Shouldn't this be an error since name isn't defined outside the type definition?

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.

Code looks good! Not much feedback to offer


Reviewed with ❤️ by PullRequest

schema/parse.go Outdated
@@ -351,63 +351,14 @@ func parseTypeDeclaration(it *lex.ItemIterator) (*pb.TypeUpdate, error) {

func parseTypeField(it *lex.ItemIterator) (*pb.SchemaUpdate, error) {
Copy link

Choose a reason for hiding this comment

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

Is this name still appropriate?

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 didn't change the protobuf so backups are not affected.

I missed the export functionality but I fixed it and the tests.

Users will have to change the export output from a previous version and update the types, though. There's not really a way around this though but I'll make sure this makes it to the release notes for the next version.

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


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

Previously, pawanrawal (Pawan Rawal) wrote…

Isn't name of type string as defined outside the types. Why is it being return as default?

This type should only return the name of the fields but we are using the SchemaUpdate proto for them so they have a type. In reality the type here doesn't matter. Ideally, only the name should be returned for now but I need to look at a way to return the types that doesn't include this unnecessary information. See the TODO above.

I'll fix this in a separate PR since I need to think of a way to do it.


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

Previously, pawanrawal (Pawan Rawal) wrote…

Can we have a test which returns scalar fields of type uid and [uid] as well?

This doesn't make sense. This just returns the type definition. Types don't have the concept of scalar or lists, etc. anymore.
See the comment above on why the type is still showing up in type queries.


schema/parse.go, line 352 at r1 (raw file):

Previously, pullrequest[bot] wrote…

Is this name still appropriate?

Yes.


schema/parse_test.go, line 400 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Shouldn't this be an error since name isn't defined outside the type definition?

No. We are not doing this types of checks yet.

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: 3 of 11 files reviewed, 4 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @pullrequest[bot])


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

Previously, martinmr (Martin Martinez Rivera) wrote…

This type should only return the name of the fields but we are using the SchemaUpdate proto for them so they have a type. In reality the type here doesn't matter. Ideally, only the name should be returned for now but I need to look at a way to return the types that doesn't include this unnecessary information. See the TODO above.

I'll fix this in a separate PR since I need to think of a way to do it.

Done.

@martinmr martinmr requested a review from pawanrawal September 18, 2019 19:16
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

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

There's also docs to update. I expect it'll be as simple as just removing the types from the examples.

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

@martinmr martinmr requested a review from danielmai as a code owner September 18, 2019 23:27
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.

Updated the docs.

Reviewable status: 3 of 13 files reviewed, 4 unresolved discussions (waiting on @danielmai, @manishrjain, @pawanrawal, and @pullrequest[bot])

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:

Reviewable status: 3 of 13 files reviewed, 4 unresolved discussions (waiting on @danielmai, @manishrjain, @pawanrawal, and @pullrequest[bot])

MichelDiz added a commit that referenced this pull request Sep 22, 2019
danielmai pushed a commit that referenced this pull request Sep 24, 2019
Type system schema changes required by #4017.
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.

Let's not break the existing APIs. Just ignore if the data type is passed. And accept when is it not passed. Do throw a warning that data type should not be passed along with the field in the type.

:lgtm_strong:

Reviewable status: 3 of 13 files reviewed, 4 unresolved discussions (waiting on @danielmai, @manishrjain, @pawanrawal, and @pullrequest[bot])

@campoy
Copy link
Contributor

campoy commented Sep 27, 2019

Hey, what's the issue related to this?

Changes to the schema or the query language like this are really really important and shouldn't be done without having a clear design in mind first.

What's the motivation behind this change?

Copy link
Contributor

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Let's pause this for a second before we change how types work.

@martinmr
Copy link
Contributor Author

This has already been agreed to. It's part of the conclusions at https://discuss.dgraph.io/t/should-we-remove-the-type-directive/5067/12?u=martinmr

@martinmr martinmr requested review from campoy and pawanrawal October 1, 2019 18:33
@martinmr martinmr merged commit 95d5587 into master Oct 1, 2019
@martinmr martinmr deleted the martinmr/simplify-types branch October 1, 2019 18:39
mangalaman93 added a commit to dgraph-io/pydgraph that referenced this pull request Oct 2, 2019
mangalaman93 added a commit to dgraph-io/dgraph4j that referenced this pull request Oct 2, 2019
mangalaman93 added a commit to dgraph-io/dgo that referenced this pull request Oct 2, 2019
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.

5 participants