Skip to content

Conversation

@metalmatze
Copy link
Contributor

@metalmatze metalmatze commented Aug 30, 2023

Rationale for this change

Similar to the changes we've made in #35744 we need this for the BooleanBuilder too.

What changes are included in this PR?

Adding the method itself and adjusting a test to test both the Value method on the builder and the final array.

Are these changes tested?

Yes

Are there any user-facing changes?

Just an enhancement to get on-par with other builders.

@kou
Copy link
Member

kou commented Aug 30, 2023

This is not a MINOR change.
See https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes for our MINOR definition.
(The link is also included in PR template you seen when you open a PR.)

Could you open a new issue for this?

@metalmatze metalmatze changed the title MINOR: [Go] Add Value method to BooleanBuilder GH-37465: [Go] Add Value method to BooleanBuilder Aug 30, 2023
@github-actions
Copy link

⚠️ GitHub issue #37465 has been automatically assigned in GitHub to PR creator.

@brancz
Copy link
Contributor

brancz commented Aug 31, 2023

cc @zeroshade

@zeroshade zeroshade merged commit d7b4d2d into apache:main Aug 31, 2023
@zeroshade zeroshade removed the awaiting review Awaiting review label Aug 31, 2023
@github-actions github-actions bot added the awaiting merge Awaiting merge label Aug 31, 2023
@metalmatze metalmatze deleted the binarybuilder-value branch August 31, 2023 15:40
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit d7b4d2d.

There was 1 benchmark result indicating a performance regression:

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
### Rationale for this change

Similar to the changes we've made in apache#35744 we need this for the BooleanBuilder too.

### What changes are included in this PR?

Adding the method itself and adjusting a test to test both the Value method on the builder and the final array.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Just an enhancement to get on-par with other builders.

* Closes: apache#37465

Authored-by: Matthias Loibl <mail@matthiasloibl.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
### Rationale for this change

Similar to the changes we've made in apache#35744 we need this for the BooleanBuilder too.

### What changes are included in this PR?

Adding the method itself and adjusting a test to test both the Value method on the builder and the final array.

### Are these changes tested?

Yes

### Are there any user-facing changes?

Just an enhancement to get on-par with other builders.

* Closes: apache#37465

Authored-by: Matthias Loibl <mail@matthiasloibl.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to read values from Boolean Builders

4 participants