Skip to content
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

Fix stored relation prefix_join on key range #286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keitharobertson
Copy link
Contributor

  • The stored relation (both with and without validity) was incorrectly using non-key values when seeking for the start key in a range resulting in the first key that should've been returned being skipped.
  • For example, in the case of a stored relation defined as *r{k => v}, a query for *r{k, v}, k >= 3 would skip over a key with value 3 because it will do a join with the key encoding of [3, null] where null is for the value of v.
  • In the prefix join, only the keys should be considered, not the value columns so truncate the bindings to the length of the keys to exclude values.

@keitharobertson
Copy link
Contributor Author

@zh217 @creatorrr would love a review. Change itself is very small and tests are copied from @creatorrr new-rocksdb work over to the old cozorocks implementation.

* The stored relation (both with and without validity) was incorrectly using non-key values when seeking for the start key in a range resulting in the first key that should've been returned being skipped.
* For example, in the case of a stored relation defined as `*r{k => v}`, a query for `*r{k, v}, k >= 3` would skip over a key with value 3 because it will do a join with the key encoding of [3, null] where null is for the value of v.
* In the prefix join, only the keys should be considered, not the value columns so truncate the bindings to the length of the keys to exclude values.
@creatorrr
Copy link
Contributor

Lgtm!

@creatorrr
Copy link
Contributor

Did the main tests also pass? @keitharobertson

@keitharobertson
Copy link
Contributor Author

Did the main tests also pass? @keitharobertson

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants