binaryHeader: fixed "invalid size" error with non existing label value #2386
Conversation
mkabischev
force-pushed
the
fix-posting-offset-search
branch
from
b58dc57 to
f6293cc
Compare
| break | ||
| } | ||
| if i > 0 && e.offsets[i].value != value { |
There was a problem hiding this comment.
Why? Really I didn't get this logic.
if e.offsets[i].value != value than means that we didn't find value and have to do nothing.
There was a problem hiding this comment.
Looks like This is still not clear, I think what would help is some extra unit test to verify
There was a problem hiding this comment.
Why do we need to decrement it? I mean that when there is no value found then we need break.
There was a problem hiding this comment.
The whole idea behind this code is to find matching offset OR the largest offset that is before wanted value. Then we iterate one by one to find exact offset.
This is because binary index hold in memory only 1/32 of offsets (to not keep all in memory). Problem is that our tests have only 3 postings most likely that's why we test nothing..
It's extremely concerning that our tests passed given that this version will literally skip 31/32 postings ((:
Signed-off-by: Mike Kabischev <[email protected]>
mkabischev
force-pushed
the
fix-posting-offset-search
branch
from
7a2d072 to
38918f7
Compare
There was a problem hiding this comment.
Thanks for this but I think you missed important information that we are this offsets hold only 1/32 of posting offsets referenced by name = value. This means that if e.offsets[i].value != value we really need to iterate over 32 offsets from the actual mapped file. This is super scary that we cannot both:
- Reproduce your issue in unit test
- Fail unit test on such clear broken logic )))):
We need a test, let's focus on this tomorrow. The bug must be somewhere here, but this change is not fixing it yet. Let's dig dipper tomorrow! 🤗
| break | ||
| } | ||
| if i > 0 && e.offsets[i].value != value { |
There was a problem hiding this comment.
The whole idea behind this code is to find matching offset OR the largest offset that is before wanted value. Then we iterate one by one to find exact offset.
This is because binary index hold in memory only 1/32 of offsets (to not keep all in memory). Problem is that our tests have only 3 postings most likely that's why we test nothing..
It's extremely concerning that our tests passed given that this version will literally skip 31/32 postings ((:
|
We found the root cause of error in another place, so close this PR. Fix will be in another one. |
Fixes #2213
There is a problem in
sort.Searchusage that causes floating error when try to get offsets for non existing combination of name & values.