feat: Add null bitmap concatenater #34914
Conversation
Signed-off-by: sunby <sunbingyi1992@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sunby The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
mergify
bot
added
dco-passed
|
@sunby Please associate the related issue to the body of your Pull Request. (eg. “issue: #”) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #34914 +/- ##
==========================================
+ Coverage 80.96% 80.97% +0.01%
==========================================
Files 1185 1182 -3
Lines 142715 143107 +392
==========================================
+ Hits 115544 115876 +332
- Misses 22760 22812 +52
- Partials 4411 4419 +8
|
| void | ||
| AddBitmap(uint8_t* bitmap, size_t size); | ||
|
|
||
| // Do not call this function multiple times |
There was a problem hiding this comment.
Can we make NullBitmapConcatenater a function instead of a class, then?
There was a problem hiding this comment.
Can we make
NullBitmapConcatenatera function instead of a class, then?
it's ok if we define a function called NullBitmapConcatenater but I prefer to use a class to manage allocated memory.
| for (const auto& bitmap : bitmaps_) { | ||
| if (bitmap.first == nullptr) { | ||
| // nullptr means nullable is false or no null value | ||
| // set offset to offset + bitmap.second bits are 0 |
There was a problem hiding this comment.
0 means the is not null right? null_bitmap_data taken from arrow, 0 means is null here.
There was a problem hiding this comment.
0 means the is not null right? null_bitmap_data taken from arrow, 0 means is null here.
oh yes, they use validity maps so 0 is invalid. I will fix it later.
|
by the way,is it necessary to provide isValid(index) or isNull(index) in bitmap concatenater? |
|
@sunby It might be a better idea to use the bitset library from |
Great! I did not notice it before. Maybe we should add more tests for this function and replace FixedVector<bool> in Column.h |
#34918