perf: bitpacking exclude sign bit if not needed #2696
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2696 +/- ##
==========================================
- Coverage 79.52% 79.41% -0.12%
==========================================
Files 227 227
Lines 66754 66880 +126
Branches 66754 66880 +126
==========================================
+ Hits 53089 53112 +23
- Misses 10538 10637 +99
- Partials 3127 3131 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
albertlockett
changed the title
There was a problem hiding this comment.
If I wanted to be super annoying (which I usually do) I would point out that you also don't need the signed bit if all values are negative 😆 (but you would need to change BitpackParams::signed into enum Signedness { AllNegative, Mixed, AllPositive }.
Please feel free to defer to follow-up or even "good first issue" for someone else in the future.
| return min_leading_bits | ||
| // +1 added here for the sign bit | ||
| .map(|leading_bits| arr.data_type().byte_width() as u64 * 8 - leading_bits + 1); | ||
| .map(|leading_bits| arr.data_type().byte_width() as u64 * 8 - leading_bits) | ||
| .map(|bits| bits + if add_signed_bit { 1 } else { 0 }) | ||
| .map(|bits| bits.max(1)) // cannot bitpack into < 1 bit | ||
| .map(|bits| BitpackParams { | ||
| num_bits: bits, | ||
| signed: add_signed_bit, | ||
| }); |
There was a problem hiding this comment.
Very minor nit: All this mapping is fine but you could avoid it and do something like...
let mut min_leading_bits = arr.data_type().byte_width() as u64 * 8 - min_leading_bits?;
if add_signed_bit {
// Need extra sign bit
min_leading_bits += 1;
}
// cannot bitpack into <1 bit
let num_bits = min_leading_bits.max(1);
Some(BitpackParams {
num_bits,
signed: add_signed_bit
})
This might be slightly more readable.
If bitpacking a signed type (e.g. i32), if all the values to be encoded are positive then there's no need to include the sign bit.