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

feat: support bitpacking for signed types #2662

Merged
merged 4 commits into from
Aug 2, 2024
Merged

Conversation

albertlockett
Copy link
Contributor

Adds support for bitpacking for signed types Int8, Int16, Int32 and Int64. In this case, we use one extra bit in the encoding for as the sign bit, and when decoding pad using 1 instead of 0 when expanding the bitpacked buffer

@github-actions github-actions bot added the enhancement New feature or request label Jul 30, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.59%. Comparing base (89ad237) to head (35e3862).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2662      +/-   ##
==========================================
+ Coverage   79.56%   79.59%   +0.03%     
==========================================
  Files         226      226              
  Lines       66424    66516      +92     
  Branches    66424    66516      +92     
==========================================
+ Hits        52847    52945      +98     
+ Misses      10476    10472       -4     
+ Partials     3101     3099       -2     
Flag Coverage Δ
unittests 79.59% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rely on the tests to cover the actual bit math but overall this looks reasonable

Some(BitpackingBufferEncoder::new(num_bits))
Some(BitpackingBufferEncoder::new(
num_bits,
!data_type.is_unsigned_integer(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge deal but we could probably get away with only adding the sign bit if there are actually negative values in the page. For example, a user might use i32 because they think they might need support for negatives someday but then never actually store any negative values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I can add this in a follow-up

@albertlockett albertlockett force-pushed the bitpack_signed branch 2 times, most recently from cb59216 to 288e1fd Compare August 2, 2024 20:19
@albertlockett albertlockett merged commit d141cc8 into main Aug 2, 2024
24 of 25 checks passed
@albertlockett albertlockett deleted the bitpack_signed branch August 2, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants