[dbnode] Optimize index.Aggregate() for FieldQuery #1628
Conversation
prateek
force-pushed
the
prateek/dbnode/issue-1620
branch
from
67887be to
61e5e62
Compare
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
9 similar comments
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
========================================
+ Coverage 71.9% 72% +<.1%
========================================
Files 954 954
Lines 79177 79188 +11
========================================
+ Hits 56993 57038 +45
+ Misses 18467 18431 -36
- Partials 3717 3719 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1628 +/- ##
======================================
Coverage 71.9% 71.9%
======================================
Files 954 954
Lines 79182 79182
======================================
Hits 56995 56995
Misses 18464 18464
Partials 3723 3723
Continue to review full report at Codecov.
|
prateek
force-pushed
the
prateek/dbnode/issue-1620
branch
from
61e5e62 to
ead0039
Compare
src/dbnode/storage/index/block.go
Outdated
| @@ -894,6 +895,10 @@ func (b *block) Aggregate( | |||
| } | |||
|
|
|||
| aggOpts := results.AggregateResultsOptions() | |||
| // ensure we iterate the provided filter fields in order. | |||
| sort.Slice(aggOpts.FieldFilter, func(i, j int) bool { | |||
There was a problem hiding this comment.
Will this cause a data race since block.Aggregate(...) happens in parallel across blocks?
There was a problem hiding this comment.
good catch, thanks.
There was a problem hiding this comment.
addressed by moving the sort up into index.go:Aggregate() before we call into the blocks.
src/dbnode/storage/index.go
Outdated
| field, isField := idx.FieldQuery(query.Query) | ||
| if isField { | ||
| fn = i.execBlockAggregateQueryFn | ||
| aopts.FieldFilter = append(index.AggregateFieldFilter{field}, aopts.FieldFilter...) |
There was a problem hiding this comment.
Hm only question here, is for the default query coming from query engine - don't we set both the FieldFilter and the TagMatcher turns into a FieldQuery?
If so, would we add the same field twice to the field filter? Would that mean iterating that field twice instead of once? After reading the field iterator code it wasn't clear, also I'd need to trace the query engine code again, but I though it both sent a tag matcher that is a FieldQuery and might also set the FieldFilter too.
There was a problem hiding this comment.
Hm, seems like it does (from tag_values.go):
return &storage.CompleteTagsQuery{
// NB: necessarily spans the entire timerange for the index.
Start: time.Time{},
End: h.nowFn(),
CompleteNameOnly: false,
FilterNameTags: [][]byte{nameBytes},
TagMatchers: models.Matchers{
models.Matcher{
Type: models.MatchRegexp,
Name: nameBytes,
Value: matchValues,
},
},
}, nilThere was a problem hiding this comment.
You're right - it would have been a duplicate iteration. Wouldn't have affected correctness but it's redundant.
Made the change to only add the field filter if it doesn't exist in the list already, and further to de-dupe the filters to avoid this if users do something silly too.
|
Awesome, thanks this is great! Let me know what you think about my comments, not 100% sure they are all correct hah but did the best I could to think through it. |
|
@robskillington ta, cheers! Addressed feedback. |
prateek
force-pushed
the
prateek/dbnode/issue-1620
branch
from
e331ae0 to
8927592
Compare
Resolves #1620