Collection Merge Operator

[adamretter]

This is a new merge operator which allows you to store a collection of records within the value of a key/value pair.

A record is just a fixed length byte string. Features include:

1. Options for controlling the collection semantics, supporting both:

   1. a Set like collection of records where each record is unique, and,
   2. a Vector like collection where there may be duplicate records.

2. The Collection Merge Operator also optionally supports ordering, where a Comparator can be provided to ensure the ordering of records.

A simple example of vector like behaviour without ordering would look like:

DB* db;
Options options;
options.create_if_missing = true;

auto collection_merge_operator = std::make_shared<CollectionMergeOperator>(4);
options.merge_operator = collection_merge_operator;

Status status = DB::Open(options, "/tmp/testdb", &db);
assert(status.ok());

Slice key1("key1");

Slice add1(std::string("100020003000).insert(0, 1, CollectionOperator::kAdd));
db->merge(key1, add1);

Slice remove1(std::string("2000).insert(0, 1, CollectionOperator::kRemove));
db->merge(key1, remove1);

assert(db.get(key1) == Slice("10003000");

[facebook-github-bot]

@adamretter has updated the pull request.

[facebook-github-bot]

@adamretter has updated the pull request. View: changes

[facebook-github-bot]

@adamretter has updated the pull request.

[facebook-github-bot]

@adamretter has updated the pull request.

[facebook-github-bot]

@adamretter has updated the pull request.

[facebook-github-bot]

@adamretter has updated the pull request.

[facebook-github-bot]

@adamretter has updated the pull request.

[adamretter]

@yuslepukhin I am getting a compile error here with Visual Studio that I don't get with clang or gcc, I wonder if there is something simple you could point out to me that is Visual Studio specific? The error is:

(ClCompile target) -> 
    c:\projects\rocksdb\utilities\merge_operators\collection\collection_merge_operator.h(83): error C3861: 'toHex': identifier not found [C:\projects\rocksdb\build\rocksdb-shared.vcxproj]
    c:\projects\rocksdb\utilities\merge_operators\collection\collection_merge_operator.h(87): error C2440: 'default argument': cannot convert from 'rocksdb::<lambda_2b3d5b4d148b31b228973f801694c3b2>' to 'const std::function<std::string (const char *,::size_t,::size_t)>' [C:\projects\rocksdb\build\rocksdb-shared.vcxproj]

[yuslepukhin]

I am tied on some production issues so it is going to be awhile before I take a look at it.

[yuslepukhin]

Did not get a change to fetch and compile. However,
const size_t is misleading and does nothing in this case, that is why the error message does not mention constness, so is const char* const should simply be const char*. The same applies to toHex.
My recommendation is to introduce two overloads and avoid default arguments (not just in this case).
The reason toHex() is not found is probably a bug. Is this VS 2017?

[adamretter]

@gfosco @siying I have rebased this to bring it up to date. Could someone take a look please?

[vjnadimpalli]

@adamretter Thanks for the effort and sorry for the delay in reviewing this PR. What is the status on this one? Are you still actively interested in contributing this? The overall idea is fantastic, we have a new API (#5604) to get all the merge operands in DB, I was wondering if that can be used to do what is done in this PR. In the value users could store the operation like you mentioned [OperationType, Operand] and when they get all the merge operands then in user code they code simply iterate over all the operands and do what ever they like which includes sorting. That would avoid a lot of Ser/DeSer operations. Let me know your thoughts.

[adamretter]

@vjnadimpalli I think these are quite different things if I understand you correctly.

Your feature allows to bring all merge operands into user code. Whereas this feature allows you to just call db.merge and have it manage a collection of values for a key for you in an atomic and safe manner without you having to know the specifics. Does that make sense?

[vjnadimpalli]

@adamretter overall the change is awesome. One thing I was curious about in collection_merge_operator.cc was why not use set data structure when set type of semantic is configured by the user. Let's say in full merge the merge_in.operand_list may first be converted to a set so all subsequent operations like add and remove can be O(1) as opposed to doing scans. Maybe it's there and I missed it so just clarifying.
Also maybe rebase so we can get it ready to merge in.

[adamretter]

@vjnadimpalli we could use a set, but I wanted to avoid making copies which would use more memory than necessary. Also the code at the moment is succint as the algorithms work on both set and vector like storage. This could of course be further changed/enhanced in future if needed, but I think it serves quite well as a first version.

I have also rebased as requested.

[adamretter]

@pdillinger Can you take a look at this please?

[adamretter]

@pdillinger @siying could you take a look at this PR please?

[facebook-github-bot]

@pdillinger has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[pdillinger]

Use GetSliceNPHash64(slice) cast to size_t

[pdillinger]

Because there is often compiler tolerance for implicit narrowing conversions, I suggest using size_t with a check on supported record len.

[pdillinger]

TODO: use binary search if ordered?

[pdillinger]

TODO?: consider sharing code with exists()

[pdillinger]

We internally have a test that makes sure that everything in the repo can be compiled together. This means we should avoid populating the top-level rocksdb namespace with generically-named but specific-use symbols, even in test code. Can you put this stuff in a sub-namespace and import that in the test?

[pdillinger]

Do you have a test using size other than DEFAULT_TEST_RECORD_SIZE?

[pdillinger]

Any thoughts on adding a factory function to MergeOperators?

[facebook-github-bot]

@adamretter has updated the pull request. You must reimport the pull request before landing.