refactor: Conceptually clarify schema vs query-language #924

AndrewSisley · 2022-10-28T19:33:35Z

Relevant issue(s)

Resolves #922

Description

Trades performance on a very uncritical code path for conceptual separation. The code is still somewhat coupled together, however it is IMO much clearer than before.

AndrewSisley · 2022-10-28T19:35:51Z

db/schema.go

+) ([]client.CollectionDescription, []core.SchemaDefinition, error) {
+	// We should not be using this package at all here, and should not be doing most of what this package does
+	// Rework: https://github.com/sourcenetwork/defradb/issues/923
+	schemaManager, err := schema.NewSchemaManager()


A new schema manager is required, as each instance caches the types and reusing the gql-query-lang one would cause collisions

thought: I look forward do see what you're going to do here :)

Should be quite simple I think, and we'll be able to go straight from ast to collectionDescription without worrying about gql-types or some virtual fields (IIRC it has to skip over stuff like _group and aggregates)

I don't think we want to do a new schema manager for each createDescriptions exec. You should be able to refer to previous types when registering new types. That wouldn't be possible if we have a new manager on each exec.

I don't necessarily see the benefit of going from AST to Description without a gql type intermediary, but thats a problem that can be kicked down the road.

You should be able to refer to previous types when registering new types. That wouldn't be possible if we have a new manager on each exec

Hmm, I forgot about that, and we have no tests for that either (and so it is quite possibly already broken, and I remember it having been broken before at one point - we need to cover this). For a test gap like this I'd be extremely tempted to say it is broken already, and we need to sort it out in another ticket anyway due to the scope-creep/likelyhood-of-finding-many-more-bugs.

I started working on this, and what you are saying is currently syntactically impossible with our current system. A schema cannot reference an existing type, as that type also needs to be amended to reference the new type (in the schema definition):

var bookAuthorGQLSchema = (` type book { name: String author: author } type author { name: String published: [book] } `)

See how both definitions contain a reference to each other - you cannot add author, and then add book (without update, which we dont support atm).

Realized this after the fact, indeed very true.

I still would've preferred a single instance of the schema, I don't really see the value in creating one for each request. There's no way to reference the collection GQL types in a consistent way.

What benefit does this approach have in your mind?

What benefit does this approach have in your mind?

Absence of state, simplicity, ease of ripping out, minimized risk of state-polution, and the fact that persisting it provides no assertable benefits atm

codecov · 2022-10-28T19:41:37Z

Codecov Report

Merging #924 (3cfc861) into develop (b847465) will decrease coverage by 0.02%.
The diff coverage is 60.52%.

@@             Coverage Diff             @@
##           develop     #924      +/-   ##
===========================================
- Coverage    60.08%   60.05%   -0.03%     
===========================================
  Files          159      159              
  Lines        17523    17532       +9     
===========================================
+ Hits         10528    10529       +1     
- Misses        6061     6066       +5     
- Partials       934      937       +3

Impacted Files	Coverage Δ
db/schema.go	`44.26% <57.14%> (+7.89%)`	⬆️
query/graphql/parser.go	`88.67% <100.00%> (+3.95%)`	⬆️

fredcarle · 2022-10-30T22:16:03Z

query/graphql/parser.go

-		Descriptions: colDesc,
-	}, nil
+func (p *parser) AddSchema(ctx context.Context, schema string) error {
+	_, _, err := p.schemaManager.Generator.FromSDL(ctx, schema)


question: Is this simply cashing the schema for the GQL package?

Generating (the query language types - used for validation and introspection) and caching them - this is done everytime the database is initialized (e.g. on restart), as well as when a new schema is provided

fredcarle · 2022-10-30T22:20:44Z

db/schema.go

+) ([]client.CollectionDescription, []core.SchemaDefinition, error) {
+	// We should not be using this package at all here, and should not be doing most of what this package does
+	// Rework: https://github.com/sourcenetwork/defradb/issues/923
+	schemaManager, err := schema.NewSchemaManager()


thought: I look forward do see what you're going to do here :)

fredcarle

LGTM

jsimnz · 2022-10-30T22:48:31Z

Have some (minor) thoughts, but at all event all day. Should be able to do a review tonight

jsimnz

Looks good, small change was suggested on the SchemaManager stuff

jsimnz · 2022-10-31T05:57:16Z

db/schema.go

+) ([]client.CollectionDescription, []core.SchemaDefinition, error) {
+	// We should not be using this package at all here, and should not be doing most of what this package does
+	// Rework: https://github.com/sourcenetwork/defradb/issues/923
+	schemaManager, err := schema.NewSchemaManager()


I don't think we want to do a new schema manager for each createDescriptions exec. You should be able to refer to previous types when registering new types. That wouldn't be possible if we have a new manager on each exec.

I don't necessarily see the benefit of going from AST to Description without a gql type intermediary, but thats a problem that can be kicked down the road.

jsimnz · 2022-10-31T06:03:14Z

db/schema.go

+		return nil, nil, err
+	}
+
+	types, astdoc, err := schemaManager.Generator.FromSDL(ctx, schemaString)


suggestion(non-blocking): Do you want to expose the BuildTypesFromAST func instead of executing the entire FromSDL generation?

Nope, I do not want to make the existing generator code more complex by adding a new entry point, for the sake of temporary code. Performance doesn't matter much here, so the code-cleanliness and 'keep it as before and dont worry' is much preferred.

A later commit will remove this function from the Parser type, so please dont comment on that :) Performance is not an issue here either, so the wastage is not really an issue in the short/medium-term

Questions answered

…k#924) * Conceptually split up query-language from schema A later commit will remove this function from the Parser type, so please dont comment on that :) Performance is not an issue here either, so the wastage is not really an issue in the short/medium-term * Remove schema definition from parser

AndrewSisley added area/schema Related to the schema system area/parser Related to the parser components refactor This issue specific to or requires *notable* refactoring of existing codebases and components action/no-benchmark Skips the action that runs the benchmark. labels Oct 28, 2022

AndrewSisley added this to the DefraDB v0.4 milestone Oct 28, 2022

AndrewSisley requested a review from a team October 28, 2022 19:33

AndrewSisley self-assigned this Oct 28, 2022

AndrewSisley force-pushed the sisley/refactor/I922-schema-rework branch from c101b6a to bd6594a Compare October 28, 2022 19:34

AndrewSisley commented Oct 28, 2022

View reviewed changes

fredcarle reviewed Oct 30, 2022

View reviewed changes

fredcarle approved these changes Oct 30, 2022

View reviewed changes

jsimnz previously requested changes Oct 31, 2022

View reviewed changes

AndrewSisley requested a review from jsimnz October 31, 2022 15:16

AndrewSisley added 2 commits November 1, 2022 09:37

Conceptually split up query-language from schema

a325be5

A later commit will remove this function from the Parser type, so please dont comment on that :) Performance is not an issue here either, so the wastage is not really an issue in the short/medium-term

Remove schema definition from parser

3cfc861

AndrewSisley force-pushed the sisley/refactor/I922-schema-rework branch from bd6594a to 3cfc861 Compare November 1, 2022 13:37

AndrewSisley merged commit a530749 into develop Nov 1, 2022

AndrewSisley deleted the sisley/refactor/I922-schema-rework branch November 1, 2022 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Conceptually clarify schema vs query-language #924

refactor: Conceptually clarify schema vs query-language #924

AndrewSisley commented Oct 28, 2022

AndrewSisley Oct 28, 2022

fredcarle Oct 30, 2022

AndrewSisley Oct 30, 2022

jsimnz Oct 31, 2022

AndrewSisley Oct 31, 2022

AndrewSisley Oct 31, 2022 •

edited

Loading

jsimnz Nov 1, 2022 •

edited

Loading

AndrewSisley Nov 1, 2022

codecov bot commented Oct 28, 2022 •

edited

Loading

fredcarle Oct 30, 2022

AndrewSisley Oct 30, 2022

fredcarle Oct 30, 2022

fredcarle left a comment

jsimnz commented Oct 30, 2022

jsimnz left a comment

jsimnz Oct 31, 2022

jsimnz Oct 31, 2022

AndrewSisley Oct 31, 2022

refactor: Conceptually clarify schema vs query-language #924

refactor: Conceptually clarify schema vs query-language #924

Conversation

AndrewSisley commented Oct 28, 2022

Relevant issue(s)

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

jsimnz Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 28, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

jsimnz commented Oct 30, 2022

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 31, 2022 •

edited

Loading

jsimnz Nov 1, 2022 •

edited

Loading

codecov bot commented Oct 28, 2022 •

edited

Loading