store: fix deduplication in proxy heap for series with multiple chunks #6663
Conversation
MichaHoffmann
force-pushed
the
mhoffm-fix-dedup-heap-multiple-chunks
branch
4 times, most recently
from
fd27b6b to
7d324ef
Compare
MichaHoffmann
force-pushed
the
mhoffm-fix-dedup-heap-multiple-chunks
branch
from
afc144f to
4eda600
Compare
|
The e2e tests work for me locally 🤔 |
| } | ||
| _, _ = hasher.Write(buf) | ||
| } | ||
| aggrChunkHash := hasher.Sum64() |
There was a problem hiding this comment.
Do I get it right, that this actually calculates a hash of hashes of all the fields?
I wonder how it differs from calculating the hash of all the fields data in terms of possible hash collisions...
There was a problem hiding this comment.
Its the hash of field hashes ( mainly to reuse the field.Hash ).
There was a problem hiding this comment.
The fix itself LGTM but now that we know about this bug, the Hash field in *storepb.Chunk becomes kind of useless. Perhaps we could add a Hash field to storepb.AggrChunk in a separate PR before merging this? That hash field would encompass of the different fields. It's needed so that the querier wouldn't have to hash everything again 😄
Storing hash in AggrChunk sounds correct! for backwards compatibility we need to do it in the querier for a bit right? I worry about the failing e2e test in CI that pass for me locally. Do you have an idea why that could be ? |
I'll try to take a look but just adding a new |
Signed-off-by: Michael Hoffmann <[email protected]>
MichaHoffmann
force-pushed
the
mhoffm-fix-dedup-heap-multiple-chunks
branch
from
6d6c81d to
4d50e46
Compare
|
Note: e2e tests worked for me previously because i didnt rebuild the docker image. |
4 participants
Changes
Consider the hash of the full chunk in deduplication
Verification
Added unittest case
Supposed to fix #6661