Skip to content

Conversation

@andishgar
Copy link
Contributor

@andishgar andishgar commented Dec 10, 2025

Rationale for this change

arrow::util::SafeLoadAs, arrow::util::SafeLoad, and arrow::util::SafeCopy do not work correctly for types that do not have a default constructor.

What changes are included in this PR?

Changed the behavior of the above-mentioned functions to support types that do not have a default constructor.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions
Copy link

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

@andishgar
Copy link
Contributor Author

andishgar commented Dec 11, 2025

@pitrou
As I found here, the approach used for bit_cast in this PR is perfectly fine.

Attuned developers use aligned_storage with memcpy, avoiding alignment pitfalls and allowing them to bit-cast non-default-constructible types.

And here is the reference implementation for std::bit_cast.
https://github.com/jfbastien/bit_cast/blob/c288208c1a68a329a5839f007200ab2ee2b65073/bit_cast.h#L57-L59

@andishgar
Copy link
Contributor Author

@pitrou I’d appreciate your review when you have a moment.

@pitrou pitrou requested review from bkietz and felipecrv December 17, 2025 12:57
@pitrou
Copy link
Member

pitrou commented Dec 17, 2025

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: dff4b7c

Submitted crossbow builds: ursacomputing/crossbow @ actions-65a6da834d

Task Status
example-cpp-minimal-build-static GitHub Actions
example-cpp-minimal-build-static-system-dependency GitHub Actions
example-cpp-tutorial GitHub Actions
test-build-cpp-fuzz GitHub Actions
test-conda-cpp GitHub Actions
test-conda-cpp-valgrind GitHub Actions
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1 GitHub Actions
test-cuda-cpp-ubuntu-24.04-cuda-13.0.2 GitHub Actions
test-debian-12-cpp-amd64 GitHub Actions
test-debian-12-cpp-i386 GitHub Actions
test-fedora-42-cpp GitHub Actions
test-ubuntu-22.04-cpp GitHub Actions
test-ubuntu-22.04-cpp-20 GitHub Actions
test-ubuntu-22.04-cpp-bundled GitHub Actions
test-ubuntu-22.04-cpp-emscripten GitHub Actions
test-ubuntu-22.04-cpp-no-threading GitHub Actions
test-ubuntu-24.04-cpp GitHub Actions
test-ubuntu-24.04-cpp-bundled-offline GitHub Actions
test-ubuntu-24.04-cpp-gcc-13-bundled GitHub Actions
test-ubuntu-24.04-cpp-gcc-14 GitHub Actions
test-ubuntu-24.04-cpp-minimal-with-formats GitHub Actions
test-ubuntu-24.04-cpp-thread-sanitizer GitHub Actions

@pitrou
Copy link
Member

pitrou commented Dec 17, 2025

arrow::util::SafeLoadAs, arrow::util::SafeLoad, and arrow::util::SafeCopy do not work correctly for types that do not have a default constructor.

Is there any type that you have in mind? There are not many types that are trivially constructible but don't have a default constructor, IMHO.

@andishgar
Copy link
Contributor Author

Is there any type that you have in mind? There are not many types that are trivially constructible but don't have a default constructor, IMHO.

@pitrou
Types with the following pattern are trivially copyable but do not have a default constructor. However, I have not found any such type in Arrow so far.

struct A {
  explicit A(int a) : a(a) {}
  int a;
};

What would you suggest here?

  1. Close the PR?
  2. Add a separate code path depending on the presence of a default constructor?
  3. Or proceed with merging this PR as is?

@pitrou
Copy link
Member

pitrou commented Dec 17, 2025

I'm ok with merging PR, I was wondering if you had a specific reason for submitting it.

@pitrou pitrou added the CI: Extra: C++ Run extra C++ CI label Dec 17, 2025
@andishgar
Copy link
Contributor Author

Mostly for correctness and to avoid relying on an implicit default-constructibility assumption that isn’t required by trivial copyability, even if we don’t currently have such a type in Arrow.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM, I'll merge

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Dec 18, 2025
@pitrou pitrou merged commit aeb6486 into apache:main Dec 18, 2025
55 of 58 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Dec 18, 2025
@andishgar andishgar deleted the ubsan branch December 18, 2025 08:34
@conbench-apache-arrow
Copy link

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

There were no benchmark performance regressions. 🎉

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

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.

2 participants