GH-48177: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures on s390x #48180

Vishwanatha-HD · 2025-11-19T14:07:50Z

Rationale for this change

This PR is intended to enable Parquet DB support on Big-endian (s390x) systems. The fix in this PR fixes "arrow-acero-asof-join-node-test" testcase failure.

The "arrow-acero-asof-join-node-test" testcase was Aborted/core dumped on Big-endian platforms.

$ ./arrow-acero-asof-join-node-test
[ RUN      ] AsofJoinNodeTest/AsofJoinBasicTest.TestBasic1Backward/3
[       OK ] AsofJoinNodeTest/AsofJoinBasicTest.TestBasic1Backward/3 (201 ms)
[ RUN      ] AsofJoinNodeTest/AsofJoinBasicTest.TestBasic1Backward/4
arrow/cpp/src/arrow/compute/util.cc:35:  Check failed: false 
Aborted (core dumped)

What changes are included in this PR?

The fix includes changes to "util.cc" file to address the Abort/Core dump issues.

Are these changes tested?

Yes. The changes are tested on s390x arch to make sure things are working fine. The fix is also tested on x86 arch, to make sure there is no new regression introduced.

Are there any user-facing changes?

No

GitHub Issue: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures on Big-Endian (s390x) systems #48177
GitHub main Issue link: [C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151

…ilures on s390x

github-actions · 2025-11-19T21:49:58Z

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

kou

Could you fix lint failure?

https://github.com/apache/arrow/actions/runs/19504115732/job/55873075753?pr=48180#step:6:84

diff --git a/cpp/src/arrow/compute/util.cc b/cpp/src/arrow/compute/util.cc
index 66c48631dc..3b671db021 100644
--- a/cpp/src/arrow/compute/util.cc
+++ b/cpp/src/arrow/compute/util.cc
@@ -325,10 +325,11 @@ void bytes_to_bits(int64_t hardware_flags, const int num_bits, const uint8_t* by
     bytes_next = SafeLoadUpTo8Bytes(bytes + num_bits - tail, tail);
 #else
     if (tail == 8) {
-      bytes_next = util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes + num_bits - tail));
+      bytes_next =
+          util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes + num_bits - tail));
     } else {
-      // On Big-endian systems, for bytes_to_bits, load all tail bytes in little-endian order
-      // to ensure compatibility with subsequent bit operations
+      // On Big-endian systems, for bytes_to_bits, load all tail bytes in little-endian
+      // order to ensure compatibility with subsequent bit operations
       bytes_next = 0;
       for (int i = 0; i < tail; ++i) {
         bytes_next |= static_cast<uint64_t>((bytes + num_bits - tail)[i]) << (8 * i);

You can use nice pre-commit run --show-diff-on-failure --color=always --all-files cpp.

kou · 2025-11-20T00:29:51Z

cpp/src/arrow/compute/util.cc

-#endif
  ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
  if (num_bytes == 8) {
    return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));


Does this work on big-endian system?

Thanks for pointing out this.. Now with the way we are handling the tail_bytes and loading the word data, we dont actually need to change "SafeLoadUpTo8Bytes()" function.. With the conditional compilation, this function will never be called on Big-endian architecture.
I have reverted this change.. Tested completely on s390x to see if all the test work. I have pushed a new commit. Please give your review comments. Thanks.

kou · 2025-11-20T00:33:23Z

cpp/src/arrow/compute/util.cc

+#if ARROW_LITTLE_ENDIAN
    uint64_t word = SafeLoadUpTo8Bytes(bits_tail, (tail + 7) / 8);
+#else
+    int tail_bytes = (tail + 7) / 8;
+    uint64_t word;
+    if (tail_bytes == 8) {
+      word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bits_tail));
+    } else {
+      // For bit manipulation, always load into least significant bits
+      // to ensure compatibility with CountTrailingZeros on Big-endian systems
+      word = 0;
+      for (int i = 0; i < tail_bytes; ++i) {
+        word |= static_cast<uint64_t>(bits_tail[i]) << (8 * i);
+      }
+    }
+#endif


Why do we need this?

The SafeLoadUpTo8Bytes() change adds support for big-endian, right?

Now that I have removed the big-endian support to SafeLoadUpTo8Bytes() function, these changes are required as these handle the way we handle the tail_bytes on big-endian systems. If the tail_bytes are equal to 8, then we call directly the SafeLoad to load the data onto "word" variable. And for rest other cases, we need to take care of loading least significant bits to ensure compatibility with "CountTrailingZeros". This is the reason why we wont be able to make a direct call "SafeLoadUpTo8Bytes()" for every tail_bytes.

I have fixed the lint errors and pushed my changes. Thanks..

…ilures on s390x

Vishwanatha-HD

I have addressed all the review comments. Please re-review. Thanks.

Vishwanatha-HD · 2025-11-20T10:28:55Z

cpp/src/arrow/compute/util.cc

-#endif
  ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
  if (num_bytes == 8) {
    return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));


Thanks for pointing out this.. Now with the way we are handling the tail_bytes and loading the word data, we dont actually need to change "SafeLoadUpTo8Bytes()" function.. With the conditional compilation, this function will never be called on Big-endian architecture.
I have reverted this change.. Tested completely on s390x to see if all the test work. I have pushed a new commit. Please give your review comments. Thanks.

Vishwanatha-HD · 2025-11-20T11:23:05Z

cpp/src/arrow/compute/util.cc

+#if ARROW_LITTLE_ENDIAN
    uint64_t word = SafeLoadUpTo8Bytes(bits_tail, (tail + 7) / 8);
+#else
+    int tail_bytes = (tail + 7) / 8;
+    uint64_t word;
+    if (tail_bytes == 8) {
+      word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bits_tail));
+    } else {
+      // For bit manipulation, always load into least significant bits
+      // to ensure compatibility with CountTrailingZeros on Big-endian systems
+      word = 0;
+      for (int i = 0; i < tail_bytes; ++i) {
+        word |= static_cast<uint64_t>(bits_tail[i]) << (8 * i);
+      }
+    }
+#endif


Now that I have removed the big-endian support to SafeLoadUpTo8Bytes() function, these changes are required as these handle the way we handle the tail_bytes on big-endian systems. If the tail_bytes are equal to 8, then we call directly the SafeLoad to load the data onto "word" variable. And for rest other cases, we need to take care of loading least significant bits to ensure compatibility with "CountTrailingZeros". This is the reason why we wont be able to make a direct call "SafeLoadUpTo8Bytes()" for every tail_bytes.

Vishwanatha-HD · 2025-11-20T12:52:51Z

cpp/src/arrow/compute/util.cc

+#if ARROW_LITTLE_ENDIAN
    uint64_t word = SafeLoadUpTo8Bytes(bits_tail, (tail + 7) / 8);
+#else
+    int tail_bytes = (tail + 7) / 8;
+    uint64_t word;
+    if (tail_bytes == 8) {
+      word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bits_tail));
+    } else {
+      // For bit manipulation, always load into least significant bits
+      // to ensure compatibility with CountTrailingZeros on Big-endian systems
+      word = 0;
+      for (int i = 0; i < tail_bytes; ++i) {
+        word |= static_cast<uint64_t>(bits_tail[i]) << (8 * i);
+      }
+    }
+#endif


I have fixed the lint errors and pushed my changes. Thanks..

kou · 2025-11-22T06:48:39Z

cpp/src/arrow/compute/util.cc

