-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
ARROW-11040: [Rust] Simplified builders #9019
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9019 +/- ##
==========================================
+ Coverage 82.55% 82.61% +0.05%
==========================================
Files 203 203
Lines 50043 49942 -101
==========================================
- Hits 41315 41259 -56
+ Misses 8728 8683 -45
Continue to review full report at Codecov.
|
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.
Nice simplification, I think this is also a good example of how to play with different offset types in a single place 👍
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.
Thanks @jorgecarleitao for the simplification
rust/arrow/src/array/builder.rs
Outdated
@@ -779,140 +781,31 @@ where | |||
let offset_buffer = self.offsets_builder.finish(); | |||
let null_bit_buffer = self.bitmap_builder.finish(); | |||
let nulls = null_bit_buffer.count_set_bits(); |
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 just noticed that we had named this unintuitively, as we're counting the set bits
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.
unset
? it proves the point, though 🤣
FYI, @nevi-me , the tests are not running (INFRA team deactivated most actions), so we need to be careful with merging to master. There is a thread in the mailing list about it.
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.
Okay, thanks for the heads-up. I'll check my emails
The full set of Rust CI tests did not run on this PR :( Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do? I apologize for the inconvenience. |
This PR simplifies the builders code. It has no semantic, execution or API change. The main idea here is to generalize `[Large]ListBuilder`, `[Large]StringBuilder`, `[Large]BinaryBuilder` as `GenericListBuilder`, `GenericStringBuilder` and `GenericBinaryBuilder` respectively, thereby removing duplicated code. The relevant changes in this PR are on `src/array/builders.rs` only. Closes apache#9019 from jorgecarleitao/generic_list Authored-by: Jorge C. Leitao <jorgecarleitao@gmail.com> Signed-off-by: Jorge C. Leitao <jorgecarleitao@gmail.com>
This PR simplifies the builders code. It has no semantic, execution or API change.
The main idea here is to generalize
[Large]ListBuilder
,[Large]StringBuilder
,[Large]BinaryBuilder
asGenericListBuilder
,GenericStringBuilder
andGenericBinaryBuilder
respectively, thereby removing duplicated code.The relevant changes in this PR are on
src/array/builders.rs
only.