Add Filter::CreateFromString to Java #387

mrambacher · 2023-02-07T00:22:00Z

This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.

udi-speedb · 2023-02-07T06:41:54Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

mrambacher · 2023-02-07T12:37:41Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

udi-speedb · 2023-02-07T12:39:20Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

udi-speedb · 2023-02-28T04:12:46Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

java/src/main/java/org/rocksdb/ConfigOptions.java

udi-speedb · 2023-02-28T05:49:29Z

java/rocksjni/portal.h

+      ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, status);
+      return kStatusError;
+    }
+  }


The function returns, in case of errors, sometimes negative error codes instead of valid pointers.
Is it OK because an exception is thrown and, therefore, the status is never actually returned (just a way to let the Java code "compile")?
It's confusing IMO. Why not return a nullptr converted to jlong or 0 (as in Java_org_rocksdb_BackupEngineOptions_newBackupEngineOptions) to show the reader that it's a null pointer?
You can also document the reason for returning the "null pointer" (the reason for the error).

The function returns, in case of errors, sometimes negative error codes instead of valid pointers. Is it OK because an exception is thrown and, therefore, the status is never actually returned (just a way to let the Java code "compile")? It's confusing IMO. Why not return a nullptr converted to jlong or 0 (as in Java_org_rocksdb_BackupEngineOptions_newBackupEngineOptions) to show the reader that it's a null pointer? You can also document the reason for returning the "null pointer" (the reason for the error).

I was following the example used by the DB code where a Status DB::Put() would throw an exception based on the Status code. If there is a different example that makes more sense, I am willing to follow it instead.

I just gave one in my comment above: Java_org_rocksdb_BackupEngine_open
And others: Java_org_rocksdb_BackupEngine_getCorruptedBackups

I do not think it matters since these values should not be seen from the exceptions. If you prefer they all return 0 rather than a negative number, I will make that change.

I do. Thanks. I think it makes the code clearer.

udi-speedb · 2023-02-28T05:55:17Z

java/rocksjni/portal.h

+    cfg_opts.ignore_unknown_options = false;
+    cfg_opts.invoke_prepare_options = true;
+    return createSharedFromString<R, T>(cfg_opts, env, s);
+  }


These fields have defaults on the cpp side. Why override them here as you do? Aren't those defaults (the cpp ones defined for ConfigOptions) adequate?

I still do not understand why do you need to explicitly override the cpp defaults:

// When true, any unused options will be ignored and OK will be returned bool ignore_unknown_options = false; // When true, any unsupported options will be ignored and OK will be returned bool ignore_unsupported_options = true; // If the strings are escaped (old-style?) bool input_strings_escaped = true; // Whether or not to invoke PrepareOptions after configure is called. bool invoke_prepare_options = true;

The only value that is different than the default is "ignore_unsupported_options=false". The rest match the default. This value would be nice to default to false, but would cause a lot of old code to fail. I could not make "unsupported=false" for historical reasons; since Java does not have this history, we can turn this off here.

I think it's best to have the JAVA default be the same as the CPP defaults. That way JAVA users get the same behaviour as cpp users by default.
Shouldn't you just avoid overriding the defaults at all (and have the defaults be defaults)?

@udi-speedb I can change it so the defaults match the C++. However, I am not certain that is what is desired in this case. The C++ ones are what they are to support older settings. For example, the default for "ignore_unsupported_options=true" would mean that attempting to create a filter that does not exist may return OK when it should return an error. This is a historical artifact as C++ behaved this way before the new infrastructure was added and needs to for compatibility. Java -- having no such compatibility issues, since this is new code -- can return errors properly under those conditions.

If you still feel strongly about it, I can make the defaults match. However, I do not know I agree that is the right thing to do.

udi-speedb · 2023-02-28T14:10:08Z

java/src/test/java/org/rocksdb/FilterTest.java

+  public void createUnknownFromString() throws RocksDBException {
+    try (final Filter filter = Filter.createFromString("unknown")) {
+    }
+  }


Please add a test to create our filter as well. This is both expected IMO (as this is our new type) and will also test our ability to create a plugin using the JNI.

@mrambacher - Could you please somehow address this comment?

@mrambacher - Could you please somehow address this comment?

@udi-speedb There is a test added under plugin/speedb/java for the Speedb-specific test.

java/src/test/java/org/rocksdb/FilterTest.java

Add a unit test for the Speedb filters.

udi-speedb · 2023-03-12T03:31:39Z

java/rocksjni/portal.h

+      ROCKSDB_NAMESPACE::RocksDBExceptionJni::ThrowNew(env, status);
+      return kStatusError;
+    }
+  }


I just gave one in my comment above: Java_org_rocksdb_BackupEngine_open
And others: Java_org_rocksdb_BackupEngine_getCorruptedBackups

udi-speedb · 2023-03-12T03:37:18Z

java/rocksjni/portal.h

+    cfg_opts.ignore_unknown_options = false;
+    cfg_opts.invoke_prepare_options = true;
+    return createSharedFromString<R, T>(cfg_opts, env, s);
+  }


I still do not understand why do you need to explicitly override the cpp defaults:

