refactor: Cleanup parsing logic #909

AndrewSisley · 2022-10-19T20:18:35Z

Relevant issue(s)

Part of #905, but does not yet resolve it

Description

Cleans up some of the parser code and types in preparation for decoupling mapper/planner/defra from gql. Moves the parser model out of the parser package, defining a public strongly typed model for interacting with defra.

There is still a little bit more to be done (mainly abstracting the schema stuff from away db.db), but I'm getting tired and these commits should be alright to review (it is also getting a bit large and will probs be easier for everyone to break this refactor into a couple of reviews).

Strongly suggest reviewing commit by commit, as there is a fair amount going on here now.

AndrewSisley · 2022-10-19T20:20:03Z

query/graphql/parser/query.go

 		Name:  field.Name.Value,
 		Alias: getFieldAlias(field),
 	}
 }

+type Aggregate struct {


I'll document this (and AggregateTarget) a bit later (outside of this PR), along with all the other models in this package. It'll also probably be refactored a little further.

AndrewSisley · 2022-10-19T20:21:07Z

query/graphql/parser/query.go

+func parseAggregate(field *ast.Field, index int) (*Aggregate, error) {
+	targets := make([]*AggregateTarget, len(field.Arguments))
+
+	for i, argument := range field.Arguments {


This function is pretty much a straight-up cut-paste of the trimmed one in mapper

codecov · 2022-10-19T20:22:42Z

Codecov Report

Merging #909 (29ef5f5) into develop (9490679) will increase coverage by 0.09%.
The diff coverage is 93.46%.

@@             Coverage Diff             @@
##           develop     #909      +/-   ##
===========================================
+ Coverage    59.79%   59.89%   +0.09%     
===========================================
  Files          156      159       +3     
  Lines        17415    17423       +8     
===========================================
+ Hits         10414    10436      +22     
+ Misses        6065     6053      -12     
+ Partials       936      934       -2

Impacted Files	Coverage Δ
db/collection_delete.go	`29.41% <0.00%> (ø)`
mapper/commitSelect.go	`0.00% <ø> (ø)`
mapper/targetable.go	`75.00% <ø> (ø)`
query/graphql/schema/descriptions.go	`87.57% <50.00%> (ø)`
db/collection_update.go	`54.93% <75.00%> (ø)`
planner/group.go	`83.51% <75.00%> (ø)`
query/graphql/parser/filter.go	`62.71% <80.00%> (ø)`
planner/planner.go	`77.21% <87.50%> (ø)`
query/graphql/parser/query.go	`82.77% <90.11%> (-0.31%)`	⬇️
planner/select.go	`78.32% <91.66%> (ø)`
... and 21 more

AndrewSisley · 2022-10-20T18:58:38Z

planner/limit.go

@@ -58,7 +58,7 @@ func (n *limitNode) Value() core.Doc        { return n.plan.Value() }

 func (n *limitNode) Next() (bool, error) {
 	// check if we're passed the limit
-	if n.limit != 0 && n.rowIndex-n.offset >= n.limit {
+	if n.limit != 0 && n.rowIndex >= n.limit+n.offset {


This had to be changed, as it actually overflows under normal conditions. A bit surprised Golang doesn't panic on overflows...

I'm trying to undestand where this overfows. Can you point it out please. I'm curious.

e.g if rowIndex is 0, and offset is greater than 0

Ah yes I see cuz they are of type uint64. It should definitely panic on overflow. https://go.dev/play/p/bDfmcZbLKma

No... my bad. It doesn't panic: https://go.dev/play/p/yHe5EyEu11I

Rust does panic: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cc6ef0c06d3af96737ec957ef1a823df

I wonder why the Go team made that choice.

Is really strange behaviour that to me in your linked playgrounds, feels inconsistent to let the runtime pass, but compile fail.

DotNet panics too, and I think so does the JVM. There is a tiny runtime cost to checking, but given how runtime heavy Go can be anyway it seems odd not to do this. Maybe it is because the lang lacks the syntax to opt-out (allowing overflows can be really handy at times, for example during key generation)

fredcarle

LGTM. This was quite easy to review. Lots of changes but not too much new stuff. Glad you've decided to create PRs as you change things and not wait for everything to be done.

shahzadlone · 2022-10-21T07:50:34Z

client/request/consts.go

@@ -8,65 +8,20 @@
 // by the Apache License, Version 2.0, included in the file


question: is the purpose of the consts.go file different from the types file? I thought there was consensus to keep types under it's own namespace using types package ?

having a types file in directory full of other files with other types is not very helpful IMO. A types file made sense when there was only a sub set of the model in it (and even that was odd as is was super-arbitrary as to what was in it)

having a types file in directory full of other files with other types is not very helpful IMO. A types file made sense when there was only a sub set of the model in it (and even that was odd as is was super-arbitrary as to what was in it)

While I agree that it was odd, I am talking from the perspective of keeping things in harmony as the code base grows and having a convention that would be well understood by the team, for example, I am happy with storing constants in consts.go file as it is very self-evident to know what that file contains. Would like to see us use consistent patterns to store constants, variables etc. (for structs probably more useful names split under different files). I really enjoyed a popular convention in C++ that said no more than one class / struct per file (perhaps too strict for Go peepz haha).

shahzadlone · 2022-10-21T07:52:44Z

client/request/filter.go

+
+// Filter contains the parsed condition map to be
+// run by the Filter Evaluator.
+// @todo: Cache filter structure for faster condition


todo: remove or add corresponding issue number.

out of scope

shahzadlone · 2022-10-21T07:54:05Z

client/request/mutation.go

+// of a graphql query. It includes all the possible
+// arguments and all
+//
+// @todo: Change name to ObjectMutation to indicate


todo: remove or add corresponding issue number.

out of scope

shahzadlone · 2022-10-21T08:02:01Z

mapper/targetable.go

-type OptionalDocKeys struct {
-	HasValue bool
-	Value    []string
-}
-


praise: Thanks for removal of this.

shahzadlone · 2022-10-21T08:06:30Z

planner/select.go

@@ -292,7 +293,7 @@ func (n *selectNode) initFields(parsed *mapper.Select) ([]aggregateNode, error)
 					// of that Target version we are querying.
 					// So instead of a LatestCommit subquery, we need
 					// a OneCommit subquery, with the supplied parameters.
-					commitSlct.DocKey = parsed.DocKeys.Value[0] // @todo check length
+					commitSlct.DocKey = client.Some(parsed.DocKeys.Value()[0]) // @todo check length


todo: remove or add the corresponding issue number.

out of scope

AndrewSisley · 2022-10-21T13:55:27Z

Thanks Fred! And thanks for the quick review :)

Type was added before we had client.Option

Should have been removed in the PR that removed the commit query

The parser models will shortly be exposed as part of the public interface, and it should not be on the users to have to always populate this property. It also makes more sense IMO to do it this way regardless, as alias is optional, but render-keys are not.

Makes it far clearer that it is optional

Also flattens them, as the previous structure suggested if one was present then the other was mandatory. Also switches from int64 to uint64 as they should not be negative (and makes it more consitent with a few other similar props who use uint64).

No reason for this to exist, and will confuse anyone using it.

Now using proper consts, and defined in the same location as everything else.

Previous if was conceptually incorrect. None option added no value (select.Root now defaults to Object, which could be handy for users anyway). Also types the enum.

Also breaks up types.go in preparation for the moving of the rest of the model.

As agreed as a team, commit does not seek to rename all variables referencing this - that can be done later to save drowning this large PR with minor changes

The parser package is now responisble for converting an gql-ast into a shared model. Soon it will be responisble for converting a string into the shared model. Mostly a copy-paste commit.

todos are out of scope

* Remove unused QueryType prop * Remove unused order.statement prop * Replace optionalDocKeys with client.Option Type was added before we had client.Option * Remove legacy commit entry Should have been removed in the PR that removed the commit query * Remove unused statement property * Remove unused func from interface * Remove Root from Field * Remove unused func * Parse aggregates in parser package * Remove statement from parser.Select * Remove ast from OperationDefinition * Remove unused Name from OperationDefinition * Make parser.Alias optional The parser models will shortly be exposed as part of the public interface, and it should not be on the users to have to always populate this property. It also makes more sense IMO to do it this way regardless, as alias is optional, but render-keys are not. * Make commit.DocKey option type Makes it far clearer that it is optional * Use option for parser.limit Also flattens them, as the previous structure suggested if one was present then the other was mandatory. Also switches from int64 to uint64 as they should not be negative (and makes it more consitent with a few other similar props who use uint64). * Use option for parser.order * Remove '.'s from parser.order.fields No reason for this to exist, and will confuse anyone using it. * Use option for parser.GroupBy * Use option for parser.Filter * Cleanup commit-query branching Now using proper consts, and defined in the same location as everything else. * Remove unused param * Remove unused commit.GetRoot func * Remove unused mutation.GetRoot func * Tidy up parser.Root references Previous if was conceptually incorrect. None option added no value (select.Root now defaults to Object, which could be handy for users anyway). Also types the enum. * Move parser.types to client dir Also breaks up types.go in preparation for the moving of the rest of the model. * Rename parser.Query to parser.Request As agreed as a team, commit does not seek to rename all variables referencing this - that can be done later to save drowning this large PR with minor changes * Move request model out of parser package The parser package is now responisble for converting an gql-ast into a shared model. Soon it will be responisble for converting a string into the shared model. Mostly a copy-paste commit.

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

AndrewSisley requested a review from a team October 19, 2022 20:18

AndrewSisley self-assigned this Oct 19, 2022

AndrewSisley commented Oct 19, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I905-decouple-from-gql branch 2 times, most recently from 4b0084c to 18534ad Compare October 19, 2022 21:01

AndrewSisley commented Oct 20, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/refactor/I905-decouple-from-gql branch 2 times, most recently from 1c17c39 to 7e13656 Compare October 20, 2022 21:19

fredcarle approved these changes Oct 20, 2022

View reviewed changes

shahzadlone reviewed Oct 21, 2022

View reviewed changes

shahzadlone previously requested changes Oct 21, 2022

View reviewed changes

AndrewSisley requested a review from shahzadlone October 21, 2022 13:54

AndrewSisley force-pushed the sisley/refactor/I905-decouple-from-gql branch from 7e13656 to b564448 Compare October 21, 2022 13:57

AndrewSisley added 7 commits October 21, 2022 10:15

Remove unused QueryType prop

a495605

Remove unused order.statement prop

09ee245

Replace optionalDocKeys with client.Option

59a2552

Type was added before we had client.Option

Remove legacy commit entry

dbc0c29

Should have been removed in the PR that removed the commit query

Remove unused statement property

2aa6fce

Remove unused func from interface

b29ef39

Remove Root from Field

70b551b

AndrewSisley added 20 commits October 21, 2022 10:15

Remove unused func

905e0b1

Parse aggregates in parser package

8541eb7

Remove statement from parser.Select

b57cbb9

Remove ast from OperationDefinition

223ee45

Remove unused Name from OperationDefinition

2b32b4d

Make parser.Alias optional

54c01da

The parser models will shortly be exposed as part of the public interface, and it should not be on the users to have to always populate this property. It also makes more sense IMO to do it this way regardless, as alias is optional, but render-keys are not.

Make commit.DocKey option type

96a2e5f

Makes it far clearer that it is optional

Use option for parser.limit

9dd361f

Also flattens them, as the previous structure suggested if one was present then the other was mandatory. Also switches from int64 to uint64 as they should not be negative (and makes it more consitent with a few other similar props who use uint64).

Use option for parser.order

08200c1

Remove '.'s from parser.order.fields

8fd6daa

No reason for this to exist, and will confuse anyone using it.

Use option for parser.GroupBy

fea5168

Use option for parser.Filter

03af621

Cleanup commit-query branching

22ea7bf

Now using proper consts, and defined in the same location as everything else.

Remove unused param

cf81e28

Remove unused commit.GetRoot func

fc63d4f

Remove unused mutation.GetRoot func

23660f6

Tidy up parser.Root references

fd4d0ef

Previous if was conceptually incorrect. None option added no value (select.Root now defaults to Object, which could be handy for users anyway). Also types the enum.

Move parser.types to client dir

79d15b4

Also breaks up types.go in preparation for the moving of the rest of the model.

Rename parser.Query to parser.Request

743183e

As agreed as a team, commit does not seek to rename all variables referencing this - that can be done later to save drowning this large PR with minor changes

Move request model out of parser package

29ef5f5

The parser package is now responisble for converting an gql-ast into a shared model. Soon it will be responisble for converting a string into the shared model. Mostly a copy-paste commit.

AndrewSisley force-pushed the sisley/refactor/I905-decouple-from-gql branch from b564448 to 29ef5f5 Compare October 21, 2022 14:15

AndrewSisley merged commit 6ccae7f into develop Oct 21, 2022

AndrewSisley deleted the sisley/refactor/I905-decouple-from-gql branch October 21, 2022 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Cleanup parsing logic #909

refactor: Cleanup parsing logic #909

AndrewSisley commented Oct 19, 2022 •

edited

Loading

AndrewSisley Oct 19, 2022 •

edited

Loading

AndrewSisley Oct 19, 2022

codecov bot commented Oct 19, 2022 •

edited

Loading

AndrewSisley Oct 20, 2022

fredcarle Oct 20, 2022

AndrewSisley Oct 20, 2022

fredcarle Oct 20, 2022

fredcarle Oct 20, 2022

fredcarle Oct 21, 2022

AndrewSisley Oct 21, 2022

fredcarle left a comment

shahzadlone Oct 21, 2022

AndrewSisley Oct 21, 2022

shahzadlone Oct 21, 2022

shahzadlone Oct 21, 2022

AndrewSisley Oct 21, 2022

shahzadlone Oct 21, 2022

AndrewSisley Oct 21, 2022

shahzadlone Oct 21, 2022

AndrewSisley Oct 21, 2022

shahzadlone Oct 21, 2022

AndrewSisley Oct 21, 2022

AndrewSisley commented Oct 21, 2022

		@@ -8,65 +8,20 @@
		// by the Apache License, Version 2.0, included in the file

refactor: Cleanup parsing logic #909

refactor: Cleanup parsing logic #909

Conversation

AndrewSisley commented Oct 19, 2022 • edited Loading

Relevant issue(s)

Description

AndrewSisley Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 19, 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

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

fredcarle 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley commented Oct 21, 2022

AndrewSisley commented Oct 19, 2022 •

edited

Loading

AndrewSisley Oct 19, 2022 •

edited

Loading

codecov bot commented Oct 19, 2022 •

edited

Loading