-
Notifications
You must be signed in to change notification settings - Fork 454
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.
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
[index] Standardizes aggregate interfaces #1502
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1502 +/- ##
========================================
- Coverage 71% 70.9% -0.1%
========================================
Files 849 849
Lines 72250 72290 +40
========================================
+ Hits 51311 51318 +7
- Misses 17581 17616 +35
+ Partials 3358 3356 -2
Continue to review full report at Codecov.
|
if err != nil { | ||
return err | ||
for _, doc := range batch { | ||
if r.aggregateOpts.Type == AggregateTagNamesAndValues { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe switch here and return if unknown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call 👍
src/dbnode/storage/index/types.go
Outdated
|
||
// LimitExceeded returns whether a given size exceeds the limit | ||
// the query options imposes, if it is enabled. | ||
func (o AggregationOptions) LimitExceeded(size int) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the embedding should give this to you fo free iirc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nice 👍
@@ -186,6 +209,9 @@ type AggregateResultsOptions struct { | |||
|
|||
// Optional param to filter aggregate values. | |||
TermFilter AggregateTermFilter | |||
|
|||
// Type determines what result is required. | |||
Type AggregationType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you delete AggregateResultsOptions now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep it as similar as possible to the regular Query
path which does a similar thing when it breaks out the options relating to the result from those relating to the query parameters here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM save minor nits
This PR fulfills several outstanding tasks for the aggregate endpoint:
This allows the query side PR(#1481) to be updated and landed, and minimizes required end-to-end work on #1483