-
Notifications
You must be signed in to change notification settings - Fork 254
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: enhance binary array encoding, make it the default #2521
feat: enhance binary array encoding, make it the default #2521
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2521 +/- ##
==========================================
+ Coverage 79.45% 79.81% +0.36%
==========================================
Files 208 207 -1
Lines 58863 58806 -57
Branches 58863 58806 -57
==========================================
+ Hits 46768 46935 +167
+ Misses 9357 9129 -228
- Partials 2738 2742 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I think we can merge this as soon as #2508 is merged in
…ical binary encoder
a399560
to
4b79e33
Compare
This adds support for the following things to the binary encoding:
In addition, I have changed the row limit on pages from
u32
tou64
. Partly, this is because string arrays larger that 2GB are not unheard of and it may be nice in some cases to store them in one page. Also pages are infrequent enough that an extra 4 bytes is trivial (actually, protobuf already stores u32 and u64 the same). Another reason is that, with some encodings (e.g. constant / compressed bitmap / etc.) it's possible to have 2^64 rows without occupying a large amount of space.Note: this is technically a
.proto
change but in protobufu32 -> u64
is backwards compatible so it is not a huge issue.