// When true, any unused options will be ignored and OK will be returned bool ignore_unknown_options = false; // When true, any unsupported options will be ignored and OK will be returned bool ignore_unsupported_options = true; // If the strings are escaped (old-style?) bool input_strings_escaped = true; // Whether or not to invoke PrepareOptions after configure is called. bool invoke_prepare_options = true;

udi-speedb · 2023-03-12T03:39:42Z

@mrambacher - I have replied on the updates. Please note that there are still some comments that you haven't addressed.
Thanks

mrambacher · 2023-03-12T22:01:26Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

How else would you like this tested? I do not see where the the other filter code is tested in any more depth than this API.

udi-speedb · 2023-03-21T03:07:20Z

@mrambacher - Preliminary question: Do you have any means to check the change / run the FilterTest.java to verify it passes?

I ran "make jtest", which runs the FilterTest

OK. Do you think that's enough to feel comfortable releasing this or should we test it in some real java environment? I am asking since my knowledge of Java is very limited

@mrambacher - You haven't answered my question. Please do

@mrambacher - Could you please answer

udi-speedb · 2023-03-21T03:18:56Z

@mrambacher - Still some comments / questions . Thanks

Yuval-Ariel · 2023-04-17T08:31:07Z

@mrambacher , this can be merged but please squash the commits when you do and provide a descriptive commit msg

mrambacher · 2023-04-18T22:03:55Z

@mrambacher , this can be merged but please squash the commits when you do and provide a descriptive commit msg

@Yuval-Ariel I have no idea how to do a squash since my branch also contains updates merged from main. Is there a reason "squash and merge" is insufficient?

This PR adds the ability to create a Filter (FilterPolicy) in Java via the CreateFromString methodology. This change allows the other filter policies (such as Ribbon Filter and plugin filters) to be created in Java using the C++ infrastructure.

Add Filter::CreateFromString to Java

3ba05e4

mrambacher requested a review from udi-speedb February 7, 2023 00:22

mrambacher self-assigned this Feb 7, 2023

Add InstanceOf test for BloomFilter

75c9499

mrambacher added Upstreamable can be upstreamed to RocksDB usability labels Feb 7, 2023

mrambacher linked an issue Feb 7, 2023 that may be closed by this pull request

Expose the new Paired bloom filter via the Java APIs #327

Closed

mrambacher mentioned this pull request Feb 8, 2023

Add CreateFromString functionality to Java Filter class facebook/rocksdb#11205

Open

udi-speedb requested changes Feb 28, 2023

View reviewed changes

mrambacher added 3 commits March 7, 2023 11:14

Address PR Comments

d345c7a

Add a unit test for the Speedb filters.

Add a comment to CreateSharedFromString

50efe3e

Yet another make format

6ea8b9a

udi-speedb requested changes Mar 12, 2023

View reviewed changes

udi-speedb self-requested a review March 21, 2023 02:58

udi-speedb approved these changes Mar 21, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into JavaBloomFilter

33c8eb6

Yuval-Ariel merged commit 5817370 into speedb-io:main Apr 19, 2023

mrambacher mentioned this pull request Aug 22, 2023

Add JNI interface for Ribbon Filter facebook/rocksdb#10679

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Filter::CreateFromString to Java #387

Add Filter::CreateFromString to Java #387

mrambacher commented Feb 7, 2023

udi-speedb commented Feb 7, 2023

mrambacher commented Feb 7, 2023

udi-speedb commented Feb 7, 2023

udi-speedb commented Feb 28, 2023

udi-speedb Feb 28, 2023

mrambacher Mar 7, 2023

udi-speedb Mar 12, 2023

mrambacher Mar 13, 2023

udi-speedb Mar 21, 2023 •

edited

Loading

udi-speedb Feb 28, 2023

udi-speedb Mar 12, 2023

mrambacher Mar 12, 2023

udi-speedb Mar 21, 2023 •

edited

Loading

mrambacher Mar 21, 2023

udi-speedb Feb 28, 2023

udi-speedb Mar 21, 2023

mrambacher Mar 21, 2023

udi-speedb Mar 12, 2023

udi-speedb Mar 12, 2023

udi-speedb commented Mar 12, 2023

mrambacher commented Mar 12, 2023

udi-speedb commented Mar 21, 2023

udi-speedb commented Mar 21, 2023

Yuval-Ariel commented Apr 17, 2023

mrambacher commented Apr 18, 2023

Add Filter::CreateFromString to Java #387

Add Filter::CreateFromString to Java #387

Conversation

mrambacher commented Feb 7, 2023

udi-speedb commented Feb 7, 2023

mrambacher commented Feb 7, 2023

udi-speedb commented Feb 7, 2023

udi-speedb commented Feb 28, 2023

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

udi-speedb Mar 21, 2023 • 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

udi-speedb Mar 21, 2023 • 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

udi-speedb commented Mar 12, 2023

mrambacher commented Mar 12, 2023

udi-speedb commented Mar 21, 2023

udi-speedb commented Mar 21, 2023

Yuval-Ariel commented Apr 17, 2023

mrambacher commented Apr 18, 2023

udi-speedb Mar 21, 2023 •

edited

Loading

udi-speedb Mar 21, 2023 •

edited

Loading