db: refactor Reader.Get #3507
db: refactor Reader.Get #3507
Conversation
|
This change is |
jbowens
force-pushed
the
get-refac
branch
3 times, most recently
from
4742d22 to
888bd84
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @itsbilal and @jbowens)
get_iter.go line 77 at r1 (raw file):
} func (g *getIter) Next() *base.InternalKV {
Do we ever call Next() on this iterator? It seems strange, since we'd only care about the first result.
get_iter.go line 243 at r1 (raw file):
} func (g *getIter) getSSTableIterators(
This could use a comment - we are getting iterators for the one sstable that might contain the key.
get_iter.go line 255 at r1 (raw file):
// the file doesn't actually contain any point keys equal to `key`. We next // to keep searching for a file that actually contains point keys ≥ key. if m.LargestPointKey.IsExclusiveSentinel() && g.comparer.Equal(m.LargestPointKey.UserKey, g.key) {
Not related to this change, but I think we should make SeekGE take a base.UserKeyBoundary
This commit refactors the implementation of getIter to be a little more understandable and avoid the unnecessary use of levelIter. Current supported format major versions guarantee that a user key is not split across sstables within a level. This ensures that Get (which only retrieves one individual user key) need only consult 1 sstable per level. This is somewhat motivated by cockroachdb#2863. Removing getIter's dependency on levelIter will make that refactor easier.
Apply a small simplification to levelIter.skipEmptyFileForward's condition for when to interleave a synthetic boundary. Previously we needed to check whether a file's largest point key was a range deletion to work around getIter's use of the rangeDelIterPtr. Now that getIter no longer uses levelIter it's unnecessary.
There was a problem hiding this comment.
TFTR!
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @RaduBerinde)
get_iter.go line 77 at r1 (raw file):
Previously, RaduBerinde wrote…
Do we ever call
Next()on this iterator? It seems strange, since we'd only care about the first result.
yeah, but I think only in the case the first key is a MERGE. added a comment
get_iter.go line 243 at r1 (raw file):
Previously, RaduBerinde wrote…
This could use a comment - we are getting iterators for the one sstable that might contain the key.
Done.
get_iter.go line 255 at r1 (raw file):
Previously, RaduBerinde wrote…
Not related to this change, but I think we should make
SeekGEtake abase.UserKeyBoundary
yeah, makes sense
db: refactor Reader.Get
This commit refactors the implementation of getIter to be a little more
understandable and avoid the unnecessary use of levelIter. Current supported
format major versions guarantee that a user key is not split across sstables
within a level. This ensures that Get (which only retrieves one individual user
key) need only consult 1 sstable per level.
This is somewhat motivated by #2863. Removing getIter's dependency on levelIter
will make that refactor easier.
db: simplify levelIter skipEmptyFileForward
Apply a small simplification to levelIter.skipEmptyFileForward's condition for
when to interleave a synthetic boundary. Previously we needed to check whether
a file's largest point key was a range deletion to work around getIter's use of
the rangeDelIterPtr. Now that getIter no longer uses levelIter it's
unnecessary.