ReadOptions.iter_start_ts should support tombstones #7178
Conversation
|
InternalKey hasn't been exposed so far. ParseInternalKey() and ParsedInternalKey are not available for application to consume. |
The C++ types |
|
I guess any application that is taking advantage of iter_start_seqnum_ is also assuming knowledge of internal key format. Not sure if it was intentional, but would be nice if support exists in the interface without internal structure being exposed. |
|
@starsiriusa , actually in include/rocksdb/types.h, we have |
|
Yes ParseFullKey() does the job. |
|
Maybe add a new type for GetEntryType(kTypeDeletionWithTimestamp)? |
SG. Will do tmr. Update: |
riversand963
force-pushed
the
dbiter-return-tombstones
branch
from
7e6b438
to
b519e81
Compare
There was a problem hiding this comment.
Thanks for the PR @riversand963 ! LGTM, just a couple of minor comments.
| for (it->Seek(Key1(0)), key = 0; it->Valid(); it->Next(), ++count, ++key) { | ||
| CheckIterEntry(it.get(), Key1(key), kTypeDeletionWithTimestamp, Slice(), | ||
| write_ts); | ||
| it->Next(); |
There was a problem hiding this comment.
The purpose of this is to skip the Put with timestamp 3, right? If so, I think a comment saying so would be useful here.
db/db_with_timestamp_basic_test.cc
Outdated
| CheckIterEntry(iter.get(), Key1(key), expected_seq, kTypeValue, | ||
| CheckIterEntry(iter.get(), Key1(key), expected_seq, | ||
| (key % 2) ? kTypeValue : kTypeDeletionWithTimestamp, | ||
| "value" + std::to_string(i + 1), write_ts_list[i + 1]); |
There was a problem hiding this comment.
Minor: we could avoid passing expected_value if it's a deletion.
There was a problem hiding this comment.
Do you suggest adding an overloaded version of CheckIterEntry that takes one fewer argument?
There was a problem hiding this comment.
That would also work but what I had in mind originally was to avoid creating the value string for deletes (since it's unused by the callee), i.e. something along the lines of (key % 2) ? ("value" + std::to_string(i + 1)) : std::string().
There was a problem hiding this comment.
SG. Will update. Thanks!
db/db_with_timestamp_basic_test.cc
Outdated
| for (size_t i = 0; i < read_ts_list.size(); ++i) { | ||
| Slice read_ts = read_ts_list[i]; | ||
| Slice read_ts_lb = read_ts_lb_list[i]; | ||
| (void)read_ts_lb; |
There was a problem hiding this comment.
Nit: probably left over/unnecessary line?
There was a problem hiding this comment.
Sorry about this. Will remove.
Summary: as title. When ReadOptions.iter_start_ts is not nullptr, DBIter::key() should return internal keys including value type. Test Plan: make check
riversand963
force-pushed
the
dbiter-return-tombstones
branch
from
b519e81
to
d3991a6
Compare
There was a problem hiding this comment.
@riversand963 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@riversand963 merged this pull request in 2735b02. |
Summary: as title. When ReadOptions.iter_start_ts is not nullptr, DBIter::key() should return internal keys including value type. Pull Request resolved: facebook#7178 Test Plan: make check Reviewed By: ltamasi Differential Revision: D22935879 Pulled By: riversand963 fbshipit-source-id: 7508d962cf11ebcfa6386d2529b4f3606b47ccfd
Summary: as title.
When ReadOptions.iter_start_ts is not nullptr, DBIter::key() should
return internal keys including value type.
Test Plan:
make check