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

Store and query types. #3018

Merged
merged 14 commits into from
Mar 11, 2019
Merged

Store and query types. #3018

merged 14 commits into from
Mar 11, 2019

Conversation

martinmr
Copy link
Contributor

@martinmr martinmr commented Feb 14, 2019

This change adds the ability to store the types and query them similarly
to how the schema is currently being parsed.


This change is Reviewable

This change adds the ability to store the types and query them similarly
to how the schema is currently being parsed.
@martinmr martinmr requested a review from a team February 14, 2019 22:38
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: Test this thoroughly, so you're not breaking anything.

I don't yet understand how would snapshot copies work -- do test them out manually -- when a snapshot is copied, all previous data is deleted, so the type system would need to be copied as well. @danielmai can help you with those.

Reviewed 10 of 16 files at r1, 5 of 6 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @martinmr)


gql/parser.go, line 889 at r2 (raw file):

// parseSchemaPredsOrTypes parses till rightround is found
func parseSchemaPredsOrTypes(it *lex.ItemIterator, s *pb.SchemaRequest) error {
	// pred (or type) should be followed by colon

remove the brackets.


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

const testSchema = `
type Person {
	name: string

Can we support capital S String?

name: String


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

	pet: Animal
}

Add a test here picking from a real GraphQL example from their docs, if possible. We should be able to parse their schema, w.r.t. the type system.


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

			return er, x.Wrapf(&InternalError{err: err}, "error while fetching schema")
		}
		if er.Types, err = worker.GetTypes(ctx, req.GqlQuery.Schema); err != nil {

Should it be req.GqlQuery.Schema? Or, Types or something?


schema/schema.go, line 96 at r2 (raw file):

	glog.Infof("Deleting type definition for type: [%s]", typeName)
	delete(s.types, typeName)

I'd delete from memory after successfully deleting it from disk.


x/keys.go, line 265 at r2 (raw file):

}

// SchemaPrefix returns the prefix for Schema keys.

The comment is incorrect.

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: 6 of 16 files reviewed, 6 unresolved discussions (waiting on @manishrjain)


gql/parser.go, line 889 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

remove the brackets.

Done.


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

Previously, manishrjain (Manish R Jain) wrote…

Can we support capital S String?

name: String

Done. Primitive types are not case sensitive so this is already supported. I added a explicit test for this.


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

Previously, manishrjain (Manish R Jain) wrote…

Add a test here picking from a real GraphQL example from their docs, if possible. We should be able to parse their schema, w.r.t. the type system.

There's a lot more parsing tests in schema/parse_test.go that tests a lot more stuff (non-nullable types, lists, etc.). I kept the types simple here since the tests are mostly about being able to retrieve the types.


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

Previously, manishrjain (Manish R Jain) wrote…

Should it be req.GqlQuery.Schema? Or, Types or something?

This is fine. The reason it's that types are queried in a schema block.


schema/schema.go, line 96 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I'd delete from memory after successfully deleting it from disk.

Done.


x/keys.go, line 265 at r2 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

The comment is incorrect.

Done.

@martinmr
Copy link
Contributor Author

martinmr commented Mar 9, 2019

Did more testing. Snapshots work as expected (was able to stop replicas and the entire cluster and still get the types back)

@martinmr martinmr merged commit d5d4883 into master Mar 11, 2019
@martinmr martinmr deleted the martinmr/store-types branch March 11, 2019 17:41
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 19, 2019
This change adds the ability to store the types and query them similarly
to how the schema is currently being parsed.
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.

2 participants