Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JNI Reader for Table Iterator #12511

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
b547bb4
Jni for TableIterator
swamirishi Apr 5, 2024
dcd5135
Merge remote-tracking branch 'origin/main' into HEAD
swamirishi Apr 5, 2024
8c4ca27
Merge branch 'main' into JniReaderForTableIterator
swamirishi Apr 5, 2024
f1be8b5
Address review comments
swamirishi Apr 6, 2024
9700892
Merge remote-tracking branch 'swamirishi/JniReaderForTableIterator' i…
swamirishi Apr 6, 2024
209f8fb
Merge remote-tracking branch 'origin/main' into HEAD
swamirishi Apr 6, 2024
0bfa474
Merge remote-tracking branch 'origin/main' into HEAD
swamirishi Apr 24, 2024
5fe932b
Jni to convert userKey to internalKey and vice versa
swamirishi May 1, 2024
5370842
Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…
swamirishi May 1, 2024
e0511d6
Add javadoc
swamirishi May 1, 2024
e567393
revert jni opt
swamirishi May 1, 2024
0569b4f
Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…
swamirishi May 4, 2024
cb81a60
Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…
swamirishi May 22, 2024
eade39d
Fix OutOfMemoryError
swamirishi May 22, 2024
5b5a25f
Address review commits
swamirishi May 24, 2024
b192395
Merge remote-tracking branch 'origin/main' into JniReaderForTableIter…
swamirishi May 24, 2024
a0b6213
Make final
swamirishi May 24, 2024
4a15702
Merge branch 'main' into JniReaderForTableIterator
swamirishi May 24, 2024
e30392b
Merge branch 'main' into JniReaderForTableIterator
swamirishi May 29, 2024
80dfbd6
Merge branch 'main' into JniReaderForTableIterator
swamirishi May 29, 2024
130581f
Merge branch 'main' into JniReaderForTableIterator
swamirishi Jun 8, 2024
fa151f1
Address review comments
swamirishi Jun 12, 2024
9ea4270
Address review comments
swamirishi Jun 12, 2024
5c743bc
Address review comments
swamirishi Jun 12, 2024
973362e
Address review comments
swamirishi Jun 12, 2024
0633d15
Add doc
swamirishi Jun 12, 2024
8a82a9e
Change function name
swamirishi Jun 22, 2024
45a22d3
Merge branch 'main' into JniReaderForTableIterator
swamirishi Jun 22, 2024
daa0a89
Merge branch 'main' into JniReaderForTableIterator
swamirishi Jul 24, 2024
26f9a50
Merge branch 'main' into JniReaderForTableIterator
swamirishi Jul 31, 2024
21eca90
Merge branch 'main' into JniReaderForTableIterator
swamirishi Aug 31, 2024
ccc6b59
Merge branch 'main' into JniReaderForTableIterator
swamirishi Sep 17, 2024
48c829c
Merge remote-tracking branch 'origin/main' into HEAD
swamirishi Jan 7, 2025
b754f11
Address review comments for error handling
swamirishi Jan 7, 2025
8695829
Add invalid enum entryType
swamirishi Jan 7, 2025
f175bc4
Merge branch 'main' into JniReaderForTableIterator
swamirishi Feb 20, 2025
d7d48d8
Merge branch 'main' into JniReaderForTableIterator
swamirishi Feb 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions java/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ set(JNI_NATIVE_SOURCES
rocksjni/write_batch_with_index.cc
rocksjni/write_buffer_manager.cc
rocksjni/writebatchhandlerjnicallback.cc
rocksjni/parsed_entry_info.cc
rocksjni/type_util.cc
)

set(JAVA_MAIN_CLASSES
Expand Down Expand Up @@ -308,6 +310,9 @@ set(JAVA_MAIN_CLASSES
src/test/java/org/rocksdb/test/TestableEventListener.java
src/test/java/org/rocksdb/util/CapturingWriteBatchHandler.java
src/test/java/org/rocksdb/util/WriteBatchGetter.java
src/main/java/org/rocksdb/ParsedEntryInfo.java
src/main/java/org/rocksdb/EntryType.java
src/main/java/org/rocksdb/TypeUtil.java
)

set(JAVA_TEST_CLASSES
Expand Down
185 changes: 185 additions & 0 deletions java/rocksjni/parsed_entry_info.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
// Copyright (c) 2011-present, Facebook, Inc. All rights reserved.
// This source code is licensed under both the GPLv2 (found in the
// COPYING file in the root directory) and Apache 2.0 License
// (found in the LICENSE.Apache file in the root directory).
//
// This file implements the "bridge" between Java and C++ and enables
// calling c++ ROCKSDB_NAMESPACE::Iterator methods from Java side.

#include <jni.h>
#include <stdlib.h>

#include "include/org_rocksdb_ParsedEntryInfo.h"
#include "rocksdb/options.h"
#include "rocksdb/types.h"
#include "rocksdb/utilities/types_util.h"
#include "rocksjni/cplusplus_to_java_convert.h"
#include "rocksjni/portal.h"

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: newParseEntryInstance
* Signature: ()J
*/
jlong JNICALL Java_org_rocksdb_ParsedEntryInfo_newParseEntryInstance(
JNIEnv * /*env*/, jclass /*cls*/) {
ROCKSDB_NAMESPACE::ParsedEntryInfo *parsed_entry_info =
new ROCKSDB_NAMESPACE::ParsedEntryInfo();
return GET_CPLUSPLUS_POINTER(parsed_entry_info);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: parseEntry
* Signature: (JJ[BI)V
*/
void JNICALL Java_org_rocksdb_ParsedEntryInfo_parseEntry(
JNIEnv *env, jclass /*cls*/, jlong handle, jlong options_handle,
jbyteArray jtarget, jint len) {
auto *options =
reinterpret_cast<const ROCKSDB_NAMESPACE::Options *>(options_handle);
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
jbyte *target = env->GetByteArrayElements(jtarget, nullptr);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@swamirishi swamirishi Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (target == nullptr) {
ROCKSDB_NAMESPACE::OutOfMemoryErrorJni::ThrowNew(env,
"Memory allocation failed in RocksDB JNI function");
return;
}
ROCKSDB_NAMESPACE::Slice target_slice(reinterpret_cast<char *>(target), len);
ROCKSDB_NAMESPACE::ParseEntry(target_slice, options->comparator,
parsed_entry_info);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: parseEntryDirect
* Signature: (JJLjava/nio/ByteBuffer;II)V
*/
void JNICALL Java_org_rocksdb_ParsedEntryInfo_parseEntryDirect(
JNIEnv *env, jclass /*clz*/, jlong handle, jlong options_handle,
jobject jbuffer, jint jbuffer_off, jint jbuffer_len) {
auto *options =
reinterpret_cast<const ROCKSDB_NAMESPACE::Options *>(options_handle);
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
auto parse = [&parsed_entry_info,
&options](ROCKSDB_NAMESPACE::Slice &target_slice) {
ROCKSDB_NAMESPACE::ParseEntry(target_slice, options->comparator,
parsed_entry_info);
};
ROCKSDB_NAMESPACE::JniUtil::k_op_direct(parse, env, jbuffer, jbuffer_off,
jbuffer_len);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: parseEntryByteArray
* Signature: (JJ[BII)V
*/
void JNICALL Java_org_rocksdb_ParsedEntryInfo_parseEntryByteArray(
JNIEnv *env, jclass /*clz*/, jlong handle, jlong options_handle,
jbyteArray jtarget, jint joff, jint jlen) {
auto *options =
reinterpret_cast<const ROCKSDB_NAMESPACE::Options *>(options_handle);
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
auto parse = [&parsed_entry_info,
&options](ROCKSDB_NAMESPACE::Slice &target_slice) {
ROCKSDB_NAMESPACE::ParseEntry(target_slice, options->comparator,
parsed_entry_info);
};
ROCKSDB_NAMESPACE::JniUtil::k_op_indirect(parse, env, jtarget, joff, jlen);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: userKeyDirect
* Signature: (JLjava/nio/ByteBuffer;II)I
*/
jint JNICALL Java_org_rocksdb_ParsedEntryInfo_userKeyDirect(
JNIEnv *env, jclass /*clz*/, jlong handle, jobject jtarget, jint joffset,
jint jlen) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
ROCKSDB_NAMESPACE::Slice key_slice = parsed_entry_info->user_key;
return ROCKSDB_NAMESPACE::JniUtil::copyToDirect(env, key_slice, jtarget,
joffset, jlen);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: userKeyByteArray
* Signature: (J[BII)I
*/
jint JNICALL Java_org_rocksdb_ParsedEntryInfo_userKeyByteArray(
JNIEnv *env, jclass /*clz*/, jlong handle, jbyteArray jtarget, jint joffset,
jint jlen) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
ROCKSDB_NAMESPACE::Slice key_slice = parsed_entry_info->user_key;
return ROCKSDB_NAMESPACE::JniUtil::copyToByteArray(env, key_slice, jtarget,
joffset, jlen);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: userKeyJni
* Signature: (J)[B
*/
jbyteArray JNICALL Java_org_rocksdb_ParsedEntryInfo_userKeyJni(JNIEnv *env,
jclass /*clz*/,
jlong handle) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
ROCKSDB_NAMESPACE::Slice key_slice = parsed_entry_info->user_key;
jbyteArray jkey = env->NewByteArray(static_cast<jsize>(key_slice.size()));
if (jkey == nullptr) {
// exception thrown: OutOfMemoryError
ROCKSDB_NAMESPACE::OutOfMemoryErrorJni::ThrowNew(env, "Memory allocation failed in RocksDB JNI function");
return nullptr;
}
ROCKSDB_NAMESPACE::JniUtil::copyToByteArray(
env, key_slice, jkey, 0, static_cast<jint>(key_slice.size()));
return jkey;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a check in copyBytes function

}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: getSequenceNumberJni
* Signature: (J)J
*/
jlong JNICALL Java_org_rocksdb_ParsedEntryInfo_getSequenceNumberJni(
JNIEnv * /*env*/, jclass /*clz*/, jlong handle) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
uint64_t sequence_number = parsed_entry_info->sequence;
return static_cast<jlong>(sequence_number);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: getValueTypeJni
* Signature: (J)B
*/
jbyte JNICALL Java_org_rocksdb_ParsedEntryInfo_getEntryTypeJni(JNIEnv * /*env*/,
jclass /*clz*/,
jlong handle) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
ROCKSDB_NAMESPACE::EntryType type = parsed_entry_info->type;
return ROCKSDB_NAMESPACE::EntryTypeJni::toJavaEntryType(type);
}

/*
* Class: org_rocksdb_ParsedEntryInfo
* Method: disposeInternalJni
* Signature: (J)V
*/
void JNICALL Java_org_rocksdb_ParsedEntryInfo_disposeInternalJni(
JNIEnv * /*env*/, jclass /*clz*/, jlong handle) {
auto *parsed_entry_info =
reinterpret_cast<ROCKSDB_NAMESPACE::ParsedEntryInfo *>(handle);
assert(parsed_entry_info != nullptr);
delete parsed_entry_info;
}
77 changes: 77 additions & 0 deletions java/rocksjni/portal.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <type_traits>
#include <vector>

#include "db/dbformat.h"
#include "rocksdb/convenience.h"
#include "rocksdb/db.h"
#include "rocksdb/filter_policy.h"
Expand Down Expand Up @@ -2477,6 +2478,33 @@ class JniUtil {
return op(key_slice);
}

/*
* Helper for operations on a key on java byte array
* Copies values from jbyte array to slice and performs op on the slice.
* for example WriteBatch->Delete
*
* from `op` and used for RocksDB->Delete etc.
*/
static void k_op_indirect(std::function<void(ROCKSDB_NAMESPACE::Slice&)> op,
JNIEnv* env, jbyteArray jkey, jint jkey_off,
jint jkey_len) {
if (jkey == nullptr || env->GetArrayLength(jkey) < (jkey_off + jkey_len)) {
ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env,
"Invalid key argument");
return;
}
const std::unique_ptr<char[]> target(new char[jkey_len]);
if (target == nullptr) {
ROCKSDB_NAMESPACE::OutOfMemoryErrorJni::ThrowNew(env,
"Memory allocation failed in RocksDB JNI function");
return;
}
env->GetByteArrayRegion(jkey, jkey_off, jkey_len,
reinterpret_cast<jbyte*>(target.get()));
ROCKSDB_NAMESPACE::Slice target_slice(target.get(), jkey_len);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return op(target_slice);
}

template <class T>
static jint copyToDirect(JNIEnv* env, T& source, jobject jtarget,
jint jtarget_off, jint jtarget_len) {
Expand All @@ -2498,6 +2526,26 @@ class JniUtil {

return cvalue_len;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

jint jtarget_off, jint jtarget_len) {
if (jtarget == nullptr ||
env->GetArrayLength(jtarget) < (jtarget_off + jtarget_len)) {
ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(
env, "Invalid target argument");
return 0;
}

const jint cvalue_len = static_cast<jint>(source.size());
const jint length = std::min(jtarget_len, cvalue_len);
env->SetByteArrayRegion(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error check - if SetByteArrayRegion raises an exception, you should return 0.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jtarget, jtarget_off, length,
const_cast<jbyte*>(reinterpret_cast<const jbyte*>(source.data())));
return cvalue_len;
}
};

class MapJni : public JavaClass {
Expand Down Expand Up @@ -6405,6 +6453,35 @@ class TxnDBWritePolicyJni {
}
};

// The portal class for org.rocksdb.EntryType
class EntryTypeJni {
public:
static jbyte toJavaEntryType(const ROCKSDB_NAMESPACE::EntryType& entry_type) {
switch (entry_type) {
case ROCKSDB_NAMESPACE::EntryType::kEntryPut:
return 0x0;
case ROCKSDB_NAMESPACE::EntryType::kEntryDelete:
return 0x1;
case ROCKSDB_NAMESPACE::EntryType::kEntrySingleDelete:
return 0x2;
case ROCKSDB_NAMESPACE::EntryType::kEntryMerge:
return 0x3;
case ROCKSDB_NAMESPACE::EntryType::kEntryRangeDeletion:
return 0x4;
case ROCKSDB_NAMESPACE::EntryType::kEntryBlobIndex:
return 0x5;
case ROCKSDB_NAMESPACE::EntryType::kEntryDeleteWithTimestamp:
return 0x6;
case ROCKSDB_NAMESPACE::EntryType::kEntryWideColumnEntity:
return 0x7;
case ROCKSDB_NAMESPACE::EntryType::kEntryOther:
return 0x8;
default:
return 0x9;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an invalid enum on java side to handle this default value.

}
}
};

// The portal class for org.rocksdb.TransactionDB.KeyLockInfo
class KeyLockInfoJni : public JavaClass {
public:
Expand Down
13 changes: 13 additions & 0 deletions java/rocksjni/sst_file_readerjni.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,19 @@ jlong Java_org_rocksdb_SstFileReader_newIterator(JNIEnv * /*env*/,
return GET_CPLUSPLUS_POINTER(sst_file_reader->NewIterator(*read_options));
}

/*
* Class: org_rocksdb_SstFileReader
* Method: newTableIterator
* Signature: (JJ)J
*/
jlong Java_org_rocksdb_SstFileReader_newTableIterator(JNIEnv * /*env*/,
jclass /*jcls*/,
jlong jhandle) {
auto *sst_file_reader =
reinterpret_cast<ROCKSDB_NAMESPACE::SstFileReader *>(jhandle);
return GET_CPLUSPLUS_POINTER(sst_file_reader->NewTableIterator().release());
}

/*
* Class: org_rocksdb_SstFileReader
* Method: disposeInternal
Expand Down
Loading