Use the comparator from the sst file table properties in sst_dump_tool #9491

satyajanga · 2022-02-02T20:47:45Z

Summary:
We introduced a new Comparator for timestamp in user keys. In the sst_dump_tool by default we use BytewiseComparator to read sst files. This change allows us to read comparator_name from table properties in meta data block and use it to read.

Test Plan:
added unittests for new functionality.
make check

riversand963 · 2022-02-02T21:12:21Z

db/compaction/compaction_iterator_test.cc

@@ -1079,7 +1080,7 @@ INSTANTIATE_TEST_CASE_P(CompactionIteratorWithAllowIngestBehindTestInstance,
 class CompactionIteratorTsGcTest : public CompactionIteratorTest {
 public:
  CompactionIteratorTsGcTest()
-      : CompactionIteratorTest(test::ComparatorWithU64Ts()) {}
+      : CompactionIteratorTest(rocksdb::ComparatorWithU64Ts()) {}


Other things aside, we no longer hard-code the namespace name as rocksdb any more.

removed it.

riversand963 · 2022-02-02T21:35:28Z

I think this PR tries to address two issues:

Allow sst_dump to create the right comparator after reading the SST's properties block.
Allow sst_dump to be able to parse and dump the content of SST files with user-defined timestamp enabled.

IMO, we can split this PR into two for 1 and 2 respectively. For 1, we can create an SST file using ReverseBytewiseComparator(), and then we verify your code by using sst_dump to read it.

For 2, we can spend more time thinking about the approach based on 1.

satyajanga · 2022-02-03T06:16:44Z

I think this PR tries to address two issues:

Allow sst_dump to create the right comparator after reading the SST's properties block.

Allow sst_dump to be able to parse and dump the content of SST files with user-defined timestamp enabled.

IMO, we can split this PR into two for 1 and 2 respectively. For 1, we can create an SST file using ReverseBytewiseComparator(), and then we verify your code by using sst_dump to read it.

For 2, we can spend more time thinking about the approach based on 1.

@riversand963 this PR addresses 1 and validates with ReverseBytewiseComparator() using a unittest.
for 2, we need a way to create SST file with user-defined timestamp enabled, so we can add a unittest and validate the behavior. As you mentioned 2 can be handled in a separate PR.

riversand963 · 2022-02-03T17:25:28Z

For 1, we do not have to move the two test comparators from test namespace to ROCKDB_NAMESPACE. The API additions are unnecessary.

riversand963

Thanks @satyajanga for the PR.
I think we can keep the two test comparators in the test namespace in this PR, and implement support for obtaining comparator name from table properties. With unit tests addition, I think we are in the right direction.

riversand963 · 2022-02-03T17:26:39Z

tools/sst_dump_test.cc

@@ -90,7 +90,8 @@ class SSTDumpToolTest : public testing::Test {
    snprintf(usage[2], kOptLength, "--file=%s", file_path.c_str());
  }

-  void createSST(const Options& opts, const std::string& file_name) {
+  void createSST(const Options& opts, const std::string& file_name,
+                 bool reverse = false) {


Instead of passing bool reverse, I think we can pass a comparator name.

sorry I misread the code. Ignore this one.

riversand963 · 2022-02-03T17:30:55Z

tools/sst_dump_test.cc

-    for (uint32_t i = 0; i < num_keys; i++) {
-      tb->Add(MakeKey(i), MakeValue(i));
+    if (reverse) {
+      for (uint32_t i = num_keys; i > 0; i--) {


In this case, 0 is not tested

I avoided it because the condition i>=0 will cause the infinite loop since i is unsigned.
I will change the i type int32_t

table/sst_file_dumper.cc

riversand963 · 2022-02-03T17:39:52Z

table/sst_file_dumper.cc

+                                           table_properties_->comparator_name,
+                                           &user_comparator);
+          if (s.ok()) {
+            internal_comparator_ = InternalKeyComparator(user_comparator, true);


Nit: we can put /*named=*/ before true. Usually, we try to do this to help understand the code without relying on IDE.

Introduce a CountedFileSystem for counting file operations (facebook#9283)

riversand963 · 2022-02-03T23:38:44Z

util/comparator.cc

+
+/* Comparator with 64-bit integer timestamp.
+This is still experimental, use it with caution.
+*/


We use // for multi-line comments

fixed in the latest commit

riversand963 · 2022-02-04T00:24:07Z

util/comparator.cc

+/* Comparator with 64-bit integer timestamp.
+This is still experimental, use it with caution.
+*/
+class ComparatorWithU64TsImpl : public Comparator {


Since this comparator is now in ROCKSDB_NAMESPACE and will be exposed via public API, we need improve it more.
Currently, the constructor hard-codes BytewiseComparator(), which is not necessary. We should be able to wrap an arbitrary comparator (without ts) using this class. When wrapping around a pointer to another comparator, we should consider object lifetime.

Its slightly tricky as I think about it.
If we can pass any arbitrarory comparator, we need a way to store/retrieve it from the sst file properties.
We need two things

timestamp comparator -> we have it

and the comparator without ts -> we dont have it If I understand correctly

Maybe there is some confusion here.
Say we want to support BytewiseComparatorWithU64Ts, ReverseBytewiseComparatorWithU64Ts and FancyComparatorWithU64Ts(). In the object registry, we have the following:

"leveldb.BytewiseComparator" => BytewiseComparator() "rocksdb.ReverseBytewiseComparator" => ReverseBytewiseComparator() "rocksdb.BytewiseComparator.u64ts" => BytewiseComparatorWithU64Ts() (this does not have to be API yet) "rocksdb.ReverseBytewiseComparator.u64ts"=> ReverseBytewiseComparatorWithU64Ts() (does not have to be public API yet) "rocksdb.FancyComparator.u64ts" => FancyComparatorWithU64Ts() // I made this up

BytewiseComparatorWithU64Ts() , ReverseBytewiseComparatorWithU64Ts() and FancyComparatorWithU64Ts() can all use ComparatorWithU64TsImpl class. In each of them, they can determine how they construct ComparatorWithU64TsImpl object.

Updated this based on the internal chat

riversand963 · 2022-02-04T00:25:13Z

include/rocksdb/comparator.h

@@ -150,4 +150,6 @@ extern const Comparator* BytewiseComparator();
 // ordering.
 extern const Comparator* ReverseBytewiseComparator();

+extern const Comparator* ComparatorWithU64Ts();


We should put the comments about its limitation here, and explicitly state it's experimental and subject to change/removal

Also we should think more about the API name and contract.

As commented earlier, we can avoid public API change

facebook-github-bot · 2022-02-04T01:00:55Z

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

riversand963 · 2022-02-04T17:00:46Z

Thinking more about this. I still think we should avoid adding public API if there is a way to do with out it. Adding a new API to expose an unoptimized comparator binding with a specific timestamp implementation seems hacky to me. Maybe we can add it later after longer period of time and broader discussion.
I think it's possible to avoid adding the ComparatorWithU64Ts() or whatever name API for this PR.
We can

Move class ComparatorWithU64TsImpl to comparator.cc in the ROCKSDB_NAMESPACE.
In comparator.cc, register this comparator in the ObjectRegistry/ObjectLibrary. After this is done, I think we can get a pointer to the comparator via CreateFromString(), which can be used to replace all existing test::ComparatorWithU64Ts() calls.

facebook-github-bot · 2022-02-04T21:55:39Z

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

satyajanga · 2022-02-04T22:05:07Z

util/comparator.cc

+ public:
+  ComparatorWithU64TsImpl()
+      : Comparator(/*ts_sz=*/sizeof(uint64_t)),
+        cmp_without_ts_(BytewiseComparator()) {


Need to change constructor to pass a
ComparatorWithU64TsImpl(T ). In the constructor I will cast and assign to Comparator

facebook-github-bot · 2022-02-04T22:22:37Z

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

riversand963

Thanks for the update. Left a few comments.

riversand963 · 2022-02-04T22:09:21Z

HISTORY.md

@@ -30,6 +30,8 @@

 ## New Features
 * Introduced an option `BlockBasedTableBuilder::detect_filter_construct_corruption` for detecting corruption during Bloom Filter (format_version >= 5) and Ribbon Filter construction.
+* Introduced a new experimental comparator for user defined timestamp feature.


This can be removed.

riversand963 · 2022-02-04T22:10:28Z

include/rocksdb/comparator.h

@@ -150,4 +150,6 @@ extern const Comparator* BytewiseComparator();
 // ordering.
 extern const Comparator* ReverseBytewiseComparator();

+extern const Comparator* ComparatorWithU64Ts();


As commented earlier, we can avoid public API change

riversand963 · 2022-02-04T22:23:08Z

util/comparator.cc

+                "T must be a inherited type of comparator");
+
+ public:
+  ComparatorWithU64TsImpl()


Add explicit.

riversand963 · 2022-02-04T22:24:12Z

util/comparator.cc

+  static const char* kClassName() {
+    // Putting this up just for getting opinion,  need to make this better.
+    static char buf[128];
+    sprintf(buf, "%s.u64", T::kClassName());


We discourage the usage of sprint. When you have to, use stringstream, especially when perf is not an issue here.

thanks for the pointer modified it

riversand963 · 2022-02-04T22:30:23Z

util/comparator.cc

+  }
+
+ private:
+  const Comparator* cmp_without_ts_{nullptr};


This can actually be const T*.

riversand963 · 2022-02-04T22:31:04Z

util/comparator.cc

+// EXPERIMENTAL
+// Comparator with 64-bit integer timestamp.
+// We did not performance test this yet.
+template <typename T>


Nit: TComp or TComparator or ComparatorType.

satyajanga · 2022-02-05T03:30:27Z

Thinking more about this. I still think we should avoid adding public API if there is a way to do with out it. Adding a new API to expose an unoptimized comparator binding with a specific timestamp implementation seems hacky to me. Maybe we can add it later after longer period of time and broader discussion. I think it's possible to avoid adding the ComparatorWithU64Ts() or whatever name API for this PR. We can

Move class ComparatorWithU64TsImpl to comparator.cc in the ROCKSDB_NAMESPACE.

In comparator.cc, register this comparator in the ObjectRegistry/ObjectLibrary. After this is done, I think we can get a pointer to the comparator via CreateFromString(), which can be used to replace all existing test::ComparatorWithU64Ts() calls.

Modified the diff based on our chat

facebook-github-bot · 2022-02-05T19:23:43Z

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

facebook-github-bot · 2022-02-05T19:36:44Z

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

facebook-github-bot · 2022-02-05T19:44:47Z

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

riversand963 · 2022-02-07T17:41:15Z

db/db_test2.cc

@@ -17,6 +17,7 @@
 #include "options/options_helper.h"
 #include "port/port.h"
 #include "port/stack_trace.h"
+#include "rocksdb/comparator.h"


Is this needed?

riversand963 · 2022-02-07T17:59:41Z

db/compaction/compaction_iterator_test.cc

@@ -9,6 +9,7 @@
 #include <vector>

 #include "db/dbformat.h"
+#include "options/options_parser.h"


Is this needed?

riversand963 · 2022-02-07T18:02:38Z

test_util/testutil.cc

+const Comparator* BytewiseComparatorWithU64TsWrapper() {
+  ConfigOptions config_options;
+  const Comparator* user_comparator = nullptr;
+  Status s = Comparator::CreateFromString(


I think you need to check s so that the failing CircleCI test can pass.

or just do s.PermitUncheckedError().

added the s.PermitUncheckedError() for addressing circleci test failures

Thinking about this more, I think we may want to expose the error s to caller via something like

Status GetBytewiseComparatorWithU64TsWrapper(const Comparator** cmp);

sorry for the late comment.

We should definitely expose it if it is a public API or prod code,
Please remember that this is a test only function.
if we want to expose the Status then we probably dont need this util, because this wrapper is not saving much code.

Every usage in the test looks like below

const Comparator* user_comparator = nullptr; Status s = BytewiseComparatorWithU64TsWrapper(&user_comparator); assert(s.ok()); options.comparator = user_comparator;

Let me know if. you think we should do this.

riversand963 · 2022-02-07T18:06:31Z

tools/sst_dump_test.cc

@@ -7,6 +7,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file. See the AUTHORS file for names of contributors.

+#include <cstdint>


Do we need this here?
If we do, is it possible to remove the #include <stdint.h> in line 16?

riversand963 · 2022-02-07T18:12:49Z

tools/sst_dump_test.cc

-    for (uint32_t i = 0; i < num_keys; i++) {
-      tb->Add(MakeKey(i), MakeValue(i));
+    const std::string comparator_name = ikc.user_comparator()->Name();
+    if (comparator_name == ReverseBytewiseComparator()->Name()) {


Nit: very minor. there are several implicit data cast and string creation here. It may be easier to just use strcmp for the two comparator's Name(). Not a big deal for test.

riversand963 · 2022-02-07T18:18:48Z

tools/sst_dump_test.cc

+        char buf[100];
+        snprintf(buf, sizeof(buf), "v_%04d", i);


It's better to wrap this logic in a helper function MakeInternalkey(...).

It will be more interesting to make the keys have different timestamps.

riversand963 · 2022-02-07T18:23:23Z

util/comparator.cc

 #include <memory>
 #include <mutex>
+#include <sstream>
+#include <string>


Not needed.

riversand963 · 2022-02-07T18:25:33Z

util/comparator.cc

@@ -12,9 +12,13 @@
 #include <stdint.h>

 #include <algorithm>
+#include <cstring>


Probably not needed.

riversand963 · 2022-02-07T18:28:49Z

util/comparator.cc

+  }
+
+  TComparator cmp_without_ts_;
+  std::string name_;


Is this ever used?

riversand963 · 2022-02-07T18:35:52Z

util/comparator.cc

+  // For TComparator = BytewiseComparator the return value is
+  // "leveldb.BytewiseComparator.u64ts"


Not necessary and can be removed.

riversand963 · 2022-02-07T18:39:19Z

table/sst_file_dumper.cc

+                                           table_properties_->comparator_name,
+                                           &user_comparator);
+          if (s.ok()) {
+            internal_comparator_ =


Let's assert user_comparator is not nullptr at this point.

riversand963 · 2022-02-07T20:06:44Z

Also, can you remove the following from PR description

Reviewers:

Subscribers:

Tasks: T102457120

Tags:

The PR description here will be the final commit message, thus we want it to be concise and precise.

facebook-github-bot · 2022-02-07T21:43:01Z

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

Summary: We introduced a new Comparator for timestamp in user keys. In the sst_dump_tool by default we use BytewiseComparator to read sst files. This change allows us to read comparator_name from table properties in meta data block and use it to read. Test Plan: make check Reviewers: Subscribers: Tasks: Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

- removed the public facing api - templatized the ComparatorWithU64Ts

facebook-github-bot · 2022-02-07T21:57:25Z

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

riversand963

Thanks @satyajanga for the PR.
The changes LGTM. Can you also add a manual test to the test plan? For example, run

make -j16 sst_dump db_with_timestamp_basic_test
KEEP_DB=1 ./db_with_timestamp_basic_test --gtest_filter=<TestName>

Then use sst_dump on the resulting database.

./sst_dump --from=<from> --file=<>

riversand963

LGTM again. Thanks @satyajanga for the contribution

facebook-github-bot · 2022-02-08T18:28:01Z

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

facebook-github-bot · 2022-02-08T18:31:06Z

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

facebook-github-bot · 2022-02-08T18:32:15Z

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

facebook-github-bot · 2022-02-08T18:34:42Z

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

facebook-github-bot · 2022-02-08T18:36:25Z

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

Summary: rocksdb.compact_deletes testcase failed ``` sst_dump: rocksdb/table/sst_file_dumper.cc:147: my_rocksdb::Status my_rocksdb::SstFileDumper::GetTableReader(const std::string &): Assertion `user_comparator' failed. suite/rocksdb/t/sst_count_rows.sh: line 27: [: =: unary operator expected ``` The issue is related to Rocksdb PR: facebook/rocksdb#9491 "Use the comparator from the sst file table properties in sst_dump_tool". After this PR, sst_dump will use The comparator from myrocks sst file which is "RocksDB_SE_v3.10". the assert is triggered by rocksdb couldn't find comparator. similar to sst_mysql_ldb, register mysql comparator during startup Reviewed By: yizhang82 Differential Revision: D36391186 fbshipit-source-id: 520242b0a4db2dfbdc46c968457fa2c9e9bf44f4

facebook#9491) Summary: We introduced a new Comparator for timestamp in user keys. In the sst_dump_tool by default we use BytewiseComparator to read sst files. This change allows us to read comparator_name from table properties in meta data block and use it to read. Pull Request resolved: facebook#9491 Test Plan: added unittests for new functionality. make check ![image](https://user-images.githubusercontent.com/4923556/152915444-28b88a1f-7b4e-47d0-815f-7011552bd9a2.png) ![image](https://user-images.githubusercontent.com/4923556/152916196-bea3d2a1-a3d5-4362-b911-036131b83e8d.png) Reviewed By: riversand963 Differential Revision: D33993614 Pulled By: satyajanga fbshipit-source-id: 4b5cf938e6d2cb3931d763bef5baccc900b8c536 Signed-off-by: v01dstar <yang.zhang@pingcap.com>

facebook-github-bot added the CLA Signed label Feb 2, 2022

satyajanga requested a review from riversand963 February 2, 2022 20:48

riversand963 reviewed Feb 2, 2022

View reviewed changes

satyajanga changed the title ~~[WIP] support timestamp in sst_dump_tool~~ Use the comparator from the sst file table properties in sst_dump_tool Feb 3, 2022

riversand963 reviewed Feb 3, 2022

View reviewed changes

Merge pull request #1 from facebook/main

bf68fd7

Introduce a CountedFileSystem for counting file operations (facebook#9283)

satyajanga force-pushed the dump_tool branch from 4ddb400 to 007e877 Compare February 3, 2022 23:13

riversand963 reviewed Feb 3, 2022

View reviewed changes

riversand963 reviewed Feb 4, 2022

View reviewed changes

satyajanga commented Feb 4, 2022

View reviewed changes

satyajanga force-pushed the dump_tool branch from 0a8bc3f to c0e3c98 Compare February 4, 2022 22:22

riversand963 reviewed Feb 4, 2022

View reviewed changes

Merge branch 'facebook:main' into main

a582a4e

riversand963 reviewed Feb 7, 2022

View reviewed changes

satyajanga added 11 commits February 7, 2022 13:56

Address comments and Add a test for ReverseBytewiseComparator

fe710e4

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Address comments and Add a test for ReverseBytewiseComparator

2734cad

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

Address comments and Add a test for ReverseBytewiseComparator

3c6dbf8

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

add unittest for user timestamp sst dump

4967dae

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

update history

0a102e7

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:

address comments

e4b2bba

adding templates u64 comparator

54b3552

address the comments

c94ab0d

- removed the public facing api - templatized the ComparatorWithU64Ts

add extra comments in the code

b76c6a5

addres the code review comments

6fc0a16

satyajanga force-pushed the dump_tool branch from e9b996f to 6fc0a16 Compare February 7, 2022 21:57

riversand963 approved these changes Feb 7, 2022

View reviewed changes

riversand963 approved these changes Feb 8, 2022

View reviewed changes

Merge branch 'main' into dump_tool

3831311

facebook-github-bot closed this in 036bbab Feb 8, 2022

		// For TComparator = BytewiseComparator the return value is
		// "leveldb.BytewiseComparator.u64ts"

Use the comparator from the sst file table properties in sst_dump_tool #9491

Use the comparator from the sst file table properties in sst_dump_tool #9491

Conversation

satyajanga commented Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 commented Feb 2, 2022

satyajanga commented Feb 3, 2022

riversand963 commented Feb 3, 2022

riversand963 left a 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

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

satyajanga Feb 4, 2022 • edited Loading

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 Feb 4, 2022

riversand963 commented Feb 4, 2022

facebook-github-bot commented Feb 4, 2022

Choose a reason for hiding this comment

facebook-github-bot commented Feb 4, 2022

riversand963 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satyajanga commented Feb 5, 2022

facebook-github-bot commented Feb 5, 2022

facebook-github-bot commented Feb 5, 2022

facebook-github-bot commented Feb 5, 2022

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

satyajanga Feb 8, 2022 • edited Loading

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

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

riversand963 commented Feb 7, 2022

facebook-github-bot commented Feb 7, 2022

facebook-github-bot commented Feb 7, 2022

riversand963 left a comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

facebook-github-bot commented Feb 8, 2022

satyajanga commented Feb 2, 2022 •

edited

Loading

satyajanga Feb 4, 2022 •

edited

Loading

satyajanga Feb 8, 2022 •

edited

Loading