Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Jan 31, 2022
Merged

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

merged 2 commits into from
Jan 31, 2022

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Jan 25, 2022 •
edited
Loading

Closes #136

Test code changes tested by hacking in fake errors and tweaking tests to check for non-existant ones.

Todo:

@AndrewSisley AndrewSisley added feature New feature or request area/query Related to the query component labels Jan 25, 2022
@AndrewSisley AndrewSisley self-assigned this Jan 25, 2022
This was referenced Jan 25, 2022
Merged
Closed
jsimnz
jsimnz requested changes Jan 27, 2022
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

query/graphql/planner/count.go Outdated Show resolved Hide resolved
@AndrewSisley AndrewSisley requested a review from jsimnz January 27, 2022 13:24
@AndrewSisley AndrewSisley force-pushed the sisley/feature/I136-error-if-aggs-dont-spec-field branch from 26c6861 to 7e518f9 Compare January 27, 2022 13:31
jsimnz
jsimnz reviewed Jan 30, 2022
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a quick comment

@AndrewSisley AndrewSisley force-pushed the sisley/feature/I136-error-if-aggs-dont-spec-field branch from 7e518f9 to ed23a30 Compare January 30, 2022 23:36
@codecov
Copy link

codecov bot commented Jan 30, 2022 •
edited
Loading

Codecov Report

Merging #141 (5123b2b) into develop (8c4cc7e) will decrease coverage by 0.13%.
The diff coverage is 40.84%.

❗ Current head 5123b2b differs from pull request most recent head be92c86. Consider uploading reports for the commit be92c86 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #141      +/-   ##
===========================================
- Coverage    64.17%   64.03%   -0.14%     
===========================================
  Files           81       81              
  Lines         7547     7585      +38     
===========================================
+ Hits          4843     4857      +14     
- Misses        2208     2228      +20     
- Partials       496      500       +4
Impacted Files Coverage Δ
query/graphql/planner/count.go 81.57% <0.00%> (ø)
query/graphql/planner/sum.go 72.27% <0.00%> (-0.28%) ⬇️
db/tests/utils.go 55.15% <30.18%> (-15.49%) ⬇️
query/graphql/parser/query.go 74.68% <81.25%> (ø)
query/graphql/planner/select.go 78.48% <0.00%> (+1.19%) ⬆️
query/graphql/planner/planner.go 75.60% <0.00%> (+2.39%) ⬆️
... and 1 more

@AndrewSisley AndrewSisley requested a review from jsimnz January 30, 2022 23:46
@AndrewSisley AndrewSisley force-pushed the sisley/feature/I136-error-if-aggs-dont-spec-field branch from 5123b2b to be92c86 Compare January 30, 2022 23:48
jsimnz
jsimnz approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@AndrewSisley AndrewSisley merged commit 504f84f into develop Jan 31, 2022
@AndrewSisley AndrewSisley deleted the sisley/feature/I136-error-if-aggs-dont-spec-field branch January 31, 2022 21:57
jsimnz pushed a commit that referenced this pull request Feb 7, 2022
@AndrewSisley @jsimnz
feature: Error if aggregates dont specify which field to aggregate (#141
e8ba397 
)

* Add support for expecting errors in integration tests

* Add early check for no aggregate property target
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
@AndrewSisley
feature: Error if aggregates dont specify which field to aggregate (s…
3aeb307 
…ourcenetwork#141)

* Add support for expecting errors in integration tests

* Add early check for no aggregate property target
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsimnz jsimnz jsimnz approved these changes

@shahzadlone shahzadlone Awaiting requested review from shahzadlone

Assignees

@AndrewSisley AndrewSisley

Labels
area/query Related to the query component feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return error for aggregates that have not specified a field
2 participants
@AndrewSisley @jsimnz