refactor: Restructure aggregate query syntax #373
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
area/query
AndrewSisley
requested review from
orpheuslummis,
jsimnz,
fredcarle and
shahzadlone
Codecov Report
@@ Coverage Diff @@
## develop #373 +/- ##
===========================================
+ Coverage 65.11% 65.26% +0.14%
===========================================
Files 80 80
Lines 9240 9251 +11
===========================================
+ Hits 6017 6038 +21
+ Misses 2607 2595 -12
- Partials 616 618 +2
|
This comment was marked as spam.
This comment was marked as spam.
AndrewSisley
force-pushed
the
sisley/refactor/I364-aggregate-query-restructure
branch
from
8489d17
to
47ad426
Compare
This comment was marked as spam.
This comment was marked as spam.
shahzadlone
previously requested changes
Apr 22, 2022
query/graphql/schema/generate.go
Outdated
| for _, t := range g.typeDefs { | ||
| sumArg := g.genSumBaseArgInputs(t) | ||
| sumBaseArgs[sumArg.Name()] = sumArg | ||
| // This needs to happen for all types before calling genSumFieldConfig |
There was a problem hiding this comment.
By 'this' do you mean the previous chunk of code or the code that follows up until g.genSumFieldConfig(t, sumBaseArgs) ?
Perhaps a line break before (if referring the code that follows) the comment line.
OR a line break after (if referring to the previous chunk of code above) the comment line.
To help make it more obvious?
There was a problem hiding this comment.
Hmmm thought it was common understanding that comments always apply to the code below them, but I will make it clearer
- Make comment clearer
AndrewSisley
force-pushed
the
sisley/refactor/I364-aggregate-query-restructure
branch
from
47ad426
to
fd19c0e
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...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
force-pushed
the
sisley/refactor/I364-aggregate-query-restructure
branch
from
4878f65
to
c818ee1
Compare
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
the type names on the Aggregate types are a concatenation of type, field, "AggregateInputObj", which as you can see in the image results in something like But the rest of the types follow a normal camel case, here we have "user" and "points" both lowercase, should prob be "userPointsSumInputArg". Minor but it bugged me :p |
jsimnz
approved these changes
Apr 26, 2022
| Fields: gql.InputObjectConfigFieldMap{ | ||
| "_": &gql.InputObjectFieldConfig{ | ||
| Type: gql.Int, | ||
| Description: "Placeholder - empty object not permitted, but will have fields shortly", |
There was a problem hiding this comment.
Is this a todo placeholder that should be resolved in this PR, or by "shortly" are you referring to later?
There was a problem hiding this comment.
We could have it so theres no input arguments to _count until theres an actual need to define something here.
There was a problem hiding this comment.
The first will likely be filter, but I'd also want limit and offset available here too at least. _count still needs input, otherwise there is no way to tell which child you want to count :)
| return nil, nil | ||
| } | ||
|
|
||
| fieldsEnum := gql.NewEnum(fieldsEnumCfg) |
There was a problem hiding this comment.
Isn't there already a defined type to handle fieldsEnum?
There was a problem hiding this comment.
Ahh, there is, but it includes other fields like _key, _group, etc. Youre just constructing a "simple" type fields enum. Can prob do it outside of this func, in its own generator, and call it when we make the original type fields enum, so if they need to be reused its more readily available
There was a problem hiding this comment.
Yeah is specific to here (or at least aggregate). Would like to organise these better and maybe rename them when we get round to refactoring generator, or maybe even when implementing average (which should share types with sum I think)
Really good spot - will have a look at that now (even if they get renamed when looking at average) as if it leaks into the release it looks really sloppy |
In preparation for top-level queries, and aggregate filters
Should be a fair bit safer now and easier to read
AndrewSisley
force-pushed
the
sisley/refactor/I364-aggregate-query-restructure
branch
from
c818ee1
to
3ad3810
Compare
Sorted, will merge after the build |
Benchmark ResultsSummary
✅ See Better Results...
❌ See Worse Results...
✨ See Unchanged Results...
🐋 See Full Results...
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AndrewSisley
deleted the
sisley/refactor/I364-aggregate-query-restructure
branch
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Restructure aggregate query syntax In preparation for top-level queries, and aggregate filters * Rework parser.GetAggregateSource Should be a fair bit safer now and easier to read
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 #365
Restructures aggregate query syntax in preparation for top-level queries, and aggregate filters as discussed on discord.