Java Load correct filter policy into table options #13336
Draft
+65
−29
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OptionsUtilTest was using a broken assertThat, and a check of the loaded filter policy was not happening. Correcting the test exposed that the filter policy was not being correctly loaded into the Java side `BlockBasedTableOptions`/`BlockBasedTableConfig`. Fix this by constructing the Java `Filter` (should be renamed `FilterPolicy` for consistency, but that is too hard) from C++ in the same way as any other Java object via JNI, and installing it in the `BlockBasedTableConfig`. Subtlety 1 is that the handle for a `FilterPolicy` is a `std::shared_ptr<FilterPolicy>` so we need to allocate a new one which shares control block with the shared_ptr stored in corresponding C++ `BlockBasedTableOptions`
rhubner
reviewed
Jan 29, 2025
| double filter_bits = 0.0; | ||
| if (const ROCKSDB_NAMESPACE::BloomLikeFilterPolicy* bloomlike_filter = | ||
| dynamic_cast<const BloomLikeFilterPolicy*>(sptr_filter->get())) { | ||
| filter_bits = bloomlike_filter->GetMillibitsPerKey() / 1000.0; |
There was a problem hiding this comment.
I slightly don't like to store filter_bits in Java when it's also in C++. But I understand that we already have this in code and probably there are reasons. Maybe hashCode & equals methods must be fast in Java and should not cross native code boundary to read filter_bits.
|
Just one comment(maybe opinion). Otherwise LGTM ✅ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OptionsUtilTest was using a broken assertThat, and a check of the loaded filter policy was not happening. Correcting the test exposed that the filter policy was not being correctly loaded into the Java side
BlockBasedTableOptions/BlockBasedTableConfig.Fix this by constructing the Java
Filter(should be renamedFilterPolicyfor consistency, but that is too hard) from C++ in the same way as any other Java object via JNI, and installing it in theBlockBasedTableConfig. Subtlety 1 is that the handle for aFilterPolicyis astd::shared_ptr<FilterPolicy>so we need to allocate a new one which shares control block with the shared_ptr stored in corresponding C++BlockBasedTableOptions