-
Notifications
You must be signed in to change notification settings - Fork 6.7k
Add a new int64 add merge operator, also with Java API #14149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add a new int64 add merge operator, also with Java API #14149
Conversation
Add back int64_add operator, unify operator tests Start to re-integrate int64_add merge operator. The core of the implementation (by @adamretter, from PR facebook#6884) and associated tests. “Each additional test binary adds substantial work in linking and substantial space (~200MB) to each build.” Therefore we now build a single merge_operators_test binary which at present links the GTESTs from int64add_test.cc and stringappend_test.cc
Build a Java API for the new Int64AddMergeOperator that looks like all the other merge operators. Add tests. Individual operator tests which are part of merge_operators_test shouldn’t be in TEST_MAIN_SOURCES Add int64 merge operator to the tests which mention the other merge operators.
Include the header that says `extern “C” { … `
Implement encoding and zigzag in Java so that we can test it more thoroughly.
Exporting the int64 merge operator to Java requires support for encoding to make it useful. It also requires that support in order to test it from Java.
Accidentally re-introduced deleted loading code when rebasing. Now removed that again, and added the loading of the new int64add merge operator into the new loading code. Moved the new int64add operator into ROCKSDB_NAMESPACE because that’s where the other merge operators are. Split the class into .h and .cc
In the context in which we use them (values in RocksDB (key,value)-pairs, value lengths are always known, so they do not need a continuation bit and a 7-bit encoding, but can be encoded more efficiently as an 8-bit encoding. This has the added advantage of being consistent with fixed size encodings, which can be seen as having trailing-zeros in the most significant bytes, which are truncated in this encoding.
Fix the test inheritance
$ python3 buckifier/buckify_rocksdb.py
The problem only shows up when ASSERT_STATUS_CHECKED=1, i.e. running changed merge_operators_test under CI
make format doesn’t format this but does complain
Don’t need to turn them into zigzag first. There end up being 2 branches in encoding anyway, but there’s perhaps a little less actual code all told.
Changed format in core C++ 8 bit varint encoding, so the Java shadow implementation (used for testing) needs to work the same way.
stdout or stderr was causing a decoding error. Make the error mode “backslashreplace” instead of the default “strict” so we have a chance of seeing the output.
|
Hi @Chessray! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Supercedes #11202 (@adamretter).
The previous PR was based on @alanpaxton's personal repository. This one is now out of Evolved Binary's own space. @alanpaxton's original description is copied for clarification.