Add a separate range classes for internal usage #12071
Conversation
jowlyzhang
force-pushed
the
user_key_range
branch
from
4f9d816
to
e05ed25
Compare
jowlyzhang
changed the title
jowlyzhang
force-pushed
the
user_key_range
branch
from
e05ed25
to
885d465
Compare
jowlyzhang
force-pushed
the
user_key_range
branch
from
885d465
to
93c7573
Compare
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks for the PR @jowlyzhang! Sorry about the long delay - this fell off my radar somehow.
db/db_impl/db_impl.cc
Outdated
| assert(ranges[i].start == nullptr || start.has_value()); | ||
| assert(ranges[i].limit == nullptr || limit.has_value()); |
There was a problem hiding this comment.
This is really an "if and only if" relation, right? If so, we could extend these assertions to cover the opposite direction too
There was a problem hiding this comment.
Thank you for catching this! This assertion is indeed only partially correct. It would pass too if ranges[i].start is null but start has value. I have updated these assertions.
db/dbformat.h
Outdated
| const Slice* start; | ||
| const Slice* limit; |
There was a problem hiding this comment.
The idea behind UserKeyRangePtr is that each boundary can be optional, right? If so, we could use std::optional<Slice> instead of const Slice*
There was a problem hiding this comment.
That's a good point. Using a pointer type here is really an implicit way of expressing the optional nature of the bound. I have updated their types.
jowlyzhang
force-pushed
the
user_key_range
branch
from
93c7573
to
e868a21
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
Thank you for the thorough review and improvement suggestions! No worries, this is just a refactor PR. I only imported it recently when I figured some other ongoing work can use this. |
jowlyzhang
force-pushed
the
user_key_range
branch
from
e868a21
to
2c3943d
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
jowlyzhang
force-pushed
the
user_key_range
branch
from
2c3943d
to
7f2c75f
Compare
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang has updated the pull request. You must reimport the pull request before landing. |
|
@jowlyzhang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@jowlyzhang merged this pull request in e3e8fbb. |
Introduce some different range classes
UserKeyRangeandUserKeyRangePtrto be used by internal implementation. TheRangeclass is used in both public APIs likeDB::GetApproximateSizes,DB::GetApproximateMemTableStats,DB::GetPropertiesOfTablesInRangeetc and internal implementations likeColumnFamilyData::RangesOverlapWithMemtables,VersionSet::GetPropertiesOfTablesInRange.These APIs have different expectations of what keys this range class contain. Public API users are supposed to populate the range with the user keys without timestamp, in the same way that point lookup and range scan APIs' key input only expect the user key without timestamp. The internal APIs implementation expect a user key whose format is compatible with the user comparator, a.k.a a user key with the timestamp.
This PR contains:
UserKeyRangeUserKeyRangePtrfor internal implementation while leave the existingRangeandRangePtrclass only for public APIs. Internal implementations are updated to use this new class instead.DB::GetPropertiesOfTablesInRangeAPI andDeleteFilesInRangesAPI.Test Plan:
existing tests
Added test for
DB::GetPropertiesOfTablesInRangeandDeleteFilesInRangesAPIs for when user-defined timestamp is enabled.The change in external_file_ingestion_job doesn't have a user-defined timestamp enabled test case coverage, will add one in a follow up PR that adds file ingestion support for UDT.