+#if ARROW_LITTLE_ENDIAN
+    bytes_next = SafeLoadUpTo8Bytes(bytes + num_bits - tail, tail);
+#else
+    if (tail == 8) {


Does this happen?
I think that tail must not be 8 because unroll is always 8 and tail = num_bits % unroll.

kou

Does this work?

diff --git a/cpp/src/arrow/compute/util.cc b/cpp/src/arrow/compute/util.cc
index b90b3a6405..163a80d9d4 100644
--- a/cpp/src/arrow/compute/util.cc
+++ b/cpp/src/arrow/compute/util.cc
@@ -30,33 +30,41 @@ namespace util {
 namespace bit_util {
 
 inline uint64_t SafeLoadUpTo8Bytes(const uint8_t* bytes, int num_bytes) {
-  // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
-  ARROW_DCHECK(false);
-#endif
   ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
   if (num_bytes == 8) {
-    return util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));
+    auto word = util::SafeLoad(reinterpret_cast<const uint64_t*>(bytes));
+#if !ARROW_LITTLE_ENDIAN
+    word = bit_util::ByteSwap(word);
+#endif
+    return word;
   } else {
     uint64_t word = 0;
     for (int i = 0; i < num_bytes; ++i) {
+#if ARROW_LITTLE_ENDIAN
       word |= static_cast<uint64_t>(bytes[i]) << (8 * i);
+#else
+      word |= static_cast<uint64_t>(bytes[num_bytes - 1 - i]) << (8 * i);
+#endif
     }
     return word;
   }
 }
 
 inline void SafeStoreUpTo8Bytes(uint8_t* bytes, int num_bytes, uint64_t value) {
-  // This will not be correct on big-endian architectures.
-#if !ARROW_LITTLE_ENDIAN
-  ARROW_DCHECK(false);
-#endif
   ARROW_DCHECK(num_bytes >= 0 && num_bytes <= 8);
   if (num_bytes == 8) {
+#if ARROW_LITTLE_ENDIAN
     util::SafeStore(reinterpret_cast<uint64_t*>(bytes), value);
+#else
+    util::SafeStore(reinterpret_cast<uint64_t*>(bytes), bit_util::ByteSwap(value));
+#endif
   } else {
     for (int i = 0; i < num_bytes; ++i) {
+#if ARROW_LITTLE_ENDIAN
       bytes[i] = static_cast<uint8_t>(value >> (8 * i));
+#else
+      bytes[i] = static_cast<uint8_t>(value >> (8 * (num_bytes - 1 - i)));
+#endif
     }
   }
 }

k8ika0s · 2025-11-23T22:56:42Z

Mostly looks good to me — just one thought after reading through the recent back-and-forth...

Given the updated handling of tail bytes and the SafeLoadUpTo8Bytes discussion, I think this PR’s direction still makes sense. I’d just double-check that the tail==8 path really can’t happen with the current unroll logic, since @kou kou raised that question.
Otherwise the fixes seem aligned with the latest comments.

Willing to help test once the approach is finalized.

apacheGH-48151: [C++][Parquet] Fix arrow-acero-asof-join-node-test fa…

d168c0c

…ilures on s390x

github-actions bot added Component: C++ awaiting review Awaiting review labels Nov 19, 2025

Vishwanatha-HD mentioned this pull request Nov 19, 2025

[C++][Parquet] Fix arrow-acero-asof-join-node-test failures on Big-Endian (s390x) systems #48177

Open

kou changed the title ~~GH-48151: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures…~~ GH-48177: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures on s390x Nov 19, 2025

kou reviewed Nov 20, 2025

View reviewed changes

apacheGH-48151: [C++][Parquet] Fix arrow-acero-asof-join-node-test fa…

ec51da8

…ilures on s390x

Vishwanatha-HD force-pushed the fixParqIssues2 branch from d80ee37 to ec51da8 Compare November 20, 2025 12:35

Vishwanatha-HD commented Nov 20, 2025

View reviewed changes

Vishwanatha-HD mentioned this pull request Nov 21, 2025

[C++][Parquet] Enable Parquet DB support on Big Endian (IBM Z) systems #48151

Open

kou reviewed Nov 22, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Nov 24, 2025

GH-48177: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures on s390x #48180

Are you sure you want to change the base?

GH-48177: [C++][Parquet] Fix arrow-acero-asof-join-node-test failures on s390x #48180

Conversation

Vishwanatha-HD commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Nov 19, 2025

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vishwanatha-HD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

k8ika0s commented Nov 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Vishwanatha-HD commented Nov 19, 2025 •

edited

Loading