JNI Reader for Table Iterator #12511

swamirishi · 2024-04-05T00:42:58Z

Adding a Jni layer for table Iterator. Also changing the table iterator's interface preparse internal key so that SSTFileReaderIterator can be reused.

swamirishi · 2024-04-05T00:45:16Z

@cbi42 @jowlyzhang Can you please review this? In ozone we depend on jni for rocksdb apis. I have just added JNI layer for the table_iterator written in your PR.

adamretter

Thanks for the PR. I have left some feedback on the Java changes which need some amendment - also a Java test needs to be added please for this new API. I will leave @ajkr @pdillinger and the RocksDB team to comment on the internal C++ changes to RocksDB table... as I cannot myself say if they are desirable or not for the RocksDB team.

java/src/main/java/org/rocksdb/RawSstFileReaderIterator.java

java/rocksjni/raw_sst_file_reader_iterator.cc

java/src/main/java/org/rocksdb/RawSstFileReaderIterator.java

adamretter · 2024-04-05T08:34:42Z

java/src/main/java/org/rocksdb/RawSstFileReaderIterator.java

+ */
+ class RawSstFileReaderIterator extends SstFileReaderIterator {
+
+  protected RawSstFileReaderIterator(SstFileReader reader, long nativeHandle) {


Please set all arguments and variables to final where possible.

adamretter · 2024-04-05T08:36:30Z

java/src/main/java/org/rocksdb/RawSstFileReaderIterator.java

+ */
+ class RawSstFileReaderIterator extends SstFileReaderIterator {
+
+  protected RawSstFileReaderIterator(SstFileReader reader, long nativeHandle) {


This class needs to implement some distinct form of disposeInternal to avoid leaking memory when it is finished with.

It extends SstFileReaderIterator which clears off the required handle.

adamretter · 2024-04-05T08:38:03Z

table/sst_file_reader.cc

@@ -113,7 +113,7 @@ std::unique_ptr<Iterator> SstFileReader::NewTableIterator() {
    // InternalIterator.
    return nullptr;
  }
-  return std::make_unique<TableIterator>(internal_iter);
+  return std::make_unique<TableIterator>(internal_iter, &rep_->options);


@ajkr @pdillinger Can you comment on this change in the internals for RocksDB please?

adamretter · 2024-04-05T08:38:45Z

table/table_iterator.h

@@ -24,7 +25,7 @@ class TableIterator : public Iterator {
  }

 public:
-  explicit TableIterator(InternalIterator* iter) : iter_(iter) {}
+  explicit TableIterator(InternalIterator* iter, Options* options) : iter_(iter), options_(options) {}


@ajkr @pdillinger Can you comment on this change in the internals for RocksDB please?

adamretter · 2024-04-05T08:38:57Z

table/table_iterator.h

@@ -48,22 +49,65 @@ class TableIterator : public Iterator {
  bool Valid() const override { return iter_->Valid(); }
  void SeekToFirst() override { return iter_->SeekToFirst(); }
  void SeekToLast() override { return iter_->SeekToLast(); }
-  void Seek(const Slice& target) override { return iter_->Seek(target); }
+  void Seek(const Slice& target) override {


@ajkr @pdillinger Can you comment on this change in the internals for RocksDB please?

I will review this. Thanks!

adamretter · 2024-04-05T08:39:06Z

table/table_iterator.h

  void SeekForPrev(const Slice& target) override {
-    return iter_->SeekForPrev(target);
+      std::string seek_key_buf;


@ajkr @pdillinger Can you comment on this change in the internals for RocksDB please?

jowlyzhang · 2024-04-05T16:35:25Z

table/table_iterator.h

@@ -48,22 +49,65 @@ class TableIterator : public Iterator {
  bool Valid() const override { return iter_->Valid(); }
  void SeekToFirst() override { return iter_->SeekToFirst(); }
  void SeekToLast() override { return iter_->SeekToLast(); }
-  void Seek(const Slice& target) override { return iter_->Seek(target); }
+  void Seek(const Slice& target) override {


I will review this. Thanks!

jowlyzhang · 2024-04-05T16:36:42Z

table/table_iterator.h

@@ -48,22 +49,65 @@ class TableIterator : public Iterator {
  bool Valid() const override { return iter_->Valid(); }
  void SeekToFirst() override { return iter_->SeekToFirst(); }
  void SeekToLast() override { return iter_->SeekToLast(); }
-  void Seek(const Slice& target) override { return iter_->Seek(target); }
+  void Seek(const Slice& target) override {


The type of key taken as input for Seek and SeekForPrev should be consistent with the type of key produced by Iterator::key. Can you add jni APIs for the util methods GetInternalKeyForSeek etc instead?

Can't we change the definition of the TableIterator to seek and get only on the user_key instead? Isn't this a better way to define this class?

The name TableIterator indicates it's iterating the raw table, and that makes internal key a better format of key for these APIs.

jowlyzhang · 2024-04-05T16:41:00Z

table/table_iterator.h

  Status GetProperty(std::string /*prop_name*/,
                     std::string* /*prop*/) override {
    assert(false);
    return Status::NotSupported("TableIterator does not support GetProperty.");
  }

+  uint64_t SequenceNumber() {


The public API is Iterator in rocksdb/includes/iterator.h, the TableIterator is an internal class that is not supposed to be publicly accessible. These SequenceNumber, and type APIs are not defined for Iterator.

We have added ParseEntry util methods to parse sequence number and type out of an internal key. You can have you own wrapper iterator class wrapping a RocksDB Iterator, and add the parsing logic in to the wrapper iterator.

We need all of these infos separately and storing the parsed entry would be much more optimal. Otherwise for each and every call we would have to parse the key over and over again. All I am doing here is changing the definition of the table iter which would seek over the same user key and return user key when getKey() is called. I have added 2 more functions on top of iterator, I believe that should be fine, since JNI would be called only when TableIterator is initialized.

@jowlyzhang TableIterator itself is a wrapper for InternalIterator why have 2 wrappers? Should we consider making this public?

It's worth having 2 wrappers because it will be your own wrapper iterator that you don't need to check into RocksDB codebase, you own those logic and you can modify it however you want. Parsing an entry every time over and over again isn't computationally heavy or performance wise concerning, you can just wrap it in your own wrapper iterator. It's beset for the RocksDB TableIterator to just do the most vanilla things of iterating raw entries. Others wishing to add their own iterating constraints can just add on top of it.

@jowlyzhang I have updated the code to add another jni functionality to convert internal Key to user key and vice versa

…nto HEAD

…ator_wip

swamirishi · 2024-05-04T07:41:08Z

@jowlyzhang Can you check this PR?

jowlyzhang · 2024-05-06T16:59:19Z

@jowlyzhang Can you check this PR?

Thanks for making the changes. Since this PR now mostly have java changes, I'll leave it to @adamretter to make the call here.

adamretter

The issues I think I have identified repeat throughout and should be addressed please

adamretter · 2024-05-07T15:28:08Z

java/src/main/java/org/rocksdb/EntryType.java

+  kEntryWideColumnEntity((byte)0x7),
+  kEntryOther((byte)0x8);
+  private final byte value;
+  private static Map<Byte, EntryType> reverseLookupMap = new HashMap<>();


Do we need a map here? The collection is small enough to iterate I would have thought?

removed the map and falling back to iterating through the values.

adamretter · 2024-05-07T15:28:28Z

java/src/main/java/org/rocksdb/EntryType.java

+  private final byte value;
+  private static Map<Byte, EntryType> reverseLookupMap = new HashMap<>();
+
+  EntryType(byte value) {


Should be final param.

adamretter · 2024-05-07T15:30:07Z

java/rocksjni/parsed_entry_info.cc

+                                                                  jint len) {
+  const std::unique_ptr<char[]> target(new char[len]);
+  if (target == nullptr) {
+    jclass oom_class = env->FindClass("/lang/java/OutOfMemoryError");


I don't think this has been tested as the class name is incorrect. Also there are util methods in portal.h for throwing errors that could be used.

Added util method to also do operations on indirect byte array buffers

adamretter · 2024-05-07T15:30:21Z

java/rocksjni/parsed_entry_info.cc

+                  "Memory allocation failed in RocksDB JNI function");
+    return;
+  }
+  env->GetByteArrayRegion(jtarget, off, len,


Needs error checking

adamretter · 2024-05-07T15:30:50Z

java/rocksjni/parsed_entry_info.cc

+  auto slice_size = key_slice.size();
+  jsize copy_size = std::min(static_cast<uint32_t>(slice_size),
+                             static_cast<uint32_t>(jlen));
+  env->SetByteArrayRegion(


Needs error checking

adamretter · 2024-05-07T15:32:31Z

java/rocksjni/type_util.cc

+  auto slice_size = key_slice.size();
+  jsize copy_size = std::min(static_cast<uint32_t>(slice_size),
+                             static_cast<uint32_t>(int_key_len));
+  env->SetByteArrayRegion(


Needs error checking.

adamretter · 2024-05-07T15:32:43Z

java/rocksjni/type_util.cc

+      reinterpret_cast<const ROCKSDB_NAMESPACE::Options *>(options_handle);
+  const std::unique_ptr<char[]> target(new char[user_key_len]);
+  if (target == nullptr) {
+    jclass oom_class = env->FindClass("/lang/java/OutOfMemoryError");


Class name and testing?

adamretter · 2024-05-07T15:32:47Z

java/rocksjni/type_util.cc

+                  "Memory allocation failed in RocksDB JNI function");
+    return static_cast<jsize>(0);
+  }
+  env->GetByteArrayRegion(user_key, user_key_off, user_key_len,


Needs error checking.

adamretter · 2024-05-07T15:32:55Z

java/rocksjni/type_util.cc

+  auto slice_size = key_slice.size();
+  jsize copy_size = std::min(static_cast<uint32_t>(slice_size),
+                             static_cast<uint32_t>(int_key_len));
+  env->SetByteArrayRegion(


Needs error checking.

adamretter · 2024-05-07T15:33:06Z

java/rocksjni/type_util.cc

+    // exception thrown: OutOfMemoryError
+    return nullptr;
+  }
+  env->SetByteArrayRegion(


Needs error checking.

…ator_wip

swamirishi · 2024-05-24T01:57:51Z

@adamretter Apologies for the late response and not following through addressing review comments on time. I have addressed all the review comments. Can you please take a look at it? I have added a Java test for the same as well and also added required util functions as well in portal.h. FYI these util functions can be used in other JNI classes as well.

swamirishi · 2024-06-12T13:50:05Z

@adamretter I have addressed all the review comments. Can you please take a look at the patch again?

swamirishi · 2024-06-20T20:24:23Z

@adamretter Can you take a look at this?

adamretter · 2024-06-21T08:16:11Z

@swamirishi Not all of my comments have been addressed.

swamirishi · 2024-06-22T02:48:09Z

@swamirishi Not all of my comments have been addressed.

@adamretter Yeah sorry I missed one of the comments. I have changed the method name and set the parameters to final. Can you check if this is fine now?

swamirishi · 2024-07-15T15:06:27Z

@adamretter can you please look into this I have addressed all the review comments here.

swamirishi · 2024-07-24T17:16:01Z

@adamretter @jowlyzhang Can you please take a look at this patch? I have addressed all the review comments and this has been pending for a while.

adamretter · 2024-07-31T08:00:19Z

@swamirishi Apologies, I have been on holiday. I will try and review this shortly...

swamirishi · 2024-07-31T22:24:46Z

@adamretter thanks a lot

swamirishi · 2024-08-31T17:09:46Z

@adamretter Any updates on this?

swamirishi · 2024-09-17T19:06:49Z

@adamretter Can you review this PR or is there anyone else who can review this PR if you are loaded with other stuff

adamretter

Still some missing error handling that needs to be implemented.

adamretter · 2024-09-18T11:35:19Z

java/rocksjni/portal.h

+    }
+    env->GetByteArrayRegion(jkey, jkey_off, jkey_len,
+                            reinterpret_cast<jbyte*>(target.get()));
+    ROCKSDB_NAMESPACE::Slice target_slice(target.get(), jkey_len);


Missing error check for the preceding JNI call, e.g. if (env->ExceptionOccurred())

adamretter · 2024-09-18T11:40:01Z

java/rocksjni/parsed_entry_info.cc

+      reinterpret_cast<const ROCKSDB_NAMESPACE::Options *>(options_handle);
+  auto *parsed_entry_info =
+      reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
+  jbyte *target = env->GetByteArrayElements(jtarget, nullptr);


You need to consider the lifetime of target... should there be a corresponding call to ReleaseByteArrayElements?

Yeah you are right thanks for pointing out the issue. I have added a boolean in ParsedEntryInfo struct to clear up on every parseEntry function or close

adamretter · 2024-09-18T11:46:23Z

java/rocksjni/portal.h

+  /* Helper for copying value in source into a byte array.
+   */
+  template <class T>
+  static jint copyToByteArray(JNIEnv* env, T& source, jbyteArray jtarget,


There are already some copyBytes functions in portal.h that are very similar apart from they allocate the jbyteArray. Could you create an overload of copyBytes that takes the jtarget, jtarget_off, jtarget_len, and then rename this copyBytes and refactor to remove code duplication?

done, renamed the function copyBytes. I don't think we can really reuse any of the existing function to reuse. I have called the existing copyBytes to create a new byte array instead of calling the copyToByteArray function

adamretter · 2024-09-18T11:46:55Z

java/rocksjni/portal.h

+      case ROCKSDB_NAMESPACE::EntryType::kEntryOther:
+        return 0x8;
+      default:
+        return 0x9;


I feel uncomfortable with this approach that raises an exception for an invalid enum value. Is there a better approach perhaps?

java/rocksjni/sst_file_readerjni.cc

adamretter · 2024-09-18T12:03:30Z

java/rocksjni/type_util.cc

+  };
+  ROCKSDB_NAMESPACE::JniUtil::k_op_indirect(getInternalKeySeek, env, user_key,
+                                            user_key_off, user_key_len);
+  ROCKSDB_NAMESPACE::Slice key_slice = seek_key_buf;


Missing an error check to see if k_op_indirect raised a Java exception.

fixed in k_op_indirect function.

adamretter · 2024-09-18T12:03:37Z

java/rocksjni/type_util.cc

+  };
+  ROCKSDB_NAMESPACE::JniUtil::k_op_indirect(getInternalKeySeek, env, user_key,
+                                            user_key_off, user_key_len);
+  ROCKSDB_NAMESPACE::Slice key_slice = seek_key_buf;


Missing an error check to see if k_op_indirect raised a Java exception.

fixed in k_op_indirect function.

adamretter · 2024-09-18T12:04:56Z

java/rocksjni/parsed_entry_info.cc

+  }
+  ROCKSDB_NAMESPACE::JniUtil::copyToByteArray(
+      env, key_slice, jkey, 0, static_cast<jint>(key_slice.size()));
+  return jkey;


Missing an error check to see if copyToByteArray raised a Java exception.

added a check in copyBytes function

adamretter · 2024-09-18T12:05:10Z

java/rocksjni/type_util.cc

+  }
+  ROCKSDB_NAMESPACE::JniUtil::copyToByteArray(
+      env, key_slice, jkey, 0, static_cast<jint>(key_slice.size()));
+  return jkey;


Missing an error check to see if copyToByteArray raised a Java exception.

adamretter · 2024-09-18T12:05:25Z

java/rocksjni/type_util.cc

+  }
+  ROCKSDB_NAMESPACE::JniUtil::copyToByteArray(
+      env, key_slice, jkey, 0, static_cast<jint>(key_slice.size()));
+  return jkey;


Missing an error check to see if copyToByteArray raised a Java exception.

adamretter · 2025-01-06T15:47:33Z

@swamirishi Are you still working on this?

swamirishi · 2025-01-06T16:56:14Z

@swamirishi Are you still working on this?
@adamretter Apologies for not addressing the issues. I have been busy working on other stuff. But I am working on it now and would be able to address all the review comments by EOD.

swamirishi · 2025-01-07T08:02:54Z

@adamretter I have addressed most of the review comments to the best of my knowledge. I had a couple of doubts for few review comments and have left comments inline.

swamirishi · 2025-02-20T05:17:57Z

@adamretter Can this be merged?

Jni for TableIterator

b547bb4

facebook-github-bot added the CLA Signed label Apr 5, 2024

swamirishi force-pushed the JniReaderForTableIterator branch from 52a2255 to 6b83480 Compare April 5, 2024 01:03

Merge remote-tracking branch 'origin/main' into HEAD

dcd5135

swamirishi force-pushed the JniReaderForTableIterator branch from 6b83480 to dcd5135 Compare April 5, 2024 01:04

adamretter self-requested a review April 5, 2024 07:03

Merge branch 'main' into JniReaderForTableIterator

8c4ca27

adamretter requested changes Apr 5, 2024

View reviewed changes

jowlyzhang requested changes Apr 5, 2024

View reviewed changes

swamirishi added 9 commits April 6, 2024 05:37

Address review comments

f1be8b5

Merge remote-tracking branch 'swamirishi/JniReaderForTableIterator' i…

9700892

…nto HEAD

Merge remote-tracking branch 'origin/main' into HEAD

209f8fb

Merge remote-tracking branch 'origin/main' into HEAD

0bfa474

Jni to convert userKey to internalKey and vice versa

5fe932b

Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…

5370842

…ator_wip

Add javadoc

e0511d6

revert jni opt

e567393

Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…

0569b4f

…ator_wip

adamretter requested changes May 7, 2024

View reviewed changes

swamirishi added 4 commits May 21, 2024 23:19

Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…

cb81a60

…ator_wip

Fix OutOfMemoryError

eade39d

Address review commits

5b5a25f

Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…

b192395

…ator_wip

swamirishi and others added 2 commits May 23, 2024 19:00

Make final

a0b6213

Merge branch 'main' into JniReaderForTableIterator

4a15702

swamirishi requested a review from adamretter May 28, 2024 16:34

swamirishi requested a review from adamretter June 12, 2024 02:09

Change function name

8a82a9e

swamirishi force-pushed the JniReaderForTableIterator branch 2 times, most recently from 08d06eb to 8a82a9e Compare June 22, 2024 02:44

Merge branch 'main' into JniReaderForTableIterator

45a22d3

adamretter changed the title ~~Jni reader for table iterator~~ JNI Reader for Table Iterator Jul 2, 2024

Merge branch 'main' into JniReaderForTableIterator

daa0a89

Merge branch 'main' into JniReaderForTableIterator

26f9a50

Merge branch 'main' into JniReaderForTableIterator

21eca90

Merge branch 'main' into JniReaderForTableIterator

ccc6b59

adamretter requested changes Sep 18, 2024

View reviewed changes

swamirishi added 3 commits January 6, 2025 16:50

Merge remote-tracking branch 'origin/main' into HEAD

48c829c

Address review comments for error handling

b754f11

Add invalid enum entryType

8695829

Merge branch 'main' into JniReaderForTableIterator

f175bc4

Merge branch 'main' into JniReaderForTableIterator

d7d48d8

JNI Reader for Table Iterator #12511

Are you sure you want to change the base?

JNI Reader for Table Iterator #12511

Conversation

swamirishi commented Apr 5, 2024

swamirishi commented Apr 5, 2024 • edited Loading

adamretter 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

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

swamirishi commented May 4, 2024

jowlyzhang commented May 6, 2024

adamretter 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

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

swamirishi commented May 24, 2024

swamirishi commented Jun 12, 2024

swamirishi commented Jun 20, 2024

adamretter commented Jun 21, 2024

swamirishi commented Jun 22, 2024

swamirishi commented Jul 15, 2024

swamirishi commented Jul 24, 2024

adamretter commented Jul 31, 2024

swamirishi commented Jul 31, 2024

swamirishi commented Aug 31, 2024

swamirishi commented Sep 17, 2024

adamretter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swamirishi Jan 7, 2025 • 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

Choose a reason for hiding this comment

adamretter commented Jan 6, 2025

swamirishi commented Jan 6, 2025

swamirishi commented Apr 5, 2024 •

edited

Loading

swamirishi Jan 7, 2025 •

edited

Loading