Skip to content

Make BooleanBufferBuilder get_bit not require mutable reference#784

Merged
Dandandan merged 1 commit into
apache:masterfrom
boazberman:master
Sep 19, 2021
Merged

Make BooleanBufferBuilder get_bit not require mutable reference#784
Dandandan merged 1 commit into
apache:masterfrom
boazberman:master

Conversation

@boazberman

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

This PR is a followup to #693 in which I did the insanely stupid mistake of requiring a mutable reference instead of immutable reference. This PR fixes this (and adds a test to validate it...).

This is required to finish apache/datafusion#884

Closes #.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions Bot added the arrow Changes to the arrow crate label Sep 18, 2021
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Merging #784 (c4c58b3) into master (499301c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #784   +/-   ##
=======================================
  Coverage   82.54%   82.54%           
=======================================
  Files         168      168           
  Lines       47912    47917    +5     
=======================================
+ Hits        39548    39554    +6     
+ Misses       8364     8363    -1     
Impacted Files Coverage Δ
arrow/src/array/builder.rs 86.23% <100.00%> (+0.04%) ⬆️
parquet/src/encodings/encoding.rs 94.67% <0.00%> (+0.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 499301c...c4c58b3. Read the comment docs.

@nevi-me nevi-me left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

}

#[test]
fn test_bool_buffer_builder_get_first_bit_not_requires_mutability() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you could omit this test, because the function taking an immutable reference is enough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Given that I managed to do the fuckup of requiring a mutable reference, I'd prefer to keep this test to prevent the possibility of this happening in the future (although unlikely). I hope you are OK with this.

@Dandandan Dandandan merged commit 615d783 into apache:master Sep 19, 2021
@Dandandan

Copy link
Copy Markdown
Contributor

Thanks @boazberman

1 similar comment
@Dandandan

Copy link
Copy Markdown
Contributor

Thanks @boazberman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants