feat: Add support for sum aggregate #121

AndrewSisley · 2022-01-17T21:12:03Z

Closes #94

Adds support for the summing of child collections of Int and Floats. Also refactors shared aggregated code somewhat (likely to be further altered when adding third aggregate).

Todo:

Update docs post-merge
Add tests (and support) for scalar summation
Review/update generate code once thunk PR is merged fix: Handle errors generated during input object thunks #123
If merged after feature: Error if aggregates dont specify which field to aggregate #141 - updated tests and behaviour to match count
If proposal commit accepted, reword it, or consider squashing it with the other aggregate-specific commits before merging the PR

codecov · 2022-01-17T21:13:14Z

Codecov Report

Merging #121 (def8d94) into develop (d0ba8d4) will increase coverage by 0.31%.
The diff coverage is 78.77%.

❗ Current head def8d94 differs from pull request most recent head be59b19. Consider uploading reports for the commit be59b19 to get more accurate results

@@             Coverage Diff             @@
##           develop     #121      +/-   ##
===========================================
+ Coverage    63.85%   64.17%   +0.31%     
===========================================
  Files           80       81       +1     
  Lines         7334     7547     +213     
===========================================
+ Hits          4683     4843     +160     
- Misses        2165     2208      +43     
- Partials       486      496      +10

Impacted Files	Coverage Δ
db/tests/query/simple/utils.go	`100.00% <ø> (ø)`
query/graphql/parser/commit.go	`59.61% <ø> (-7.06%)`	⬇️
query/graphql/planner/sum.go	`72.54% <72.54%> (ø)`
query/graphql/planner/count.go	`81.57% <78.57%> (-3.61%)`	⬇️
query/graphql/planner/select.go	`77.29% <79.10%> (+0.56%)`	⬆️
query/graphql/schema/generate.go	`82.35% <81.25%> (-0.43%)`	⬇️
query/graphql/parser/query.go	`74.68% <85.36%> (+0.23%)`	⬆️
query/graphql/planner/planner.go	`73.20% <100.00%> (+0.43%)`	⬆️
... and 3 more

jsimnz · 2022-01-19T11:42:58Z

Do you want me to review this code before we talk about some potential architecture changes to the aggregate code?

AndrewSisley · 2022-01-19T13:34:23Z

Do you want me to review this code before we talk about some potential architecture changes to the aggregate code?

Yes please - if I make those changes it will be as part of the next aggregate :) And some of the refactorings here would likely be very useful to have (as well as a 3rd implementation)

jsimnz · 2022-01-19T14:19:42Z

Do you want me to review this code before we talk about some potential architecture changes to the aggregate code?

Yes please - if I make those changes it will be as part of the next aggregate :) And some of the refactorings here would likely be very useful to have (as well as a 3rd implementation)

Sounds good, ill jump on it

jsimnz

few things, nothing major

query/graphql/schema/generate.go

db/tests/query/inline_array/with_sum_test.go

query/graphql/parser/query.go

query/graphql/planner/planner.go

query/graphql/planner/select.go

query/graphql/planner/sum.go

jsimnz

some stuff about parsing.

jsimnz · 2022-01-27T05:41:29Z

query/graphql/parser/query.go

