Add Merge Operator support to WriteBatchWithIndex #8135

mrambacher · 2021-03-30T23:10:20Z

The WBWI has two differing modes of operation dependent on the value
of the constructor parameter overwrite_key.
Currently, regardless of the parameter, neither mode performs as
expected when using Merge. This PR remedies this by correctly invoking
the appropriate Merge Operator before returning results from the WBWI.

Examples of issues that exist which are solved by this PR:

Example 1 with `overwrite_key=false`

Currently, from an empty database, the following sequence:

Put('k1', 'v1')
Merge('k1', 'v2')
Get('k1')

Incorrectly yields v2, that is to say that the Merge behaves like a Put.

Example 2 with o`verwrite_key=true`

Currently, from an empty database, the following sequence:

Put('k1', 'v1')
Merge('k1', 'v2')
Get('k1')

Incorrectly yields ERROR: kMergeInProgress.

Example 3 with `overwrite_key=false`

Currently, with a database containing ('k1' -> 'v1'), the following sequence:

Merge('k1', 'v2')
GetFromBatchAndDB('k1')

Incorrectly yields v1,v2

Example 4 with `overwrite_key=true`

Currently, with a database containing ('k1' -> 'v1'), the following sequence:

Merge('k1', 'v1')
GetFromBatchAndDB('k1')

Incorrectly yields ERROR: kMergeInProgress.

Example 5 with `overwrite_key=false`

Currently, from an empty database, the following sequence:

Put('k1', 'v1')
Merge('k1', 'v2')
GetFromBatchAndDB('k1')

Incorrectly yields v1,v2

Example 6 with `overwrite_key=true`

Currently, from an empty database, ('k1' -> 'v1'), the following sequence:

Put('k1', 'v1')
Merge('k1', 'v2')
GetFromBatchAndDB('k1')

Incorrectly yields ERROR: kMergeInProgress.

pdillinger

Also update HISTORY.md

pdillinger · 2021-04-06T20:14:55Z

include/rocksdb/utilities/write_batch_with_index.h

