-
Notifications
You must be signed in to change notification settings - Fork 38
feat: Implement Storable for tuples with three elements #195
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
Closed
dragoljub-djuric
wants to merge
25
commits into
main
from
EXC-1551-implement-storable-for-tuples-with-three-elements
Closed
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
e76a104
.
dragoljub-djuric b5c41f2
refactore bound calculation
dragoljub-djuric 5d15dae
.
dragoljub-djuric 93e0ef7
.
dragoljub-djuric 0dcdb57
.
dragoljub-djuric 5dc1876
.
dragoljub-djuric b68c993
add doc comment
dragoljub-djuric 12a3eb4
do not encode size when the type has fixed size
dragoljub-djuric 9c10234
Revert "do not encode size when the type has fixed size"
dragoljub-djuric 511be61
Fix comments
dragoljub-djuric 37b5cab
Fix comments
dragoljub-djuric 822b9d0
Change encoding of (A, B, C) to take smallest possible footprint
dragoljub-djuric 617ea80
.
dragoljub-djuric 21c2928
add encoding/decoding for unounded types
dragoljub-djuric 51d3c9c
.
dragoljub-djuric e5919e3
.
dragoljub-djuric 0043f45
refactor
dragoljub-djuric 5a92dbd
Merge branch 'main' into EXC-1551-implement-storable-for-tuples-with-…
dragoljub-djuric ee3ec62
Update src/storable.rs
dragoljub-djuric d58b37b
rename
dragoljub-djuric 3eeaf15
.
dragoljub-djuric e946631
rename
dragoljub-djuric 177a12d
refactor
dragoljub-djuric f9cddc2
rename
dragoljub-djuric 6432778
size of lenght of Unbounded types to 4 B
dragoljub-djuric File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
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.
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 been thinking about this way of encoding tuples. I see the following drawbacks:
If a user changes the
max_sizeof any of the types in the tuple, the number of bytes required to store the size can be different. The encoding will then possibly crash or (worse) return garbage. We used to say that you cannot change the max size once it's set, but we now allow it, since developers can now change themax_sizefromBoundedtoUnbounded.This is more of a nice-to-have, but leaves the door open for optimizations down the road:
Suppose
StorableforAis implemented in such a way such that, for any two elementsa_1anda_2,a_1.to_bytes() <op> a_2.to_bytes()iffa_1 <op> a_2, where<op>is any comparison operation (<, >, <=, >=, ==). This is just a fancy way of saying that two elements of typeAcan be compared just by comparing their bytes - we don't need to deserialize them and run thePartialEqfunction in Rust. This isn't an optimization that we do currently, but we can do in the future.If
A,B, andChave that property, it'd be very beneficial if(A, B, C)has that property as well. If this isn't clear, I'm happy to discuss over a call.In any case, here's my concrete suggestion so that we address the two points above:
For a tuple
(a, b, c), we store:<sizes_byte>is a byte added at the end that encodes the length of<size_a>and<size_b>.<sizes_byte>, we don't rely on themax_sizewhen decoding anymore, so developers can change themax_sizewithout breaking the decoding.Uh oh!
There was an error while loading. Please reload this page.
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.
Even more, if we want to make it work in that case we will do even worse. There is no way we can support such behavior with the current serialization of the tuple (A, B) when A and B are bounded because we rely on the Bounds of A and B to determine size. So we will be left with inconsistent behavior because changing the bound will be allowed for (A, B, C) but not for (A, B).
Did I misunderstand something?
A1 = "10", B1 = "1", A2 = "1", B2 = "11"
so if we have tuples (A1, B1), (A2, B2)
and we compare them (using >), expecting that we first compare the first element of the tuple followed by the second element first tuple will be bigger since A1 > A2, but (A1, B1).to_bytes() < (A2, B2).to_bytes() since the second bit in first tuple is "0" while in second is "1" and they have the same size.
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 is already a reality. The new btreemap was designed in such a way that allow developers to seamlessly upgrade and remove the bound on their types, and this is something we already support. If we now decide to not support this, we basically tell developers that they can't remove the bounds from their types and will have to create a new btreemap and migrate their data - it's too late to change that (nor is that best for a developer experience). I agree two-element tuples don't support it in its current form, but we can address that issue separately.
That's a good counter-example. Perhaps there isn't a way to preserve that byte-sorting property in the encoding schema.
Uh oh!
There was an error while loading. Please reload this page.
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 is implemented in PR, the approach was enough different so it was easier to do it from scratch.