You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/30 19:33:19 UTC
[GitHub] [arrow-rs] boazberman opened a new pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
boazberman opened a new pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383
# Which issue does this PR close?
<!---
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Relates to https://github.com/apache/arrow-datafusion/issues/240 & https://github.com/apache/arrow-datafusion/pull/342
# Rationale for this change
<!---
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
`BooleanBufferBuilder` behaves as a bitmap. My intention is to extend its API to allow mutating bits already set, to make it's API closer to the `bitvec` crate and useable for my use-case in `arrow-datafusion`.
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
This PR includes the implementation and some tests that currently **does not** pass. I'm making the PR public to get help from the community, as I'm stuck.
# Are there any user-facing changes?
There is a new public method `set_bit` added to `BooleanBufferBuilder`.
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-851398299
Thanks @boazberman !
Here is the Slack link @Dandandan is referring to: https://the-asf.slack.com/archives/C01QUFS30TD/p1622403410176000
FWIW @tustvold implemented some version of this logic in https://github.com/influxdata/influxdb_iox/blob/main/arrow_util/src/bitset.rs in case you want to have a friendly look (and perhaps we can eventually use `BooleanBufferBuilder` rather than our own bitset creator,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] boaz-codota commented on a change in pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
boaz-codota commented on a change in pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#discussion_r645988790
##########
File path: arrow/src/array/builder.rs
##########
@@ -302,6 +302,15 @@ impl BooleanBufferBuilder {
self.len
}
+ #[inline]
+ pub fn set_bit(&mut self, index: usize, bit: bool) {
Review comment:
fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] boaz-codota commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
boaz-codota commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-856517323
hey, what is blocking this from being merged? @Dandandan @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-851059933
I added some details on the ASF Slack. I think the change is fine, but only the tests should be fixed.
Maybe it makes sense to expose `is_set` as well? This also could make the tests a bit more intuitive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan edited a comment on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
Dandandan edited a comment on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-851059933
I added some details on the ASF Slack. I think the change is fine, but only the tests should be fixed.
Maybe it makes sense to expose `get_bit` as well? This also could make the tests a bit more intuitive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-855284362
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#383](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d3b89d1) into [master](https://codecov.io/gh/apache/arrow-rs/commit/0a165748c924b3752cc00ff86e56917427985872?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0a16574) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #383 +/- ##
==========================================
+ Coverage 82.62% 82.65% +0.02%
==========================================
Files 162 162
Lines 44479 44542 +63
==========================================
+ Hits 36751 36814 +63
Misses 7728 7728
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [arrow/src/util/bit\_util.rs](https://codecov.io/gh/apache/arrow-rs/pull/383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL3V0aWwvYml0X3V0aWwucnM=) | `100.00% <ø> (ø)` | |
| [arrow/src/array/builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIucnM=) | `85.90% <100.00%> (+0.61%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [2ddc717...d3b89d1](https://codecov.io/gh/apache/arrow-rs/pull/383?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-855380675
> which I haven't tested but might have performance implications (?)
Hopefully the compiler can figure it out -- I think the way you have it is fine until we get some sort of profiling / performance measurements that show it isn't. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-856518795
> hey, what is blocking this from being merged? @Dandandan @alamb
Merged! Thank you 🚀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-856866662
> hey, what is blocking this from being merged? @Dandandan @alamb
Lack of time :) Sorry about that @boaz-codota -- thanks for following up and thanks for merging @Dandandan 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] boaz-codota commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
boaz-codota commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-855430342
@alamb fixed clippy failures
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-851395966
FYI @tustvold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan commented on a change in pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#discussion_r645988558
##########
File path: arrow/src/array/builder.rs
##########
@@ -302,6 +302,15 @@ impl BooleanBufferBuilder {
self.len
}
+ #[inline]
+ pub fn set_bit(&mut self, index: usize, bit: bool) {
Review comment:
```suggestion
pub fn set_bit(&mut self, index: usize, v: bool) {
```
(as called in other methods)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Dandandan merged pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
Dandandan merged pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] boaz-codota edited a comment on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
boaz-codota edited a comment on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-855236871
Fix the tests, could you review? @Dandandan @alamb ?
Also, @alamb I noticed that in https://github.com/influxdata/influxdb_iox/blob/main/arrow_util/src/bitset.rs#L87 is differ from what is done here:
```rust
// influx_iox
data[i >> 3] |= 1 << (i & 7)
```
vs
```rust
// arrow-rs
const BIT_MASK: [u8; 8] = [1, 2, 4, 8, 16, 32, 64, 128];
// ...
data[i >> 3] |= BIT_MASK[i & 7];
```
which I haven't tested but might have performance implications (?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] boaz-codota commented on pull request #383: Add set_bit to BooleanBufferBuilder to allow mutating bit in index
Posted by GitBox <gi...@apache.org>.
boaz-codota commented on pull request #383:
URL: https://github.com/apache/arrow-rs/pull/383#issuecomment-855236871
Fix the tests, could you review? @Dandandan @alamb ?
Also, @alamb I noticed that in https://github.com/influxdata/influxdb_iox/blob/main/arrow_util/src/bitset.rs#L87 is differ from what is done here:
```rust
// influx_iox
data[byte_idx] |= 1 << (i & 7)
```
vs
```rust
// arrow-rs
const BIT_MASK: [u8; 8] = [1, 2, 4, 8, 16, 32, 64, 128];
// ...
data[i >> 3] |= BIT_MASK[i & 7];
```
which I haven't tested but might have performance implications (?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org