@@ -176,6 +176,11 @@ class WriteBatchWithIndex : public WriteBatchBase {
 Iterator* NewIteratorWithBase(ColumnFamilyHandle* column_family,
 Iterator* base_iterator,
 const ReadOptions* opts = nullptr);
+ Iterator* NewIteratorWithBase(DB* db, Iterator* base_iterator,


When is the DB parameter needed? (This is public API.)

The DB option is used similarly to the DB option for "GetFromBatchAndBase". Without the DB option, if you see a Merge without a preceding Put/Delete, you cannot get the "initial" value for the key without the DB. If the other signatures are used (without the DB) and a Merge is encountered without a root, then a "MergeInProgress" error is returned.

I can update the comment appropriately when I figure out the wording...

Although I don't know this area well, the rest of the PR LGTM, seems like a clear improvement even if it's not perfect.

Someone questioned whether it's a good idea for WBWI to effectively re-implement all the DB logic (without persistence, etc.). I believe it's potentially useful for finding bugs on both sides if we compare them.

facebook-github-bot · 2021-04-08T19:27:04Z

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

facebook-github-bot · 2021-04-11T20:34:16Z

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

Found a solution that did not require the new API signature. Updated the code accordingly. Updated HISTORY

facebook-github-bot · 2021-04-11T20:56:16Z

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

facebook-github-bot · 2021-04-11T22:19:22Z

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

pdillinger

OK with me!

db/merge_context.h

facebook-github-bot · 2021-04-15T21:25:11Z

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

facebook-github-bot · 2021-04-15T21:30:48Z

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

facebook-github-bot · 2021-04-15T23:06:47Z

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

facebook-github-bot · 2021-04-15T23:47:29Z

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

Summary: Moves some of the structural refactoring from #8135 into this PR. This just cleans up the code by moving implementation out of the .h file and into the .cc file. Should be considered for merge before both #7214 and #8135 Pull Request resolved: #8229 Reviewed By: jay-zhuang Differential Revision: D27999669 Pulled By: mrambacher fbshipit-source-id: 6eccecbf1f11bb9f5a173e86d1e7bc448bc96071

facebook-github-bot · 2021-05-04T18:27:44Z

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

facebook-github-bot · 2021-05-04T18:34:36Z

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

facebook-github-bot · 2021-05-04T18:36:25Z

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

facebook-github-bot · 2021-05-04T19:51:33Z

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

facebook-github-bot · 2021-05-05T01:08:12Z

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

facebook-github-bot · 2021-05-05T23:16:45Z

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

facebook-github-bot · 2021-05-05T23:30:45Z

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

facebook-github-bot · 2021-05-06T01:05:49Z

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

Attempt to improve code coverage...

facebook-github-bot · 2021-05-07T20:03:48Z

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

facebook-github-bot · 2021-05-07T20:52:10Z

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

facebook-github-bot · 2021-05-10T13:19:03Z

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

facebook-github-bot · 2021-05-10T19:50:34Z

@mrambacher merged this pull request in ff46374.

Summary: Remove obsolete comment. Support for WriteBatchWithIndex::NewIteratorWithBase when overwrite_key=false is added in #8135, as you can clearly see in the HISTORY.md. Pull Request resolved: #11636 Reviewed By: jowlyzhang Differential Revision: D47722955 Pulled By: ajkr fbshipit-source-id: 4fa44a309d9708e9f4a1530918a9aaf7114c9032

Summary: Remove obsolete comment. Support for WriteBatchWithIndex::NewIteratorWithBase when overwrite_key=false is added in facebook/rocksdb#8135, as you can clearly see in the HISTORY.md. Pull Request resolved: facebook/rocksdb#11636 Reviewed By: jowlyzhang Differential Revision: D47722955 Pulled By: ajkr fbshipit-source-id: 4fa44a309d9708e9f4a1530918a9aaf7114c9032

mrambacher added 3 commits January 26, 2021 10:48

Make GetFromBatch work for overwrite=true

406e6db

Merge branch 'master' into WBWI

110c8ae

Merge branch 'master' into WBWI

d74970c

mrambacher requested review from adamretter and pdillinger March 30, 2021 23:10

facebook-github-bot added the CLA Signed label Mar 30, 2021

mrambacher requested a review from ajkr March 30, 2021 23:10

pdillinger reviewed Apr 6, 2021

View reviewed changes

Address PR Comments

f2665d1

Found a solution that did not require the new API signature. Updated the code accordingly. Updated HISTORY

mrambacher force-pushed the WBWI branch from ed54deb to f2665d1 Compare April 11, 2021 20:56

pdillinger approved these changes Apr 15, 2021

View reviewed changes

db/merge_context.h Show resolved Hide resolved

Merge branch 'master' into WBWI

8df67f0

Fix lint issues

c4fb939

adamretter mentioned this pull request Apr 24, 2021

WBWI Internal Move implementation from .h into .cpp #8229

Closed

mrambacher added 2 commits May 4, 2021 13:08

Merge branch 'master' into WBWI

425e13d

Fix merge issue

988e589

Address PR Comment

2ce07f8

mrambacher force-pushed the WBWI branch from 3a69a96 to 2ce07f8 Compare May 4, 2021 18:36

Merge branch 'master' into WBWI

f3f96af

Merge branch 'master' into WBWI

2405b05

Add test with failing MergeOperator

5c01084

Attempt to improve code coverage...

facebook-github-bot closed this in ff46374 May 10, 2021

facebook-github-bot added the Merged label May 10, 2021

zhichao-cao mentioned this pull request May 14, 2021

Fix MultiGet with PinnableSlices and Merge for WBWI #8299

Closed

ywave620 mentioned this pull request Jul 23, 2023

Fix comment in WriteBatchWithIndex::NewIteratorWithBase #11636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Merge Operator support to WriteBatchWithIndex #8135

Add Merge Operator support to WriteBatchWithIndex #8135

mrambacher commented Mar 30, 2021

pdillinger left a comment

pdillinger Apr 6, 2021

mrambacher Apr 6, 2021

pdillinger Apr 7, 2021

facebook-github-bot commented Apr 8, 2021

facebook-github-bot commented Apr 11, 2021

facebook-github-bot commented Apr 11, 2021

facebook-github-bot commented Apr 11, 2021

pdillinger left a comment

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 6, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 10, 2021

facebook-github-bot commented May 10, 2021

Add Merge Operator support to WriteBatchWithIndex #8135

Add Merge Operator support to WriteBatchWithIndex #8135

Conversation

mrambacher commented Mar 30, 2021

Example 1 with overwrite_key=false

Example 2 with overwrite_key=true

Example 3 with overwrite_key=false

Example 4 with overwrite_key=true

Example 5 with overwrite_key=false

Example 6 with overwrite_key=true

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Apr 6, 2021

Choose a reason for hiding this comment

mrambacher Apr 6, 2021

Choose a reason for hiding this comment

pdillinger Apr 7, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Apr 8, 2021

facebook-github-bot commented Apr 11, 2021

facebook-github-bot commented Apr 11, 2021

facebook-github-bot commented Apr 11, 2021

pdillinger left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented Apr 15, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 4, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 5, 2021

facebook-github-bot commented May 6, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 7, 2021

facebook-github-bot commented May 10, 2021

facebook-github-bot commented May 10, 2021

Example 1 with `overwrite_key=false`

Example 2 with o`verwrite_key=true`

Example 3 with `overwrite_key=false`

Example 4 with `overwrite_key=true`

Example 5 with `overwrite_key=false`

Example 6 with `overwrite_key=true`