-
Notifications
You must be signed in to change notification settings - Fork 727
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
Update Bitmap::len to return bits rather than bytes #749
Conversation
@jorgecarleitao FYI. I did what i thought was the simplest update by simply multiplying bytes by 8 and updating when running all let me know your thoughts :) |
Looks a good start. I think that every |
The errors were actually just because i needed to add the submodules. I see you already approved - do you still need me to check if |
No, checks pass, it should be good to go. Thank you for your contribution! |
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.
Thank you @matthewmturner -- I think this does what is called for in #744 👍
@jorgecarleitao perhaps you can have a look as well?
(edit -- missed that Jorge had already reviewed)
I also looked at the uses of Bitmap::len
in the arrow crate and it only appears to be used in tests:
/Users/alamb/Software/arrow-rs/arrow/src/bitmap.rs
120: assert_eq!(64, Bitmap::new(63 * 8).len());
121: assert_eq!(64, Bitmap::new(64 * 8).len());
122: assert_eq!(128, Bitmap::new(65 * 8).len());
One concern I have is related to backwards compatibility -- if we change len
to return the length in bits but change nothing else, the rustc
compiler will not alert any users of this function that the API has changed.
What about also renaming Bitmap::len()
to Bitmap::byte_len()
to make it super clear what is going on as well as generating compiler errors on upgrade?
@@ -45,7 +45,7 @@ impl Bitmap { | |||
} | |||
|
|||
pub fn len(&self) -> usize { |
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.
Can we also add a docstring here that explains what Bitmap::len
actually returns (aka the length in bits, not bytes)?
I would not consider this backward incompatibility but a bug fix: imo |
Which issue does this PR close?
Closes #730
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?