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

Change Key::from_ord_bytes to accept Cow #254

Closed
ecton opened this issue May 16, 2022 · 2 comments · Fixed by #277
Closed

Change Key::from_ord_bytes to accept Cow #254

ecton opened this issue May 16, 2022 · 2 comments · Fixed by #277
Labels
enhancement New feature or request
Milestone

Comments

@ecton
Copy link
Member

ecton commented May 16, 2022

From a conversation with @asonix on Discord, a suggestion was made to pass Cow<'k, [u8]> instead of &'k [u8]. After some thought, I realized this was a wonderful idea even though it initially sounds like it complicates things.

One of the ugly parts of the to-be-released CompositeKeyEncoder is that it doesn't support values that contain null bytes. The reason is that borrowed data couldn't be supported properly in those situations due needing to escape the data, which necessarily requires extra allocations that cannot be borrowed from. By passing the Cow instead of always giving a slice, it gives the implementor of Key the option of implementing either version or returning an error if it can't fulfill the deserialization given the lifetime requirements.

@ecton ecton added the enhancement New feature or request label May 16, 2022
@ecton ecton added this to the v0.5.0 milestone May 16, 2022
@ecton
Copy link
Member Author

ecton commented Feb 28, 2023

Unfortunately this isn't possible to support while still supporting composite key decoding. The reason is quite complicated, but it boils down to this:

With from_ord_bytes(bytes: Cow<'a, [u8>), any implementor that attempts to borrow from bytes must borrow from a local variable, since the only way to access bytes as a &[u8] is to either use the lifetime of bytes or needs to only match Cow::Borrowed.

This causes all sorts of issues when trying to apply these changes to composite key decoding, since it requires using only part of the input bytes for each field. Additionally, it has performance implications with Option/Result encoding due to those types encoding a single extra byte.

If anyone wants to try their hands at fixing the issues on this branch: https://github.com/khonsulabs/bonsaidb/compare/a9c6af0dc161daaa41ced39b5822f3d63f219039...ecton:cow_key?expand=1, it should be fairly straightfoward to reproduce the errors. There needs to be an update to arc-bytes for one type -- if that hasn't happened by the time someone plays with this, replacing that From implementation with a todo!() should allow the lifetime errors to be the only remaining issues.

@ecton ecton closed this as completed Feb 28, 2023
@asonix asonix mentioned this issue Mar 2, 2023
@ecton
Copy link
Member Author

ecton commented Mar 2, 2023

After thinking about the approach @asonix proposed in #277, I'm reopening this for another attempt.

@ecton ecton reopened this Mar 2, 2023
@ecton ecton closed this as completed in #277 Mar 7, 2023
ecton added a commit that referenced this issue Mar 21, 2023
After #254, null bytes no longer need to be either allow/deny. Instead,
we can now escape which is the default behavior. This will cause extra
allocations when null bytes are encountered during the encoding process,
but the correct sort behavior is preserved.

Allow/deny is still an option for situations where users do not want the
extra overhead that encoding can incur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant