Add support for Wide columns in Java by rhubner · Pull Request #12334 · facebook/rocksdb

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.

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

+            "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

+
+    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

+      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

+      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

+      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

+    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

+    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

+        "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

+                                        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!

Make it exactly as CI says Which isn’t how it ends up if you try to `make format` I strongly suspect `make format` is subtly borked on Mac

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 2 times, most recently from 1c944f2 to e1d45d1 Compare September 30, 2024 17:23

alanpaxton force-pushed the eb/wide-columns branch from e1d45d1 to 9599cdf Compare May 19, 2025 08:25

rhubner force-pushed the eb/wide-columns branch from 3d0bd2c to 896c56a Compare September 3, 2025 20:31

rhubner and others added 17 commits November 24, 2025 20:29

Simple write.

d27380d

Get/Put Entity for ByteBuffer.

a50d1b4

Use GetIntArrayRegion than GetIntArrayElements.

ec2f759

Remove debug code, add tests.

23731c7

Reformat code.

a659477

Remove shadowed declaration.

2625458

Fix PMD violations.

56431c1

Fix formatting, add Javadoc.

21fd84e

Add wide column support for WriteBatch.

fbbec0d

Add wide columns for WriteBatchWithIndex.

eb0b53c

Refactor WideColumn JNI code.

2a7795b

Fix PMD.

5653e0b

Fix naming.

5afc7c0

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

026144b

Address PR comments, delete local references in loop.

1f1a811

Fix format

9f195b4

Manually fix format

40a7af2

Make it exactly as CI says Which isn’t how it ends up if you try to `make format` I strongly suspect `make format` is subtly borked on Mac

Chessray force-pushed the eb/wide-columns branch from 896c56a to 40a7af2 Compare November 24, 2025 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for Wide columns in Java#12334

Add support for Wide columns in Java#12334
rhubner wants to merge 17 commits into
facebook:mainfrom
evolvedbinary:eb/wide-columns

rhubner commented Feb 7, 2024 •

edited by adamretter

Loading

adamretter left a comment

Uh oh!

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

Labels

6 participants


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

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

Uh oh!

Conversation

rhubner commented Feb 7, 2024 • edited by adamretter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

adamretter left a comment

Choose a reason for hiding this comment

Uh oh!

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

Labels

6 participants

rhubner commented Feb 7, 2024 •

edited by adamretter

Loading