-
-
Notifications
You must be signed in to change notification settings - Fork 28
Add prefix bloom filter support #151
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
Conversation
@marvin-j97 just want to make sure, did you want me to address the linter failures? they seem to be related to existing code
@marvin-j97 just want to make sure, did you want me to address the linter failures? they seem to be related to existing code
Linting seems to work now on the 3.0.0 branch, if you resolve the merge conflicts and pull, it should be fine.
e27b31c to
9d8b4f0
Compare
hmm: help: use'_for type paths | 298 | pub(crate) fn get_binary_index_reader(&self) -> BinaryIndexReader<'_> { | ++++
hmm clippy is still not happy but i haven't changed this stuff? am i basing this off the right branch?
hmm:
help: use'_for type paths | 298 | pub(crate) fn get_binary_index_reader(&self) -> BinaryIndexReader<'_> { | ++++hmm clippy is still not happy but i haven't changed this stuff? am i basing this off the right branch?
That's just a warning though, right? I haven't really gotten to running clippy lints over the entire code base, so there are a lot of warnings as of now.
going to take a closer look, also noticed some api's have changed so my tests are failing now, i'll fix that and test locally before pushing
5c502c3 to
7f70eeb
Compare
ok it was my fault, it was hidden with the other clippy warnings
cc303e0 to
8b995d4
Compare
9a4719b to
5e53adb
Compare
41781cc to
a1335f2
Compare
18921a8 to
768055f
Compare
768055f to
323c0b3
Compare
7fca57b to
f1381c0
Compare
f1381c0 to
b0a6bd8
Compare
1a9b4f4 to
b09d983
Compare
did i break the build and test lsm example build or some other isssue?
did i break the build and test lsm example build or some other isssue?
The CI examples script does not support raw .rs files in the examples/ folder, so it crashes.
b09d983 to
c09ffa3
Compare
did i break the build and test lsm example build or some other isssue?
The CI examples script does not support raw
.rsfiles in theexamples/folder, so it crashes.
should be fixed now, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking how to make use of this.
I guess this should be stored in each keyspace's config (in fjall) to check that the given prefix extractor matches the one that was initially configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, I may need to do more work but in rocksdb, if you want to change your prefix extractor then you need to use a different name which would cause rocksdb to ignore the old bloom that was used for the existing files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe the prefix extractor name also needs to be stored in every table's properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to do this and match rocksdb's behaviour:
"If prefix extractor changes when DB restarts, some SST files may contain prefix bloom filters generated using different prefix extractor than the one in current options. When opening those files, RocksDB will compare prefix extractor name stored in the properties of the SST files. If the name is different from the prefix extractor provided in the options, the prefix bloom filter is not used for this file"
one thing is how does fjall handle options that can be dynamically changeable vs can't be changed without a db restart? I don't think we would want a user to be able to change a prefix extractor on the fly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the configuration cannot be changed during runtime. However I don't think it would be a problem, because tables that use the old prefix extractor would continue using that, and otherwise the filter is skipped. But that's probably an edge case and just shouldn't be allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok cool, so then what I have here should be good.
I deleted the 3.0.0 branch, so this needs to rebased on main now
Uh oh!
There was an error while loading. Please reload this page.
resolve #97