@@ -435,16 +414,31 @@ func parseSelectFields(root SelectionType, fields *ast.SelectionSet) ([]Selectio

 // parseField simply parses the Name/Alias
 // into a Field type
-func parseField(root SelectionType, field *ast.Field) *Field {
+func parseField(i int, root SelectionType, field *ast.Field) (*Field, error) {


Should we be checking if the field argument on an aggregate is empty here during the parsing. I know theres an error catch further down the line when you create a aggregate node in the GetAggregateSource func but at that point theres already tons of work gone into creating the query plan. Would be preferable if we could jump out early if theres that error during parsing, and not plan building.

The amount of required paths is very specific to the type of aggregate (e.g. _count requires one, and _sum requires 2 unless it is pointed at a scalar array). I'd really rather not leak those implementation details into the shared code

Not sure what you mean by _count requires one and _sum requires 2? They both require the field argument no? Even just the existence of the value is prob good enough, without going into more semantic details. Its formally a parsing validation issue compared to a planning issue.

This is the sumType argument generator, as far as I can see, its only field, no?

defradb/query/graphql/schema/generate.go

Line 523 in def8d94

"field": newArgConfig(sumType),

Also, if im understanding correctly, its not really implementation details it they are query spec compliant details.

node implementation details, not defra :) The check is different between count and sum as count needs to reference a list/slice/whatever whereas sum needs to reference a property of a list-item (unless it is a scalar-array). It is not possible to have this in the shared code without the shared code knowing this (e.g. via an if/switch-count/sum/etc or some extra abstract complexity).

2 was a lazy reference to the source variable(s), which needs to be len = 1 for count, and either 1 or two for sum depending on target type.

jsimnz · 2022-01-27T05:42:22Z

query/graphql/parser/query.go

 		case string:
 			path = []string{arguementValue}
 		case []*ast.ObjectField:
 			if len(arguementValue) == 0 {
-				//Note: Scalar arrays will hit this clause and should be handled appropriately (not adding now as not testable in a time-efficient manner)
-				return fmt.Errorf("Unexpected error: aggregate field contained no child field selector"), PropertyTransformation{}
+				return []string{}, fmt.Errorf("Unexpected error: aggregate field contained no child field selector")


See here as a reference for my comment above. We can keep this error check here, but it should be initially checked during parsing, and not planning.

query/graphql/planner/select.go

jsimnz

Does the sum system (and I guess the count system) support aggregating values 2 levels deep into a relation? From what I can tell it might only be able to go 1 level deep. I can test on my side, but wanted to ask anyway.

query/graphql/planner/sum.go

Makes it easier to share code between aggregates if they are parsed into the same struct

New implementation should happily support other aggregates out of the box

jsimnz

Had a few suggestions about the parser and initFields stuff. Neither are necessary right now, but id love your thoughts and if theres some consensus then we can make a seperate issue for some small refactoring.

If youd prefer to make the changes now, feel free to, otherwise, im approving this 👍

jsimnz · 2022-01-30T20:25:54Z

query/graphql/parser/query.go

@@ -435,16 +414,31 @@ func parseSelectFields(root SelectionType, fields *ast.SelectionSet) ([]Selectio

 // parseField simply parses the Name/Alias
 // into a Field type
-func parseField(root SelectionType, field *ast.Field) *Field {
+func parseField(i int, root SelectionType, field *ast.Field) (*Field, error) {


Not sure what you mean by _count requires one and _sum requires 2? They both require the field argument no? Even just the existence of the value is prob good enough, without going into more semantic details. Its formally a parsing validation issue compared to a planning issue.

This is the sumType argument generator, as far as I can see, its only field, no?

defradb/query/graphql/schema/generate.go

Line 523 in def8d94

"field": newArgConfig(sumType),

jsimnz · 2022-01-30T20:28:56Z

query/graphql/parser/query.go

@@ -435,16 +414,31 @@ func parseSelectFields(root SelectionType, fields *ast.SelectionSet) ([]Selectio

 // parseField simply parses the Name/Alias
 // into a Field type
-func parseField(root SelectionType, field *ast.Field) *Field {
+func parseField(i int, root SelectionType, field *ast.Field) (*Field, error) {


Also, if im understanding correctly, its not really implementation details it they are query spec compliant details.

query/graphql/parser/query.go

query/graphql/planner/count.go

query/graphql/planner/select.go

jsimnz · 2022-01-30T20:52:48Z

query/graphql/planner/select.go

-				break
-			}
+// Join any child collections required by the given transformation if the child collections have not been requested for render by the consumer
+func (n *selectNode) joinAggregatedChild(parsed *parser.Select, field *parser.Field) error {


Another spot where I think it would be cleaner to pass in a parser.AggregateField instead of just a parser.Field.

* Remove unnessecary object deconstruction * Correct count test description * Handle count generation errors * Replace query.Count struct with query.PropertyTransformation Makes it easier to share code between aggregates if they are parsed into the same struct * Generalize aggregate alias magic New implementation should happily support other aggregates out of the box * Move count plan planning to expand aggregate plans function * Extract hidden join logic to generic function * Add support for sum aggregate

AndrewSisley added feature New feature or request area/query Related to the query component labels Jan 17, 2022

AndrewSisley requested review from jsimnz and shahzadlone January 17, 2022 21:12

AndrewSisley self-assigned this Jan 17, 2022

AndrewSisley force-pushed the sisley/feat/I94-sum branch 8 times, most recently from 3b2e4ce to d504c8d Compare January 24, 2022 17:01

AndrewSisley mentioned this pull request Jan 25, 2022

feature: Error if aggregates dont specify which field to aggregate #141

Merged

1 task

jsimnz requested changes Jan 25, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/feat/I94-sum branch 11 times, most recently from c6f67ce to c5635d5 Compare January 25, 2022 22:48

AndrewSisley force-pushed the sisley/feat/I94-sum branch 3 times, most recently from 14231d0 to 1655062 Compare January 25, 2022 23:00

AndrewSisley requested a review from jsimnz January 25, 2022 23:04

jsimnz requested changes Jan 27, 2022

View reviewed changes

jsimnz reviewed Jan 27, 2022

View reviewed changes

query/graphql/planner/select.go Outdated Show resolved Hide resolved

jsimnz reviewed Jan 27, 2022

View reviewed changes

query/graphql/planner/sum.go Show resolved Hide resolved

AndrewSisley force-pushed the sisley/feat/I94-sum branch 2 times, most recently from 9ee87b8 to a49f019 Compare January 27, 2022 12:56

AndrewSisley requested a review from jsimnz January 27, 2022 13:02

AndrewSisley force-pushed the sisley/feat/I94-sum branch from a49f019 to d58fa51 Compare January 27, 2022 17:50

AndrewSisley added 2 commits January 27, 2022 14:22

Remove unnessecary object deconstruction

48bb4f6

Correct count test description

e1467ba

AndrewSisley force-pushed the sisley/feat/I94-sum branch from d58fa51 to 12adc78 Compare January 27, 2022 19:27

AndrewSisley added 5 commits January 27, 2022 18:00

Handle count generation errors

09714f2

Replace query.Count struct with query.PropertyTransformation

7eb898a

Makes it easier to share code between aggregates if they are parsed into the same struct

Generalize aggregate alias magic

b0d7bc3

New implementation should happily support other aggregates out of the box

Move count plan planning to expand aggregate plans function

21b6e82

Extract hidden join logic to generic function

766b354

AndrewSisley force-pushed the sisley/feat/I94-sum branch 2 times, most recently from 1249da6 to def8d94 Compare January 27, 2022 23:01

jsimnz approved these changes Jan 30, 2022

View reviewed changes

Add support for sum aggregate

be59b19

AndrewSisley force-pushed the sisley/feat/I94-sum branch from def8d94 to be59b19 Compare January 30, 2022 23:31

AndrewSisley merged commit 8c4cc7e into develop Jan 30, 2022

AndrewSisley deleted the sisley/feat/I94-sum branch January 30, 2022 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for sum aggregate #121

feat: Add support for sum aggregate #121

AndrewSisley commented Jan 17, 2022 •

edited

Loading

codecov bot commented Jan 17, 2022 •

edited

Loading

jsimnz commented Jan 19, 2022

AndrewSisley commented Jan 19, 2022

jsimnz commented Jan 19, 2022

jsimnz left a comment

jsimnz left a comment

jsimnz Jan 27, 2022

AndrewSisley Jan 27, 2022

jsimnz Jan 30, 2022

jsimnz Jan 30, 2022

AndrewSisley Jan 30, 2022 •

edited

Loading

jsimnz Jan 27, 2022

jsimnz left a comment

jsimnz left a comment

jsimnz Jan 30, 2022

jsimnz Jan 30, 2022

jsimnz Jan 30, 2022

feat: Add support for sum aggregate #121

feat: Add support for sum aggregate #121

Conversation

AndrewSisley commented Jan 17, 2022 • edited Loading

codecov bot commented Jan 17, 2022 • edited Loading

Codecov Report

jsimnz commented Jan 19, 2022

AndrewSisley commented Jan 19, 2022

jsimnz commented Jan 19, 2022

jsimnz left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

AndrewSisley Jan 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

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 commented Jan 17, 2022 •

edited

Loading

codecov bot commented Jan 17, 2022 •

edited

Loading

AndrewSisley Jan 30, 2022 •

edited

Loading