Skip to content

GH-41684: [C++][Python] Add optional null_bitmap to MapArray::FromArrays - #41757

Merged
jorisvandenbossche merged 5 commits into
apache:mainfrom
AlenkaF:gh-41684-map-array-mask
May 31, 2024
Merged

GH-41684: [C++][Python] Add optional null_bitmap to MapArray::FromArrays#41757
jorisvandenbossche merged 5 commits into
apache:mainfrom
AlenkaF:gh-41684-map-array-mask

Conversation

@AlenkaF

@AlenkaFAlenkaF commented May 21, 2024•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

When constructing a MapArray with FromArrays one can not supply a null_bitmap.

What changes are included in this PR?

Optional null_bitmap argument is added to MapArray::FromArrays.

Are these changes tested?

TODO (have them locally, need to clean them up and commit.

Are there any user-facing changes?

No.

@AlenkaF
AlenkaF marked this pull request as ready for review May 27, 2024 17:59
jorisvandenbossche
jorisvandenbossche reviewed May 30, 2024
View reviewed changes

@jorisvandenbosschejorisvandenbossche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Just two small comments

Comment threadcpp/src/arrow/array/array_nested.cc Outdated
Comment threadpython/pyarrow/tests/test_array.py Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels May 30, 2024
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 30, 2024
ianmcook
ianmcook approved these changes May 30, 2024
View reviewed changes

@ianmcookianmcook left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested this and confirmed it fixes the issue I described in #41684. Thanks!

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 30, 2024
@jorisvandenbossche
jorisvandenbossche merged commit 255dbf9 into apache:mainMay 31, 2024
@jorisvandenbosschejorisvandenbossche removed the awaiting merge Awaiting merge label May 31, 2024
@github-actionsgithub-actionsBot added the awaiting merge Awaiting merge label May 31, 2024
@jorisvandenbosschejorisvandenbossche mentioned this pull request May 31, 2024
Closed
@AlenkaF
AlenkaF deleted the gh-41684-map-array-mask branch May 31, 2024 11:46
felipecrv
felipecrv reviewed Jun 3, 2024
View reviewed changes

@felipecrvfelipecrv 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.

Sorry for not reviewing this before merge.

Comment threadcpp/src/arrow/array/array_nested.cc
Comment threadcpp/src/arrow/array/array_nested.cc
Comment threadcpp/src/arrow/array/array_nested.cc
Comment threadcpp/src/arrow/array/array_nested.cc
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jun 3, 2024
@AlenkaF

AlenkaF commented Jun 3, 2024

Copy link
Copy Markdown
MemberAuthor

Thank you for the comments @felipecrv! Will create a follow-up issue and PR (tomorrow) 👍

This was referenced Jun 4, 2024
Closed
Merged
@conbench-apache-arrow

conbench-apache-arrowBot commented Jun 7, 2024

Copy link
Copy Markdown

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

There were no benchmark performance regressions. 🎉

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

felipecrv pushed a commit that referenced this pull request Jun 13, 2024
@AlenkaF@kou
GH-41955: [C++] Follow up of adding null_bitmap to MapArray::FromArra…
fe1f7c0
…ys (#41956)
### Rationale for this change
There have been some new comments regarding the work done in #41757.
### What changes are included in this PR?
This PR addresses the comments from #41757 (review)
### Are these changes tested?
Yes. Existing tests should pass.
### Are there any user-facing changes?
No.
* GitHub Issue: #41955
Lead-authored-by: AlenkaF <frim.alenka@gmail.com>
Co-authored-by: Alenka Frim <AlenkaF@users.noreply.github.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Felipe Oliveira Carvalho <felipekde@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felipecrvfelipecrvfelipecrv left review comments
@ianmcookianmcookianmcook approved these changes
@jorisvandenbosschejorisvandenbosschejorisvandenbossche approved these changes

Assignees

No one assigned

Labels

awaiting changesAwaiting changesComponent: C++Component: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AlenkaF@felipecrv@ianmcook@jorisvandenbossche