-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Fixes Iterator boundaries #2136
Conversation
d526ce8
to
73cc654
Compare
The start should be inclusive and the end is supposed to be exclusive. If start and end are equal, then only one entry can be returned which need to also be equal to start. Fixes grafana#2124 Those issues were edge cases where the boundaries of a block or iterator would be equal to the start. Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
73cc654
to
ad4f113
Compare
Codecov Report
@@ Coverage Diff @@
## master #2136 +/- ##
==========================================
+ Coverage 61.23% 61.39% +0.16%
==========================================
Files 146 146
Lines 11196 11202 +6
==========================================
+ Hits 6856 6878 +22
+ Misses 3794 3777 -17
- Partials 546 547 +1
|
if maxt > b.mint && b.maxt > mint { | ||
its = append(its, b.iterator(ctx, c.readers, filter)) | ||
if maxt < b.mint || b.maxt < mint { | ||
continue |
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.
Just thinking out loud, clarifying how this addresses #2124.
With the patch: ignore the current block when none of its entries is within the interval [mint, maxt]
. Otherwise append it for further processing. Or, in other words: if the current block has any entry within the interval [mint, maxt]
then append it for further processing.
The special case discussed in #2124 corresponds to b.maxt == mint
in this code section here, and with the old behavior maxt > b.mint && b.maxt > mint
a block containing an entry within the interval [mint, maxt]
was erroneously ignored for further processing.
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.
one bit I think could be a bit simpler, but LGTM
|
||
if ok && (i.maxt.Before(ts) || i.maxt.Equal(ts)) { // The maxt is exclusive. | ||
ok = false | ||
if ok { |
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.
couldn't this more simply be expressed as
if i.maxt.Before(ts) || i.maxt.Equal(ts) {
ok = false
}
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.
No this is a very specific path covering the case where both maxt == mint == ts.
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 shortcut is required. I tried it but the test wasn't passing.
The start should be inclusive and the end is supposed to be exclusive.
If start and end are equal, then only one entry can be returned which needs to also be equal to start.
Fixes #2124
Those issues were edge cases where the boundaries of a block or iterator would be equal to the start.
Signed-off-by: Cyril Tovena cyril.tovena@gmail.com