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-05-29T04:54:54Z

@adamretter can you please take a look at this?

swamirishi · 2024-06-08T02:40:41Z

@adamretter @jowlyzhang Can you please take a look at this patch, whenever you find time?

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

java/src/main/java/org/rocksdb/ParsedEntryInfo.java

adamretter · 2024-06-11T09:32:58Z

java/src/main/java/org/rocksdb/ParsedEntryInfo.java

+ return result;
+ }
+
+ public void parseEntry(Options options, byte[] internalKey) {


Please mark parameters as final. (and methods/functions below)

java/rocksjni/portal.h

adamretter · 2024-06-11T09:50:23Z

java/rocksjni/portal.h

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


There does not appear to be a 0x9 defined in the Java enum, so this will raise an exception. Is that intentional?

adamretter · 2024-06-11T09:51:01Z

java/rocksjni/sst_file_readerjni.cc

+ jlong jhandle) {
+ auto *sst_file_reader =
+ reinterpret_cast<ROCKSDB_NAMESPACE::SstFileReader *>(jhandle);
+ return GET_CPLUSPLUS_POINTER(sst_file_reader->NewTableIterator().release());


I am a bit worried about the release() here? What is the ownership model for the iterator in your code please?

It is the same as sstFileReaderIterator. So release should be triggered manually irrespective of sstFileReader class

java/rocksjni/type_util.cc

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?

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

Merge branch 'main' into JniReaderForTableIterator

e30392b

swamirishi added 2 commits May 29, 2024 16:20

Merge branch 'main' into JniReaderForTableIterator

80dfbd6

Merge branch 'main' into JniReaderForTableIterator

130581f

adamretter requested changes Jun 11, 2024

View reviewed changes

swamirishi added 5 commits June 11, 2024 18:51

Address review comments

fa151f1

Address review comments

9ea4270

Address review comments

5c743bc

Address review comments

973362e

Add doc

0633d15

swamirishi force-pushed the JniReaderForTableIterator branch from 323d6c0 to 0633d15 Compare June 12, 2024 02:08

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

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 May 29, 2024

swamirishi commented Jun 8, 2024

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 Jun 12, 2024

swamirishi commented Jun 20, 2024

adamretter commented Jun 21, 2024

swamirishi commented Jun 22, 2024

swamirishi commented Apr 5, 2024 •

edited

Loading