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

log2(0) panicked at 'attempt to subtract with overflow', parquet/src/util/bit_util.rs:148:5 #1901

Closed
HaoYang670 opened this issue Jun 17, 2022 · 3 comments · Fixed by #1905
Closed
Labels
bug parquet Changes to the parquet crate

Comments

@HaoYang670
Copy link
Contributor

HaoYang670 commented Jun 17, 2022

Describe the bug
https://github.com/apache/arrow-rs/blob/master/parquet/src/util/bit_util.rs#L142-L155

To Reproduce
assert_eq!(log2(0), 0);

Expected behavior
Alter 1. remove it and replaced by num_required_bits
Alter 2: return Some(n) when input > 0 and None when input == 0;
Alter 3: use the std function core::num::log2 which has not been stable yet. https://doc.rust-lang.org/std/primitive.u64.html#method.log2
Alter 4: panic when input == 0 (follow the behavior of num::core::log2)
Alter 5: return 0 when input == 0 (follow the behavior of num::core::log2)

Additional context
Add any other context about the problem here.

@HaoYang670 HaoYang670 added the bug label Jun 17, 2022
@jhorstmann
Copy link
Contributor

Since this seems to be only used for getting the width of bitpacked data, a faster way to calculate that is using the leading_zeros function. I contributed a similar improvements to parquet2 some time ago: https://github.com/jorgecarleitao/parquet2/blob/34aac65e433cff8bdd9fb07147cb551584a5415c/src/read/levels.rs#L3

@HaoYang670
Copy link
Contributor Author

Thank you @jhorstmann.
Agree with you on using leading_zeros, and this is also how the std library implements:

                #[inline]
                pub const fn log2(self) -> u32 {
                    Self::BITS - 1 - self.leading_zeros()
                }

But the corner case is what should the behavior of log2(0) be.

@HaoYang670
Copy link
Contributor Author

Ha! Maybe we could remove this function directly because we already have num_required_bits which provide same functionality.
https://github.com/apache/arrow-rs/blob/master/parquet/src/util/bit_util.rs#L180-L189

@paddyhoran paddyhoran added the parquet Changes to the parquet crate label Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants