Fix badger merge-join algorithm to correctly filter indexes #1721
Conversation
burmanm
requested review from
black-adder,
jpkrohling,
objectiser,
pavolloffay,
tiffon,
vprithvi and
yurishkuro
as code owners
burmanm
force-pushed
the
badger_merge_fix
branch
from
2a3e3d1
to
baff4cb
Compare
…acing#1719 Signed-off-by: Michael Burman <yak@iki.fi>
Codecov Report
@@ Coverage Diff @@
## master #1721 +/- ##
==========================================
+ Coverage 98.36% 98.36% +<.01%
==========================================
Files 193 193
Lines 9358 9361 +3
==========================================
+ Hits 9205 9208 +3
Misses 119 119
Partials 34 34
Continue to review full report at Codecov.
|
pavolloffay
changed the title
| @@ -346,53 +345,60 @@ func (r *TraceReader) durationQueries(query *spanstore.TraceQueryParameters, ids | |||
| return ids | |||
| } | |||
|
|
|||
| func mergeJoinIds(left, right [][]byte) [][]byte { | |||
There was a problem hiding this comment.
Are the ids sorted? Maybe that should be documented somewhere.
There was a problem hiding this comment.
It's mentioned at the beginning of the package. Everything is sorted (it's a sorted K/V).
There was a problem hiding this comment.
As for the name, it's because the algorithm is called "sort-merge join" and is used in relational databases. Here the sorting phase happens in the DB and the merge phase in this code. It's pretty descriptive in my opinion since if someone wants to improve this method such as doing it parallel or using sharding from multiple badgers there are known algorithms for those variations too (which would underneath use this in any case).
There was a problem hiding this comment.
Thanks for the explanation. Now it rings a bell..
| @@ -200,6 +205,7 @@ func TestIndexSeeks(t *testing.T) { | |||
| params.OperationName = "operation-1" | |||
| tags := make(map[string]string) | |||
| tags["k11"] = "val0" | |||
| tags["error"] = "true" | |||
There was a problem hiding this comment.
Curious why this was added, as doesn't seem related to the PR?
There was a problem hiding this comment.
Devil is in the details. That single line exploits the bug (the test fails with older version) since it adds another index query against the tags.
There was a problem hiding this comment.
As for the id list, it is basically the list of matches for the search query. A form of a posting list (of traceIDs) if thinking in terms of the ES.
In terms of relational database, it's equivalent to something like: SELECT id FROM dbo.spans WHERE service = 'invoices'
That is, a single id list is equivalent to that one. Just imagine each id list is one similar query, touching a single index and single value. It doesn't matter if the index is the same or not (so one query could be against service, one against tags index etc).
Signed-off-by: Michael Burman <yak@iki.fi>
Which problem is this PR solving?
Resolves #1719, the index seeks were not correctly merged and filtered.
Short description of the changes
Make the merge-join correctly update two indices when encountering equal items. Also, the input must be the output of previous merge. Also, changed ASC to DESC reversing to happen after the top query filtering - thus reducing unnecessary work.