Skip to content
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.

Sign up for GitHub

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

Jump to bottom

GH-36417: [C++] Add Buffer::data_as, Buffer::mutable_data_as #36418

Merged
merged 5 commits into from
Jul 12, 2023
Merged

GH-36417: [C++] Add Buffer::data_as, Buffer::mutable_data_as #36418

merged 5 commits into from
Jul 12, 2023

Conversation

bkietz
Copy link
Member

@bkietz bkietz commented Jun 30, 2023 •
edited by github-actions bot
Loading

Rationale for this change

There's a lot of boilerplate in casting buffer bytes. A templated accessor will decrease that.

What changes are included in this PR?

  • {Buffer, BufferBuilder, BufferSpan}::{data_as<T>, mutable_data_as<T>}
  • rewriting a few files' worth of reinterpret_casts to use these accessors to showcase usage
  • Buffer::FromVector, a std::vector analog to Buffer::FromString
  • some tangential cleanup of unnecessary copies in the Array classes

Are these changes tested?

Not really. The rewritten files exercise the new accessors

Are there any user-facing changes?

No, but I've noticed Buffer::operator[] again and that should probably be deprecated.

@bkietz bkietz requested a review from felipecrv June 30, 2023 19:13
felipecrv
felipecrv reviewed Jun 30, 2023
View reviewed changes
cpp/src/arrow/array/array_dict.cc Outdated
if (!dictionary_) {
// FIXME this isn't thread safe
Copy link
Contributor

Choose a reason for hiding this comment

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

File issue and mention it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or move dictionary_ init to constructor?

Copy link
Member

Choose a reason for hiding this comment

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

Or use https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic

Copy link
Member

Choose a reason for hiding this comment

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

Atomic shared pointer tent to use mutex (folly has implement one using atomic swap). I guess it may hurts the performance

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, though an uncontended mutex should be reasonably fast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow up filed here #36503

will update the comment to

Suggested change
// FIXME this isn't thread safe
// TODO(GH-36503) this isn't thread safe

felipecrv
felipecrv reviewed Jun 30, 2023
View reviewed changes
cpp/src/arrow/buffer.h Outdated Show resolved Hide resolved
felipecrv
felipecrv reviewed Jun 30, 2023
View reviewed changes
cpp/src/arrow/buffer.h Outdated Show resolved Hide resolved
felipecrv
felipecrv reviewed Jun 30, 2023
View reviewed changes
cpp/src/arrow/buffer.h Outdated Show resolved Hide resolved
felipecrv
felipecrv reviewed Jun 30, 2023
View reviewed changes
Copy link
Contributor

@felipecrv felipecrv left a comment

Choose a reason for hiding this comment

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

Looks good!

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 6, 2023
@bkietz bkietz requested a review from wgtmac as a code owner July 6, 2023 21:12
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 6, 2023
pitrou
pitrou reviewed Jul 11, 2023
View reviewed changes
cpp/src/arrow/buffer.h Outdated Show resolved Hide resolved
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review awaiting merge Awaiting merge labels Jul 11, 2023
@github-actions github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 11, 2023
@pitrou
Copy link
Member

pitrou commented Jul 12, 2023

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: 5946f60

Submitted crossbow builds: ursacomputing/crossbow @ actions-eb005cb286

Task Status
test-alpine-linux-cpp Github Actions
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-cuda-cpp Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-minimal-with-formats Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions
test-ubuntu-22.04-cpp-20 Github Actions

@pitrou pitrou merged commit 95a8bfb into apache:main Jul 12, 2023
33 of 34 checks passed
@pitrou pitrou removed the awaiting change review Awaiting change review label Jul 12, 2023
@bkietz bkietz deleted the 36417-buffer-data-as branch July 12, 2023 13:06
@js8544 js8544 mentioned this pull request Jul 19, 2023
Closed
chelseajonesr pushed a commit to chelseajonesr/arrow that referenced this pull request Jul 20, 2023
@bkietz @chelseajonesr
apacheGH-36417: [C++] Add Buffer::data_as, Buffer::mutable_data_as (a…
0816e66 
…pache#36418)

### Rationale for this change

There's a lot of boilerplate in casting buffer bytes. A templated accessor will decrease that.

### What changes are included in this PR?

- `{Buffer, BufferBuilder, BufferSpan}::{data_as<T>, mutable_data_as<T>}`
- rewriting a few files' worth of reinterpret_casts to use these accessors to showcase usage
- `Buffer::FromVector`, a std::vector analog to Buffer::FromString
- some tangential cleanup of unnecessary copies in the Array classes

### Are these changes tested?

Not really. The rewritten files exercise the new accessors

### Are there any user-facing changes?

No, but I've noticed `Buffer::operator[]` again and that should probably be deprecated.

* Closes: apache#36417

Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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.

R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this pull request Aug 20, 2023
@bkietz @R-JunmingChen
apacheGH-36417: [C++] Add Buffer::data_as, Buffer::mutable_data_as (a…
8a97ec5 
…pache#36418)

### Rationale for this change

There's a lot of boilerplate in casting buffer bytes. A templated accessor will decrease that.

### What changes are included in this PR?

- `{Buffer, BufferBuilder, BufferSpan}::{data_as<T>, mutable_data_as<T>}`
- rewriting a few files' worth of reinterpret_casts to use these accessors to showcase usage
- `Buffer::FromVector`, a std::vector analog to Buffer::FromString
- some tangential cleanup of unnecessary copies in the Array classes

### Are these changes tested?

Not really. The rewritten files exercise the new accessors

### Are there any user-facing changes?

No, but I've noticed `Buffer::operator[]` again and that should probably be deprecated.

* Closes: apache#36417

Authored-by: Benjamin Kietzman <bengilgit@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@pitrou pitrou mentioned this pull request Aug 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felipecrv felipecrv felipecrv left review comments

@mapleFU mapleFU mapleFU left review comments

@westonpace westonpace westonpace approved these changes

@pitrou pitrou pitrou approved these changes

@wjones127 wjones127 Awaiting requested review from wjones127

@wgtmac wgtmac Awaiting requested review from wgtmac wgtmac is a code owner

Assignees
No one assigned
Labels
Component: C++ Component: Parquet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++] Add Buffer::data_as
5 participants
@bkietz @pitrou @felipecrv @westonpace @mapleFU