Allow to use a string as a delimiter in StringAppendOperator #8536

east825 · 2021-07-15T10:51:44Z

An arbitrary string can be used as a delimiter in StringAppend merge operator
flavor. In particular, it allows using an empty string, combining binary values for
the same key byte-to-byte one next to another.

mrambacher

I generally agree with these changes (and made many of the same ones in #8481).

My one concern is in the stringappend_test. The way it is set up now it does not look as if the old 'char' interface is ever invoked, which would reduce the test coverage. Is it possible to somehow restructure the tests so that both the new and old constructors can be exercised?

east825 · 2021-07-15T16:23:17Z

Oh, wow, I should have submitted my PR earlier :)

I agree, not invoking the old constructor in tests is my oversight. I will try to get covered again.

east825 · 2021-07-27T09:36:30Z

For the history, I brought up this idea in the Google group explaining why we need this at JetBrains specifically (currently we maintain our own fork only to be able to use an empty delimiter on merge). Also, there is an older PR addressing the same limitation, but it seems abandoned for a while.

mrambacher · 2021-07-27T14:33:27Z

@east825 If you wish for your PR to land before mine, I will work on getting it merged once you add the missing tests. I can update my change to support this PR.

Alternatively, your PR will have to merge with mine to add the Java APIs/tests.

east825 · 2021-07-28T10:25:06Z

@mrambacher Thanks a lot. I'll add these tests today. It indeed might be a bit cumbersome for me to merge after you since I'm not that experienced with the project's internals, but on the whole, I'll be happy with whoever changes adding this functionality to the API.

zhichao-cao · 2021-07-28T14:42:56Z

Thanks for the contribution! It is a very useful extension to the operator. Please follow Mark's comment. I'm reviewing his 8481 PR and it would better this PR can be landed before his. Thanks!

zhichao-cao · 2021-07-28T14:45:18Z

@adamretter Can you help to take a look at the Java changes of this PR?

zhichao-cao · 2021-07-28T14:46:54Z

utilities/merge_operators/string_append/stringappend2.cc

@@ -51,14 +53,14 @@ bool StringAppendTESTOperator::FullMergeV2(
    printDelim = true;
  } else if (numBytes) {
    merge_out->new_value.reserve(
-        numBytes - 1);  // Minus 1 since we have one less delimiter
+        numBytes - delim_.size());  // Adjust for one less delimiter


Need a simple explanation for why we minus delim_.size()

We want to reserve the space for the original value and N following operands with preceding delimiters. The size of "N following operands with preceding delimiters" is stored as numBytes. However, if there is no original value, the very first delimiter becomes redundant, therefore we reduce the amount of space needed by the size of one delimiter.

For instance, if the original value was 'foo' and operands 'bar', 'baz' with delimiter ',', we would need 3 + numBytes = 3 + 2 * (3 + 1) = 11 bytes, however if there is no original value, we need only 0 + numBytes - 1 = 7 bytes.
This adjustment just takes into account that a delimiter can now be of an arbitrary length, not only one byte.

Thanks! Can you add these as the comment to the code?

zhichao-cao · 2021-07-28T14:48:22Z

utilities/merge_operators.h

@@ -20,6 +20,8 @@ class MergeOperators {
  static std::shared_ptr<MergeOperator> CreateUInt64AddOperator();
  static std::shared_ptr<MergeOperator> CreateStringAppendOperator();
  static std::shared_ptr<MergeOperator> CreateStringAppendOperator(char delim_char);
+  static std::shared_ptr<MergeOperator> CreateStringAppendOperator(


Since this is a behavior change of StringAppend merge operator, also mention this change in HISTORY.md.

@zhichao-cao This header is not in the public API. Should it still be mentioned in HISTORY or not?

I suggest to mention it in the HISTORY.md, since this merge operator is used by many users and they should be area of this behavior change. So mention it in the HISTORY.md would be better.

I removed the mention of this specific header from HISTORY.md, leaving just StringAppendOperator itself.

facebook-github-bot · 2021-07-28T14:49:24Z

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

mrambacher · 2021-07-29T13:29:25Z

utilities/merge_operators.h

@@ -20,6 +20,8 @@ class MergeOperators {
  static std::shared_ptr<MergeOperator> CreateUInt64AddOperator();
  static std::shared_ptr<MergeOperator> CreateStringAppendOperator();
  static std::shared_ptr<MergeOperator> CreateStringAppendOperator(char delim_char);
+  static std::shared_ptr<MergeOperator> CreateStringAppendOperator(


@zhichao-cao This header is not in the public API. Should it still be mentioned in HISTORY or not?

utilities/merge_operators/string_append/stringappend.h

facebook-github-bot · 2021-07-29T14:24:00Z

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

facebook-github-bot · 2021-07-29T14:29:59Z

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

east825 · 2021-07-29T14:39:00Z

@mrambacher

I generally agree with these changes (and made many of the same ones in #8481).

My one concern is in the stringappend_test. The way it is set up now it does not look as if the old 'char' interface is ever invoked, which would reduce the test coverage. Is it possible to somehow restructure the tests so that both the new and old constructors can be exercised?

I went for the simplest solution, switching between the two constructors depending on the length of a delimiter specified in a test. I don't want to add another layer of indirection on top of StringAppendOperatorTest::OpenFuncPtr to accommodate two possible signatures. It will probably be difficult to wrap one's head around.

mrambacher

LGTM. Thanks for the fix! Can you rebase/update so the fix can be accepted?

An arbitrary string can be used as a delimiter in "StringAppend" merge operator flavor. In particular, it allows to use an empty string, effectively combining binary values for the same key byte-to-byte one next to another.

Expecting the upcoming changes turning the operator into a customizable entity.

facebook-github-bot · 2021-07-29T14:59:22Z

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

east825 · 2021-07-29T15:03:14Z

I can squash everything into one changeset on my side if you want to. In some projects, it's done automatically, not sure which workflow is preferred here.

mrambacher · 2021-07-29T15:04:19Z

I can squash everything into one changeset on my side if you want to. In some projects, it's done automatically, not sure which workflow is preferred here.

No need to squash. As long as you are up-to-date with master and there are no conflicts for the merge, this should be good to go.

facebook-github-bot · 2021-07-29T19:26:08Z

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

facebook-github-bot · 2021-07-29T19:27:31Z

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

facebook-github-bot · 2021-07-29T19:27:39Z

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

facebook-github-bot · 2021-07-29T19:27:52Z

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

HISTORY.md

zhichao-cao · 2021-07-29T19:46:58Z

utilities/merge_operators/string_append/stringappend_test.cc

@@ -259,7 +271,7 @@ TEST_P(StringAppendOperatorTest, SimpleTest) {
 }

 TEST_P(StringAppendOperatorTest, SimpleDelimiterTest) {
-  auto db = OpenDb('|');


For the test, can you keep all the original tests and add the new test that uses the string? The old interface is still in use and we do not want to remove the tests until the interface is deprecated. Also, keep the std::shared_ptr OpenTtlDb(char delim_char).

But the old interface is still used if the specified string is of length 1, addressing the problem @mrambacher pointed at.

It's a bit tricky to write completely independent tests using the new interface there since they are all parameterized and use two different database opening routines depending on the parameter in their SetUp. I believe it will complicate the setup step too much.

… the note

facebook-github-bot · 2021-07-30T10:45:55Z

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

facebook-github-bot · 2021-08-02T22:07:13Z

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

facebook-github-bot · 2021-08-02T23:50:52Z

@zhichao-cao merged this pull request in 8f52972.

facebook-github-bot added the CLA Signed label Jul 15, 2021

mrambacher reviewed Jul 15, 2021

View reviewed changes

mrambacher mentioned this pull request Jul 27, 2021

Make MergeOperator+CompactionFilter/Factory into Customizable Classes #8481

Closed

zhichao-cao reviewed Jul 28, 2021

View reviewed changes

mrambacher reviewed Jul 29, 2021

View reviewed changes

mrambacher approved these changes Jul 29, 2021

View reviewed changes

east825 and others added 4 commits July 29, 2021 17:40

Allow to use a string as a delimiter in StringAppendOperator

e1e3972

An arbitrary string can be used as a delimiter in "StringAppend" merge operator flavor. In particular, it allows to use an empty string, effectively combining binary values for the same key byte-to-byte one next to another.

Add a note about the API change in HISTORY.md

0b78765

Use both versions of StringAppend constructor in tests

5476ff4

Remove "const" modifier of StringAppendOperator.delim_

7783b51

Expecting the upcoming changes turning the operator into a customizable entity.

east825 force-pushed the east825/StringAppendOperator-accepts-strings-as-delimiters branch from 4fd7589 to 7783b51 Compare July 29, 2021 14:59

zhichao-cao reviewed Jul 29, 2021

View reviewed changes

HISTORY.md Outdated Show resolved Hide resolved

zhichao-cao reviewed Jul 29, 2021

View reviewed changes

east825 added 2 commits July 30, 2021 13:42

Replace "Public API change" section with "Behavior Changes", simplify…

e8edf24

… the note

Add a more thorough comment about the adjustment of reserved space

1fbb144

facebook-github-bot closed this in 8f52972 Aug 2, 2021

facebook-github-bot added the Merged label Aug 2, 2021

mrambacher mentioned this pull request Feb 18, 2022

Extend StringAppendTESTOperator to use string delimiter of variable length #4806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to use a string as a delimiter in StringAppendOperator #8536

Allow to use a string as a delimiter in StringAppendOperator #8536

east825 commented Jul 15, 2021

mrambacher left a comment

east825 commented Jul 15, 2021

east825 commented Jul 27, 2021

mrambacher commented Jul 27, 2021

east825 commented Jul 28, 2021

zhichao-cao commented Jul 28, 2021

zhichao-cao commented Jul 28, 2021

zhichao-cao Jul 28, 2021

east825 Jul 29, 2021

zhichao-cao Jul 29, 2021

zhichao-cao Jul 28, 2021

mrambacher Jul 29, 2021

zhichao-cao Jul 29, 2021

east825 Jul 30, 2021

facebook-github-bot commented Jul 28, 2021

mrambacher Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

east825 commented Jul 29, 2021

mrambacher left a comment •

edited

Loading

facebook-github-bot commented Jul 29, 2021

east825 commented Jul 29, 2021

mrambacher commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

zhichao-cao Jul 29, 2021

east825 Jul 30, 2021

facebook-github-bot commented Jul 30, 2021

facebook-github-bot commented Aug 2, 2021

facebook-github-bot commented Aug 2, 2021

Allow to use a string as a delimiter in StringAppendOperator #8536

Allow to use a string as a delimiter in StringAppendOperator #8536

Conversation

east825 commented Jul 15, 2021

mrambacher left a comment

Choose a reason for hiding this comment

east825 commented Jul 15, 2021

east825 commented Jul 27, 2021

mrambacher commented Jul 27, 2021

east825 commented Jul 28, 2021

zhichao-cao commented Jul 28, 2021

zhichao-cao commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 28, 2021

Choose a reason for hiding this comment

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

east825 commented Jul 29, 2021

mrambacher left a comment • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jul 29, 2021

east825 commented Jul 29, 2021

mrambacher commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

facebook-github-bot commented Jul 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 30, 2021

facebook-github-bot commented Aug 2, 2021

facebook-github-bot commented Aug 2, 2021

mrambacher left a comment •

edited

Loading