Skip to content

fix: resolve char signedness issues on AArch64 - #159

Closed
u70b3 wants to merge 2 commits into
apache:mainfrom
u70b3:agent/fix-arm-char-portability
Closed

fix: resolve char signedness issues on AArch64#159
u70b3 wants to merge 2 commits into
apache:mainfrom
u70b3:agent/fix-arm-char-portability

Conversation

@u70b3

@u70b3 u70b3 commented Jul 27, 2026

Copy link
Copy Markdown

Purpose

Linked issue: N/A

Fix AArch64 (ARM64) build failures and runtime differences caused by plain char being unsigned by default on the platform.

  • Represent serialized byte fixtures with std::uint8_t and explicit hexadecimal values, converting to char only at existing string/data boundaries.
  • Classify UTF-8 leading bytes with unsigned masks instead of signed shifts.
  • Use std::int8_t explicitly for MurmurHash tail-byte sign extension so behavior continues to match Java byte semantics.
  • Make legacy GetByte() expectations explicit without changing its public contract.
  • Preserve the existing exported BinaryString::NumBytesForFirstByte(char) signature by converting to std::uint8_t only inside the implementation.

Although AArch64 exposed the problem, the fix makes byte handling independent of the implementation-defined signedness of plain char on every platform. It avoids a global -fsigned-char compiler workaround.

Tests

Validated natively on AArch64 Ubuntu 22.04 with GCC 12 and tests enabled. Plain char is unsigned and the effective compile flags do not include -fsigned-char.

  • Latest affected rebuild after the ABI-preserving follow-up: 375/375 build steps passed.
  • Targeted byte-fixture tests: 31/31 passed.
  • Targeted runtime regressions: 7/7 passed.
  • paimon-common-test: 1121/1121 tests passed.
  • Earlier full CTest validation: 22/23 test programs passed.
  • clang-format 20.1.8 dry run and git diff --check passed.
  • The rebuilt shared library exports BinaryString::NumBytesForFirstByte(char), preserving the previous C++ ABI symbol.

The remaining paimon-core-test program has two pre-existing AArch64 failures in CastExecutorTest involving out-of-range, Infinity, and NaN floating-point-to-integer conversion. They are independent of these char signedness changes.

API and Format

No public API, exported ABI, storage format, or protocol change.

Documentation

No documentation change. This is an AArch64 portability fix.

Generative AI tooling

Generated-by: OpenAI Codex (GPT-5)

@u70b3
u70b3 marked this pull request as ready for review July 27, 2026 16:30
@u70b3 u70b3 changed the title fix: make byte handling independent of char signedness fix: resolve char signedness issues on AArch64 Jul 27, 2026
@zjw1111

zjw1111 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@SteNicholas Could you take a look?

@zjw1111

zjw1111 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Thanks a lot for this patch, and for the detailed validation you did on AArch64.
It is worth noting that, @SteNicholas is already driving AArch64 support, tracked in alibaba/paimon-cpp#132. His upcoming PR is expected to bring complete AArch64 build support, which will also cover the char signedness issues you fixed here. Once his PR is ready, we'll ping you for review.
As additional context: The Apache repository is still in the middle of the code migration, so we prioritize accepting new PRs in the alibaba/paimon-cpp repository.
Thanks again for your contribution!

@u70b3 u70b3 closed this Jul 28, 2026
@u70b3

u70b3 commented Jul 28, 2026

Copy link
Copy Markdown
Author

Thanks a lot for this patch, and for the detailed validation you did on AArch64. It is worth noting that, @SteNicholas is already driving AArch64 support, tracked in alibaba/paimon-cpp#132. His upcoming PR is expected to bring complete AArch64 build support, which will also cover the char signedness issues you fixed here. Once his PR is ready, we'll ping you for review. As additional context: The Apache repository is still in the middle of the code migration, so we prioritize accepting new PRs in the alibaba/paimon-cpp repository. Thanks again for your contribution!

Thanks for the context. I wasn’t aware of the ongoing repository migration or the existing AArch64 work. I ran into this issue in my own AArch64 environment and fixed it along the way, so I opened this PR in case the changes and validation results might be useful. I’m happy to review the upcoming PR and validate it directly in my AArch64 environment once it’s ready.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants