feat: Allow summation of aggregates #341
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewSisley
added
feature
AndrewSisley
commented
Apr 12, 2022
| } | ||
|
|
||
| // Returns true if the value to be summed is a float, otherwise false. | ||
| func (p *Planner) isValueFloat( |
There was a problem hiding this comment.
This will likely move when adding other aggregates, but it can live here for now (count cant act on an aggregate and so wont need this)
There was a problem hiding this comment.
I don't think this func needs to be a method on the planner object. From what I can tell, the only reason it is, is because the getSourceField is on the planner.
But getSourceField also probably doesn't need to be on planner, which calls getSourceProperty which doens't call anything from the planner object.
So I suggest, isValueFloat, getSourceField, and getSourceProperty all get lifted off the planner.
There was a problem hiding this comment.
In previous PRs you have requested the opposite and asked for unscoped functions to be scoped to the planner! Is a weakness of Go IMO that it has no private functions, and atm I would really rather not have other bits of code call these functions. Planner is a pretty weak scope, but at least they are not global...
As noted, it is likely that these functions will become shared code, but they are not written/tested as such at the moment and it would be wasteful to put that effort in now
Codecov Report
@@ Coverage Diff @@
## develop #341 +/- ##
===========================================
+ Coverage 64.98% 65.05% +0.06%
===========================================
Files 80 80
Lines 9183 9251 +68
===========================================
+ Hits 5968 6018 +50
- Misses 2602 2618 +16
- Partials 613 615 +2
|
AndrewSisley
force-pushed
the
sisley/feat/I149-sum-of-sums
branch
from
8f0ebf5
to
08bef70
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
force-pushed
the
sisley/feat/I149-sum-of-sums
branch
2 times, most recently
from
5e3637e
to
5164d0e
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shahzadlone
reviewed
Apr 17, 2022
query/graphql/planner/sum.go
Outdated
Comment on lines
180
to
195
| if _, isAggregate := parser.Aggregates[sourceProperty]; isAggregate { | ||
| var sourceField *parser.Field | ||
| for _, field := range parent.GetSelections() { | ||
| if field.GetName() == source[0] { | ||
| for _, childField := range field.(*parser.Select).Fields { | ||
| if childField.GetAlias() == sourceProperty { | ||
| sourceField = childField.(*parser.Field) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| sourceProperty = sourceField.GetName() | ||
| } | ||
|
|
||
| return sourceProperty |
There was a problem hiding this comment.
Less nesting this way:
Suggested change
| if _, isAggregate := parser.Aggregates[sourceProperty]; isAggregate { | |
| var sourceField *parser.Field | |
| for _, field := range parent.GetSelections() { | |
| if field.GetName() == source[0] { | |
| for _, childField := range field.(*parser.Select).Fields { | |
| if childField.GetAlias() == sourceProperty { | |
| sourceField = childField.(*parser.Field) | |
| } | |
| } | |
| } | |
| } | |
| sourceProperty = sourceField.GetName() | |
| } | |
| return sourceProperty | |
| if _, isAggregate := parser.Aggregates[sourceProperty]; !isAggregate { | |
| return sourceProperty | |
| } | |
| var sourceField *parser.Field | |
| for _, field := range parent.GetSelections() { | |
| if field.GetName() != source[0] { | |
| continue | |
| } | |
| for _, childField := range field.(*parser.Select).Fields { | |
| if childField.GetAlias() == sourceProperty { | |
| sourceField = childField.(*parser.Field) | |
| } | |
| } | |
| } | |
| return sourceField.GetName() |
There was a problem hiding this comment.
There is less indentation, but I think you lose in terms of content structure - the if isAggregate block is a second class citizen here - an addition to the normal code flow (and so are the inner ifs). Returning early changes the emphasis (the primary code path being shoved into an early return), and makes it easier to introduce bugs when adding to the primary code path (you need to be mindful of the early return instead of being able to add to any line in the function. But will review properly and get back to you.
- Review isAggregate block
There was a problem hiding this comment.
On review I still prefer the existing, but also spotted that I missed a could of breaks that have now been added. Again let me know if you strongly object and I can move things as you suggested.
There was a problem hiding this comment.
Reasoning makes sense, in that case would it be possible to use helper function / method to knockdown the cyclomatic complexity. i.e. maybe a function just for the inner most for. However I am guessing you might have to pass a pointer in the helper to mutate and grab the state ( in that case dw about it ). Otherwise looks good!
There was a problem hiding this comment.
My bad - this is the helper function you are asking for - I have tweaked it slightly to return early instead of the mutation and control-flow statements...
AndrewSisley
force-pushed
the
sisley/feat/I149-sum-of-sums
branch
2 times, most recently
from
151c817
to
2b9da2d
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
fredcarle
reviewed
Apr 19, 2022
| sourceInfo *sourceInfo, | ||
| field *parser.Field, | ||
| parent *parser.Select, | ||
| ) (*sumNode, error) { |
There was a problem hiding this comment.
Can we keep this on a single line? Or did we decide to format function declarations that way past a certain number of characters?
There was a problem hiding this comment.
linter said no - there is a char limit that didnt like it when it was on one line. It might now fit as the limit was expanded slightly, but I wasn't bothered enough to see if it does (apart from Golang forcing ) (*sumNode, ...) { to have zero indentation, it's probably 50-50 which is better here and different devs will have different opinions.
There was a problem hiding this comment.
If the linter doesn't complain, then it's okay to follow whichever way seems reasonable to the developer for stylistic things like these. If there are advantages to formatting a certain way, then we can enforce it through linters or other tooling in the future.
shahzadlone
reviewed
Apr 19, 2022
query/graphql/planner/sum.go
Outdated
| break | ||
| } | ||
| } | ||
| sourceProperty = sourceField.GetName() |
There was a problem hiding this comment.
What happens if sourceField is still the zero value here ( nil I am guessing )?
Inother words, if the inner most if above isn't hit and the assignment doesn't happen: sourceField = childField.(*parser.Field)
There was a problem hiding this comment.
It should be impossible for that to happen in the code you commented on (but is non-obvious to readers), I made another change here that removes this line though
AndrewSisley
force-pushed
the
sisley/feat/I149-sum-of-sums
branch
from
2b9da2d
to
ced4c83
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
shahzadlone
approved these changes
Apr 20, 2022
jsimnz
reviewed
Apr 20, 2022
| } | ||
|
|
||
| // Returns true if the value to be summed is a float, otherwise false. | ||
| func (p *Planner) isValueFloat( |
There was a problem hiding this comment.
I don't think this func needs to be a method on the planner object. From what I can tell, the only reason it is, is because the getSourceField is on the planner.
But getSourceField also probably doesn't need to be on planner, which calls getSourceProperty which doens't call anything from the planner object.
So I suggest, isValueFloat, getSourceField, and getSourceProperty all get lifted off the planner.
jsimnz
previously requested changes
Apr 20, 2022
Comment on lines
+181
to
+193
| sourceProperty := source[1] | ||
| if _, isAggregate := parser.Aggregates[sourceProperty]; isAggregate { | ||
| for _, field := range parent.GetSelections() { | ||
| if field.GetName() == source[0] { | ||
| for _, childField := range field.(*parser.Select).Fields { | ||
| if childField.GetAlias() == sourceProperty { | ||
| return childField.(*parser.Field).GetName() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Lets add just a breif description to this (beyond the method docstring) since this is 5 neested deep, to the dev who doesn't know what this does on first read, we should prob just give them a helping hand :)
There was a problem hiding this comment.
What would that comment say that is not already in the the func comment or obvious from the code? Sure it is 5 deep, but is quite simple and contains only 2 code paths (especially considering that source will be renamed/restructured in the very near future
AndrewSisley
force-pushed
the
sisley/feat/I149-sum-of-sums
branch
from
4468f07
to
2a41039
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
dismissed
jsimnz’s stale review
Approval given over discord I think (I hope this button doesnt remove his comments)
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Remove previously done todo * Refactor source collection var * Refactor source property * Remove strange variable aliasing * Refactor isFloat property * Add summation of aggregates support * Remove else clause (PR request)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #149
Adds support for the summation of aggregates. Also tested manually to check it play nicely with the Altair client.