Make ordinary indexes store their keys unsliced #659
Conversation
jeltsch
requested review from
mheinzel,
recursion-ninja and
wenkokke
as code owners
| = do | ||
| #ifdef NO_IGNORE_ASSERTS | ||
| maybeLastLastKey <- Growing.readMaybeLast lastKeys | ||
| assert (all (< key) maybeLastLastKey) $ return () | ||
| maybeLastUnslicedLastKey <- Growing.readMaybeLast unslicedLastKeys | ||
| assert (all (< key) (fromUnslicedKey <$> maybeLastUnslicedLastKey)) | ||
| (return ()) | ||
| #endif | ||
| Growing.append lastKeys pageCount key | ||
| Growing.append unslicedLastKeys pageCount (makeUnslicedKey key) |
There was a problem hiding this comment.
Something along these lines would fix the compiler error with assertions disabled
= do
let !key' = makeUnslicedKey key
#ifdef NO_IGNORE_ASSERTS
maybeLastUnslicedLastKey <- Growing.readMaybeLast unslicedLastKeys
assert (all (< key') maybeLastUnslicedLastKey)
(return ())
#endif
Growing.append unslicedLastKeys pageCount key'There was a problem hiding this comment.
What compiler error do you get here? I can’t see an error in this code, and commenting out the #ifdef–#endif part doesn’t cause GHC to show me an error message.
There was a problem hiding this comment.
Ah, I see: When removing both assertion blocks, there is an unused import. I had a similar issue in the same module with readMaybeLast, which I solved by a conditional import. I can just move the import of fromUnslicedKey into the conditional block with the readMaybeLast import.
There was a problem hiding this comment.
I can just move the import of
fromUnslicedKeyinto the conditional block with thereadMaybeLastimport.
Done in dd6b71c.
| = assert (firstKey <= lastKey) $ | ||
| do | ||
| #ifdef NO_IGNORE_ASSERTS | ||
| maybeLastLastKey <- Growing.readMaybeLast lastKeys | ||
| assert (all (< firstKey) maybeLastLastKey) $ return () | ||
| maybeLastUnslicedLastKey <- Growing.readMaybeLast unslicedLastKeys | ||
| assert | ||
| (all (< firstKey) (fromUnslicedKey <$> maybeLastUnslicedLastKey)) | ||
| (return ()) | ||
| #endif | ||
| Growing.append lastKeys 1 lastKey | ||
| Growing.append unslicedLastKeys 1 (makeUnslicedKey lastKey) |
There was a problem hiding this comment.
See my suggestion for appendMulti
|
Let's squash before we merge |
jeltsch
force-pushed
the
jeltsch/unsliced-keys-in-ordinary-indexes
branch
from
dd6b71c to
84f5ca5
Compare
Previously, the keys stored in ordinary indexes and ordinary-index accumulators were of type
SerialisedKey, and the append operations for ordinary-index accumulators did not truncate keys that were represented as slices of larger byte sequences to their actual payloads. This pull request changesIndexOrdinaryandIndexOrdinaryAccto useUnsliced SerialisedKeyfor storing keys, so that it is guaranteed that only actual key contents are stored.