Add support for Wide columns in Java #12334

rhubner · 2024-02-07T14:09:21Z

This PR adds support for wide columns in Java. First implementation is primarily focused on correctness rather than performance. We would like to merge this first and hear feedback from RocksDB Java community before refining it.

Fix #12113

adamretter

I have done a partial review... There are three main things that you might want to look at IMHO:

Whether you really need std::make_unique for a pointer that never escapes its function scope. It seems superfluous to me.
Clean up of JNI resources you have allocated before you return from a function when an error condition occurs (or even when the function completes normally)
String conversion from JNI types to C++ standard types. There are functions in portal.h to assist in doing this correctly. If they don't suit, we can consider adding an additional one for your use-case.

java/rocksjni/portal.h

adamretter · 2024-03-13T08:21:56Z

java/rocksjni/portal.h

+            "ByteBuffer)");
+        return;
+      }
+      auto name = reinterpret_cast<char*>(_name) + namesOffset[i];


There are utility methods in portal.h to go from jstring to std::string or char* - could we use those here instead?

We are not working with jstring but with void* here. I cast it to char* so that I can add offset to it.

adamretter · 2024-03-13T08:22:02Z

java/rocksjni/portal.h

+
+    const auto columns_len = static_cast<int>(env->GetArrayLength(jNames));
+
+    auto namesOffset = std::make_unique<jint[]>(columns_len);


make_unique seems an unnecessary overhead here?

Yes, that's true, but it allows me to automatically release all the memory allocated here and simplify return statements.

adamretter · 2024-03-13T08:23:48Z

java/rocksjni/portal.h

+      auto j_value =
+          static_cast<jbyteArray>(env->GetObjectArrayElement(jvalues, i));
+      if (env->ExceptionCheck()) {
+        return;


Do we need to free the JNI local references for name/jname here?

No we don't need to. doc

As I understand it, GetObjectArrayElement returns a jobject and when you are finished with this (on either success or error paths), you should call DeleteLocalRef on that object. There are other examples of this pattern already in the existing code-base, for example: https://github.com/facebook/rocksdb/blob/main/java/rocksjni/rocksjni.cc#L462

adamretter · 2024-03-13T08:24:33Z

java/rocksjni/portal.h

+      auto value = std::make_unique<jbyte[]>(jvalue_len);
+      env->GetByteArrayRegion(j_value, 0, jvalue_len, value.get());
+      if (env->ExceptionCheck()) {
+        return;


Do we need to free the JNI local references for name/jname and value/jvalue here?

No we don't need to. GetByteArrayRegion only copies data from java arrays to provide memory. This memory is allocated as a smart pointer and is automatically released when we exit the method.

adamretter · 2024-03-13T08:29:39Z

java/rocksjni/rocksjni.cc

+      auto value = reinterpret_cast<char*>(_value) + valuesOffset[i];
+      valuesRequiredSize[i] = static_cast<jint>(column.value().size());
+      max_data_len = std::min(valuesRequiredSize[i], (valuesLen[i]));
+      std::memcpy(value, column.value().data(), max_data_len);


Is explicit memcpy needed?

Yes, See previous comment

adamretter · 2024-03-13T08:29:48Z

java/rocksjni/rocksjni.cc

+    env->SetIntArrayRegion(jnamesRequiredSize, 0, max_items,
+                           namesRequiredSize.get());
+    if (env->ExceptionCheck()) {
+      return nullptr;


Do we need to free the previous JNI array fetches and local-references here?

No we don't need to. See previous comment

adamretter · 2024-03-13T08:29:53Z

java/rocksjni/rocksjni.cc

+    env->SetIntArrayRegion(jvaluesRequiredSize, 0, max_items,
+                           valuesRequiredSize.get());
+    if (env->ExceptionCheck()) {
+      return nullptr;


Do we need to free the previous JNI array fetches and local-references here?

No we don't need to. See previous comment

adamretter · 2024-03-13T08:30:35Z

java/rocksjni/rocksjni.cc

+        "Invalid key argument (argument is not a valid direct ByteBuffer)");
+    return nullptr;
+  }
+  auto key = reinterpret_cast<char*>(_key) + key_offset;


Could this use portal.h for string conversion

No. See previous comment

adamretter · 2024-03-13T08:32:14Z

java/rocksjni/rocksjni.cc

+                                        jlong jcf_handle) {
+  auto* db = reinterpret_cast<ROCKSDB_NAMESPACE::DB*>(jdb_handle);
+
+  auto key = std::make_unique<jbyte[]>(keyLength);


std::make_unique seems superfluous here

Yes, See previous comment

test-wangxiaoyu · 2024-03-27T01:03:40Z

Hello, after I finished compiling, the run encountered the following error：
librocksdbjni-osx-x86_64.jnilib was not found inside JAR.
My machine environment is macbook air 2022，The compiled command is as follows：
make clean jclean
DEBUG_LEVEL=0 make -j12 rocksdbjava

rhubner · 2024-03-27T15:56:03Z

Hello @test-wangxiaoyu

librocksdbjni-osx-x86_64.jnilib was not found inside JAR.

This is a very strange error. Considering you have a macbook air 2022, these models have ARM CPU (
Mac computers with Apple silicon ) and Java is trying to search for Intel version of native library. My suggestion is to check that you are running native ARM version of Java. Sometimes also called aarch64. From the error message it looks like you are running Intel version of Java via Rosetta emulation layer. You can download correct version from Adoptium project or Liberica

You can also check what native library is inside the jar archive. For example this is how it looks like on my linux machine.

$ jar -t --file java/target/rocksdbjni-9.2.0-linux64.jar | grep librocksdbjni
librocksdbjni-linux64.so

Radek

adamretter

LGTM

adamretter · 2024-07-02T12:35:22Z

@pdillinger @ajkr Can we get this one merged please?

nomed · 2024-09-18T19:18:57Z

Hi,

Thank you for the great work on this feature! I am very interested in using wide columns in RocksDB. Could you please let me know if this feature is planned to be merged? Also, do you have an estimated timeline for when it might be available in a release?

Thank you!

facebook-github-bot added the CLA Signed label Feb 7, 2024

rhubner force-pushed the eb/wide-columns branch 2 times, most recently from 33d9052 to a0a634e Compare February 9, 2024 04:26

rhubner force-pushed the eb/wide-columns branch 2 times, most recently from 47a2294 to 31e973b Compare March 5, 2024 14:07

rhubner force-pushed the eb/wide-columns branch from 31e973b to 6b75fb2 Compare March 12, 2024 09:18

adamretter self-requested a review March 13, 2024 08:18

adamretter added the java-api label Mar 13, 2024

adamretter requested changes Mar 13, 2024

View reviewed changes

rhubner force-pushed the eb/wide-columns branch 5 times, most recently from 239f9df to a9f7753 Compare March 20, 2024 08:22

rhubner force-pushed the eb/wide-columns branch from a9f7753 to aa1fd9a Compare March 25, 2024 16:20

rhubner marked this pull request as ready for review March 25, 2024 17:16

rhubner mentioned this pull request Mar 26, 2024

Java API for GetEntityFromBatch #12482

Draft

rhubner force-pushed the eb/wide-columns branch from aa1fd9a to 55716e0 Compare March 28, 2024 07:24

rhubner force-pushed the eb/wide-columns branch from 55716e0 to af9d0db Compare June 19, 2024 12:05

adamretter self-requested a review July 2, 2024 12:33

adamretter approved these changes Jul 2, 2024

View reviewed changes

rhubner force-pushed the eb/wide-columns branch from af9d0db to d8822e3 Compare July 2, 2024 12:37

rhubner force-pushed the eb/wide-columns branch from d8822e3 to 3a7a3ae Compare July 17, 2024 07:03

rhubner force-pushed the eb/wide-columns branch from 3a7a3ae to a427a7f Compare August 29, 2024 13:11

rhubner force-pushed the eb/wide-columns branch from a427a7f to 1c944f2 Compare September 27, 2024 05:21

Simple write.

f57cd27

rhubner added 14 commits September 30, 2024 21:23

Get/Put Entity for ByteBuffer.

f1a1a59

Use GetIntArrayRegion than GetIntArrayElements.

70d7fbd

Remove debug code, add tests.

9ffef85

Reformat code.

80707a8

Remove shadowed declaration.

cea6d7a

Fix PMD violations.

74c703d

Fix formatting, add Javadoc.

bb60ae0

Add wide column support for WriteBatch.

3620a0f

Add wide columns for WriteBatchWithIndex.

cc153b0

Refactor WideColumn JNI code.

4deb1a7

Fix PMD.

c8c43aa

Fix naming.

c47cf45

Fix JNI memory leak, add test for default column name.

4d671cd

Address PR comments, delete local references in loop.

e1d45d1

rhubner force-pushed the eb/wide-columns branch from 1c944f2 to e1d45d1 Compare September 30, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Wide columns in Java #12334

Add support for Wide columns in Java #12334

rhubner commented Feb 7, 2024 •

edited by adamretter

Loading

adamretter left a comment

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 25, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

adamretter Mar 13, 2024

rhubner Mar 13, 2024

test-wangxiaoyu commented Mar 27, 2024

rhubner commented Mar 27, 2024

adamretter left a comment

adamretter commented Jul 2, 2024

nomed commented Sep 18, 2024


		const auto columns_len = static_cast<int>(env->GetArrayLength(jNames));

		auto namesOffset = std::make_unique<jint[]>(columns_len);

Add support for Wide columns in Java #12334

Are you sure you want to change the base?

Add support for Wide columns in Java #12334

Conversation

rhubner commented Feb 7, 2024 • edited by adamretter 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

Choose a reason for hiding this comment

test-wangxiaoyu commented Mar 27, 2024

rhubner commented Mar 27, 2024

adamretter left a comment

Choose a reason for hiding this comment

adamretter commented Jul 2, 2024

nomed commented Sep 18, 2024

rhubner commented Feb 7, 2024 •

edited by adamretter

Loading