Fix order of the returned results from badger backend. #1939
Conversation
burmanm
requested review from
black-adder,
jpkrohling,
objectiser,
pavolloffay,
tiffon,
vprithvi and
yurishkuro
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1939 +/- ##
==========================================
+ Coverage 98.43% 98.43% +<.01%
==========================================
Files 198 198
Lines 9847 9851 +4
==========================================
+ Hits 9693 9697 +4
Misses 116 116
Partials 38 38
Continue to review full report at Codecov.
|
burmanm
force-pushed
the
badger_fix_1809
branch
from
53ba6d1
to
639d711
Compare
tests to verify this as well as add the ability to request StarTime filtering for only TraceIDs. Signed-off-by: Michael Burman <yak@iki.fi>
|
|
||
| timeMax := model.TimeAsEpochMicroseconds(startTimeMax) | ||
| for it.Seek(startIndex); scanFunction(it, indexKeyValue, timeMax); it.Next() { | ||
| for it.Seek(startIndex); scanFunction(it, indexKeyValue, plan.startTimeMin); it.Next() { |
There was a problem hiding this comment.
Has changed from startTimeMax to Min - is that correct? Is it because of reversing the time order?
There was a problem hiding this comment.
Exactly. We finish the scan when the min timestamp is hit.
| key := make([]byte, len(item.Key())) | ||
| copy(key, item.Key()) | ||
| indexResults = append(indexResults, key) | ||
| traceIDBytes := item.Key()[len(item.Key())-sizeOfTraceID:] |
There was a problem hiding this comment.
Wondering whether it would be better to move knowledge about how to extract traceIDBytes from the key into the Item type - or possibly have a Key type that encapsulates all the necessary knowledge about how the key is encoded? Could be done as a separate PR, but might make it easier to maintain.
There was a problem hiding this comment.
The key is encoded in different way depending on the type of key. For example, primary keys that store the trace have different structure than the indexKeys.
Also, Item/Key are types used internally by badger, so that might be a bit confusing if those names were used. But yes, this could be built as "indexKey" and "traceKey" structs.
| @@ -100,48 +98,34 @@ func TestDecodeErrorReturns(t *testing.T) { | |||
| assert.Error(t, err) | |||
| } | |||
|
|
|||
| func TestSortMergeIdsDuplicateDetection(t *testing.T) { | |||
There was a problem hiding this comment.
Could you just clarify why this test is no longer required?
| err := r.store.View(func(txn *badger.Txn) error { | ||
| opts := badger.DefaultIteratorOptions | ||
| opts.PrefetchValues = false // Don't fetch values since we're only interested in the keys | ||
| opts.Reverse = true |
… the limit in scanTimeRange only scenario Signed-off-by: Michael Burman <yak@iki.fi>
… returned values, whichever is smallest Signed-off-by: Michael Burman <yak@iki.fi>
|
@jpkrohling As for the code that should be removed.. it was removed already? I think you looked at the wrong side of the compare ;) |
What's going on with me... :-) |
Which problem is this PR solving?
Add additional tests to verify that results are returned in correct DESC order as well as add the ability to request StartTime filtering for only TraceIDs, instead of requiring complete Traces.
Fixes #1913
Short description of the changes
Last index scan will now do a reverse scan (badger defaults to ASC) and retain the order when doing the hash-join against previous index scan results. Otherwise the index scans will keep using the sort-merge-join.