Have indexing by record scan by key ranges instead of by tuple ranges #2655
Conversation
MMcM
force-pushed
the
index-by-record-ranges
branch
from
5a9b5e0 to
954b481
Compare
Result of fdb-record-layer-pr on Linux CentOS 7
|
| final byte[] keyPrefix = store.recordsSubspace().pack(); | ||
| final byte[] rangeStart = ByteArrayUtil.join(keyPrefix, range.begin); | ||
| final byte[] rangeEnd = ByteArrayUtil.join(keyPrefix, range.end); | ||
| final KeyRange keyRange = new KeyRange(rangeStart, rangeEnd); |
There was a problem hiding this comment.
I'm trying to wrap my head around the KeyRange. Looking at TupleRange::toRange (which is still used to translate a KeyRange into an FDB Range), a KeyRange where the low endpoint is RANGE_INCLUSIVE and the end endpoint is always RANGE_EXCLUSIVE will always behave just like a Range over those same two keys. But it also behaves just like a TupleRange with packed elements, so for instance, something like new KeyRange(bytes, EndpointType.RANGE_INCLUSIVE, bytes, EndpointType.RANGE_INCLUSIVE) represents all keys "strictly" prefixed by the range (that is, excluding keys prefixed by bytes + '\xff').
That feels fair enough.
| // tupleRange has an inclusive high endpoint, so end isn't a valid tuple. | ||
| // But buildRangeOnly needs to convert missing Ranges back to TupleRanges, so round up. | ||
| rangeEnd = ByteArrayUtil.strinc(range.end); | ||
| rangeEnd = range.end; |
There was a problem hiding this comment.
Setting up this particular test may be a pain, so it may not be worth it, but I think if we wanted to validate that this fixed anything, we could add a test that used non-integer record type keys. Or a test that used a record type key with what would have been problematic value for its record type key, like 255
| * @return a cursor that will scan everything in the range, picking up at continuation, and honoring the given scan properties | ||
| */ | ||
| @Nonnull | ||
| RecordCursor<FDBStoredRecord<M>> scanRecordsKeyRange(@Nonnull KeyRange range, |
There was a problem hiding this comment.
Do you think this API should be INTERNAL? Most of the use cases I can think of are probably internal, though there may be some use case for APIs that returns a KeyRange that the user can hand back, or something.
| @Nonnull | ||
| @SuppressWarnings("PMD.CloseResource") | ||
| private <M extends Message> RecordCursor<FDBStoredRecord<M>> scanTypedRecordsInternal(@Nonnull RecordSerializer<M> typedSerializer, | ||
| @Nonnull Consumer<KeyValueCursor.Builder> setRange, |
There was a problem hiding this comment.
minor: I was trying to think if there was a better way to structure this than a Consumer. The answer may be "no". The best alternatives I could think of would be to either have this take a KeyValueCursor.Builder than callers would pre-fill with the range already set, or something more exotic like adding a .setRange(KeyValueCursor.Builder) to the TupleRange and KeyValueRange abstractions, or making TupleRange and KeyValueRange implement some common interface that could set the low and high endpoints on a KeyValueCursor.Builder.
This avoids having to recover a
TupleRangefrom an unbuiltRange.See #2651 (comment)