-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat: Add support for rust-rocksdb storage engine #284
Conversation
Signed-off-by: Diwank Singh Tomer <[email protected]>
Comments / thoughts @zh217 ? |
I'm doing some testing and this is working for me and even fixed a bug with the rocksdb windows thread shutdown |
@keitharobertson I’m not sure why the tests are failing though. Haven’t had a chance to sit and debug. Maybe I missed something? |
} | ||
|
||
#[test] | ||
fn test_range_operations() -> Result<()> { |
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.
This test does not work for the old rocks implementation either. The problem is it is attempting to seek for the key with values (appended to the keys) included in the encoded prefix, which causes it to skip the first result. I can put a PR up with a fix, but it is unrelated to your implementation here.
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.
Gotcha
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.
Nice job finding bugs with the tests
Fix thanks to @keitharobertson Co-authored-by: Keith Robertson <[email protected]>
Thanks @keitharobertson! This is awesome. Committed your suggested changes. Another question is naming? @zh217 what do you think? |
Renaming the old one to cozorocks would be a breaking change so we probably don't want to do that. It is a different major version of rocks so maybe |
It won't be a breaking change at the cozoscript level though, right? I like rocksdb-9 naming scheme though. |
yeah- I think it would only matter in Cargo.toml |
Actually I think in |
https://crates.io/crates/rocksdb