Move general append utilities into general index module #467
Conversation
jeltsch
requested review from
mheinzel,
recursion-ninja and
wenkokke
as code owners
| -- | Min\/max key-info for pages | ||
| data Append = | ||
| -- | One or more keys are in this page, and their values fit within a single | ||
| -- page. | ||
| AppendSinglePage SerialisedKey SerialisedKey | ||
| -- | There is only one key in this page, and it's value does not fit within | ||
| -- a single page. | ||
| | AppendMultiPage SerialisedKey Word32 -- ^ Number of overflow pages | ||
|
|
||
| instance NFData Append where | ||
| rnf (AppendSinglePage kmin kmax) = rnf kmin `seq` rnf kmax | ||
| rnf (AppendMultiPage k nOverflow) = rnf k `seq` rnf nOverflow | ||
|
|
||
| -- | Append a new page entry to a mutable compact index. | ||
| -- | ||
| -- INVARIANTS: see [construction invariants](#construction-invariants). | ||
| append :: Append -> IndexCompactAcc s -> ST s [Chunk] | ||
| append (AppendSinglePage kmin kmax) ica = toList <$> appendSingle (kmin, kmax) ica | ||
| append (AppendMultiPage k n) ica = appendMulti (k, n) ica | ||
|
|
There was a problem hiding this comment.
Just a small suggestion for PR review ergonomics: when code is moved to a different module and subsequently changed as well, it's hard to see from the diff what the interesting code changes are. My usual approach is to move the code in one commit, and do the interesting changes in a different commit.
Right now we're only talking about a small bit of code, so it's fine and doesn't have to change in this PR, but it might be good to keep in mind for future PRs
There was a problem hiding this comment.
My usual approach is to move the code in one commit, and do the interesting changes in a different commit.
Sounds very reasonable to me. I’ll try to remember this approach and use it in future pull requests of mine. Sadly, Git essentially doesn’t track file movement. Well, its manual page calls it right at the beginning “the stupid content tracker”. 😉
There was a problem hiding this comment.
One scenario where Git is smart enough is when a file is first moved and then its contents are changed. The diff will show that the file is renamed, instead of showing that one file was deleted and one new file was created. And the diff of the renamed file just shows the changed lines, not the unchanged lines. But that's about as smart as it gets in my experience
There was a problem hiding this comment.
Yes, and it’s only based on heuristics: as soon as the old and the new content differ too much, Git will treat the old and the new file as unrelated, which according to my experience can happen even if they are morally the same file and you as a human can easily see the similarities of the old and the new content.
There was a problem hiding this comment.
Hmm, now I wonder whether this mishap might have been because of me not actually moving the file but only moving its contents. Does Git perhaps track inode numbers? 🤔
There was a problem hiding this comment.
I found the answer: Git does not track inode numbers. It detects renames based on content only. See the documentation of the -M option in the git-diff(1) man page.
jorisdral
left a comment
jorisdral
left a comment
There was a problem hiding this comment.
LGTM! Let's squash and then press merge
jeltsch
force-pushed
the
jeltsch/append-separation
branch
from
0bf6c30 to
114c2c9
Compare
jeltsch
force-pushed
the
jeltsch/append-separation
branch
from
114c2c9 to
3dbb4e3
Compare
This pull request moves
Appendandappendout ofDatabase.LSMTree.Internal.IndexCompact. The reason is thatAppendis not specific to the compact index andappendwill not be either once the class of index types is introduced. The target of this move is a new module for indexes in general, situated in theextraspackage, as only tests and benchmarks are usingAppendandappend.