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

Add support for encoding/decoding with bincode. #45

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gz
Copy link

@gz gz commented Sep 27, 2022

This PR adds optional support for serialization of ArcStr with bincode (similar to how it already exists for serde).

This avoids external projects which use arcstr and bincode having to wrap the ArcStr type to implement bincode::Decode and bincode::Encode for it.

@thomcc
Copy link
Owner

thomcc commented Sep 27, 2022

I'm willing to add this once there's a public release of bincode 2.0.0, but not before. It may be a semver-breaking change for me to move from 2.0.0-rc.1 to 2.0.0, and I don't want to make a major version bump of arcstr because of this.

@codecov-commenter
Copy link

Codecov Report

Base: 96.33% // Head: 95.56% // Decreases project coverage by -0.76% ⚠️

Coverage data is based on head (ea2b03e) compared to base (f183959).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
- Coverage   96.33%   95.56%   -0.77%     
==========================================
  Files           7        8       +1     
  Lines         873      880       +7     
==========================================
  Hits          841      841              
- Misses         32       39       +7     
Impacted Files Coverage Δ
src/impl_bincode.rs 0.00% <0.00%> (ø)
src/lib.rs 50.00% <ø> (-50.00%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@thomcc
Copy link
Owner

thomcc commented Sep 27, 2022

This should also be implemented for Substr, and needs tests. That said, I wouldn't blame you on waiting on this given my other comments.

@gz
Copy link
Author

gz commented Sep 27, 2022

Sounds good to me, while we're waiting for v2, I will work on adding support for Substr + tests.

Also added tests for bincode, bump bincode version to new RC.
@gz
Copy link
Author

gz commented Oct 5, 2022

I added tests and implement traits for Substr too.

blp added a commit to feldera/feldera that referenced this pull request Jul 26, 2023
DBSP relies on bincode version 2.0.0-rc3, which was released at the end of
March 2023.  DBSP also uses arcstr and we want to serialize and
deserialize `ArcStr`s with bincode.  Unfortunately, arcstr doesn't support
bincode 2.0 yet and won't until bincode 2.0 finally gets released[1], which
could be a long time, so we have a patch in Cargo.toml to support that.

We'd like to publish DBSP to crates.io[2], but we can't if it depends on
patches, so it seems best to avoid them.  This commit drops the patch and
switches to using #[bincode(with_serde)] to serialize ArcStr fields.

[1]: thomcc/arcstr#45
[2]: #14

Signed-off-by: Ben Pfaff <blp@feldera.com>
Fixes: #395
blp added a commit to feldera/feldera that referenced this pull request Jul 26, 2023
DBSP relies on bincode version 2.0.0-rc3, which was released at the end of
March 2023.  DBSP also uses arcstr and we want to serialize and
deserialize `ArcStr`s with bincode.  Unfortunately, arcstr doesn't support
bincode 2.0 yet and won't until bincode 2.0 finally gets released[1], which
could be a long time, so we have a patch in Cargo.toml to support that.

We'd like to publish DBSP to crates.io[2], but we can't if it depends on
patches, so it seems best to avoid them.  This commit drops the patch and
switches to using #[bincode(with_serde)] to serialize ArcStr fields.

[1]: thomcc/arcstr#45
[2]: #14

Signed-off-by: Ben Pfaff <blp@feldera.com>
Fixes: #395
@thomcc thomcc added the blocked label May 7, 2024
@thomcc
Copy link
Owner

thomcc commented May 7, 2024

Update May 2024: Still blocked on v2.0 of bincode.

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

Successfully merging this pull request may close these issues.

3 participants