This is an automated email from the ASF dual-hosted git repository.
paleolimbot pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-nanoarrow.git
The following commit(s) were added to refs/heads/main by this push:
new 08a64ff4 fix: ArrowDecimalSetDigits() on s390x + test fixes for big
endian (#732)
08a64ff4 is described below
commit 08a64ff44fce354dfca9c011443dbe555f3a8715
Author: Dewey Dunnington <[email protected]>
AuthorDate: Mon Mar 24 09:20:28 2025 -0500
fix: ArrowDecimalSetDigits() on s390x + test fixes for big endian (#732)
This PR fixes Decimal32 and Decimal64 handling on big endian. The
previous code used a cast on the least significant word to convert
between 64-bit words and the 32 bit value; however, this is only correct
on big endian. This was exposed by #720, which in turn exposed that the
get/set digits behaviour was not tested for Decimal 32/64. This code was
updated to use a `memcpy()` instead of a cast for the 32-bit case. The
s390x workflow had been failing because of a segfaulting compiler for a
while and so I'd missed that the decimal 32/64 change had broken it 😬 .
There is also one change to get the Windows verification job passing
that I noticed when running the verify.yaml workflow to test big endian.
This can be checked locally with:
```shell
export NANOARROW_ARCH=s390x
docker compose run --rm verify
```
(and was also checked via the verify.yaml workflow)
---
src/nanoarrow/common/inline_types.h | 28 +++++++++-----
src/nanoarrow/common/utils.c | 19 +++++++++-
src/nanoarrow/common/utils_test.cc | 76 ++++++++++++++++++++-----------------
src/nanoarrow/ipc/decoder_test.cc | 2 +
src/nanoarrow/ipc/files_test.cc | 24 ++++++++++--
5 files changed, 100 insertions(+), 49 deletions(-)
diff --git a/src/nanoarrow/common/inline_types.h
b/src/nanoarrow/common/inline_types.h
index 603d566e..a8d194f1 100644
--- a/src/nanoarrow/common/inline_types.h
+++ b/src/nanoarrow/common/inline_types.h
@@ -954,8 +954,11 @@ static inline void ArrowDecimalInit(struct ArrowDecimal*
decimal, int32_t bitwid
/// to 18 is sufficiently small).
static inline int64_t ArrowDecimalGetIntUnsafe(const struct ArrowDecimal*
decimal) {
if (decimal->n_words == 0) {
- return (int32_t)decimal->words[0];
+ int32_t value;
+ memcpy(&value, decimal->words, sizeof(int32_t));
+ return value;
}
+
return (int64_t)decimal->words[decimal->low_word_index];
}
@@ -963,25 +966,29 @@ static inline int64_t ArrowDecimalGetIntUnsafe(const
struct ArrowDecimal* decima
/// \ingroup nanoarrow-utils
static inline void ArrowDecimalGetBytes(const struct ArrowDecimal* decimal,
uint8_t* out) {
- memcpy(out, decimal->words,
- (decimal->n_words > 0 ? decimal->n_words : 1) * sizeof(uint64_t));
+ if (decimal->n_words == 0) {
+ memcpy(out, decimal->words, sizeof(int32_t));
+ } else {
+ memcpy(out, decimal->words, decimal->n_words * sizeof(uint64_t));
+ }
}
/// \brief Returns 1 if the value represented by decimal is >= 0 or -1
otherwise
/// \ingroup nanoarrow-utils
static inline int64_t ArrowDecimalSign(const struct ArrowDecimal* decimal) {
if (decimal->n_words == 0) {
- return 1 | ((int32_t)(decimal->words[0]) >> 31);
+ return ArrowDecimalGetIntUnsafe(decimal) >= 0 ? 1 : -1;
+ } else {
+ return 1 | ((int64_t)(decimal->words[decimal->high_word_index]) >> 63);
}
-
- return 1 | ((int64_t)(decimal->words[decimal->high_word_index]) >> 63);
}
/// \brief Sets the integer value of this decimal
/// \ingroup nanoarrow-utils
static inline void ArrowDecimalSetInt(struct ArrowDecimal* decimal, int64_t
value) {
if (decimal->n_words == 0) {
- decimal->words[0] = (int32_t)value;
+ int32_t value32 = (int32_t)value;
+ memcpy(decimal->words, &value32, sizeof(int32_t));
return;
}
@@ -998,9 +1005,10 @@ static inline void ArrowDecimalSetInt(struct
ArrowDecimal* decimal, int64_t valu
/// \ingroup nanoarrow-utils
static inline void ArrowDecimalNegate(struct ArrowDecimal* decimal) {
if (decimal->n_words == 0) {
- uint32_t elem = (uint32_t)decimal->words[0];
- elem = ~elem + 1;
- decimal->words[0] = (int32_t)elem;
+ int32_t value;
+ memcpy(&value, decimal->words, sizeof(int32_t));
+ value = -value;
+ memcpy(decimal->words, &value, sizeof(int32_t));
return;
}
diff --git a/src/nanoarrow/common/utils.c b/src/nanoarrow/common/utils.c
index 9dad5607..5b5116a1 100644
--- a/src/nanoarrow/common/utils.c
+++ b/src/nanoarrow/common/utils.c
@@ -346,13 +346,14 @@ ArrowErrorCode ArrowDecimalSetDigits(struct ArrowDecimal*
decimal,
// Use 32-bit words for portability
uint32_t words32[8];
+ memset(words32, 0, sizeof(words32));
int n_words32 = decimal->n_words > 0 ? decimal->n_words * 2 : 1;
NANOARROW_DCHECK(n_words32 <= 8);
memset(words32, 0, sizeof(words32));
ShiftAndAdd(value, words32, n_words32);
- if (decimal->low_word_index == 0) {
+ if (_ArrowIsLittleEndian() || n_words32 == 1) {
memcpy(decimal->words, words32, sizeof(uint32_t) * n_words32);
} else {
uint64_t lo;
@@ -378,6 +379,22 @@ ArrowErrorCode ArrowDecimalAppendDigitsToBuffer(const
struct ArrowDecimal* decim
struct ArrowBuffer* buffer) {
NANOARROW_DCHECK(decimal->n_words == 0 || decimal->n_words == 1 ||
decimal->n_words == 2 || decimal->n_words == 4);
+
+ // For the 32-bit case, just use snprintf()
+ if (decimal->n_words == 0) {
+ int32_t value;
+ memcpy(&value, decimal->words, sizeof(int32_t));
+ NANOARROW_RETURN_NOT_OK(ArrowBufferReserve(buffer, 16));
+ int n_chars = snprintf((char*)buffer->data + buffer->size_bytes,
+ (buffer->capacity_bytes - buffer->size_bytes),
"%d", value);
+ if (n_chars <= 0) {
+ return EINVAL;
+ }
+
+ buffer->size_bytes += n_chars;
+ return NANOARROW_OK;
+ }
+
int is_negative = ArrowDecimalSign(decimal) < 0;
uint64_t words_little_endian[4];
diff --git a/src/nanoarrow/common/utils_test.cc
b/src/nanoarrow/common/utils_test.cc
index 0d9c52f8..b9b64870 100644
--- a/src/nanoarrow/common/utils_test.cc
+++ b/src/nanoarrow/common/utils_test.cc
@@ -19,13 +19,13 @@
#include <string>
#if defined(NANOARROW_BUILD_TESTS_WITH_ARROW)
+#include <arrow/config.h>
#include <arrow/util/decimal.h>
#endif
#include <gmock/gmock-matchers.h>
#include <gtest/gtest.h>
#include "nanoarrow/nanoarrow.hpp"
-#include "nanoarrow/nanoarrow_testing.hpp"
#if defined(NANOARROW_BUILD_TESTS_WITH_ARROW)
using namespace arrow;
@@ -403,7 +403,7 @@ TEST(DecimalTest, DecimalNegateTest) {
} else if (bitwidth == 64) {
decimal.words[decimal.low_word_index] =
std::numeric_limits<int64_t>::max();
} else {
- decimal.words[decimal.low_word_index] =
std::numeric_limits<int32_t>::max();
+ ArrowDecimalSetInt(&decimal, std::numeric_limits<int32_t>::max());
}
ASSERT_EQ(ArrowDecimalSign(&decimal), 1);
ArrowDecimalNegate(&decimal);
@@ -417,8 +417,7 @@ TEST(DecimalTest, DecimalNegateTest) {
EXPECT_EQ(decimal.words[decimal.low_word_index],
std::numeric_limits<int64_t>::max());
} else {
- EXPECT_EQ(decimal.words[decimal.low_word_index],
- std::numeric_limits<int32_t>::max());
+ EXPECT_EQ(ArrowDecimalGetIntUnsafe(&decimal),
std::numeric_limits<int32_t>::max());
}
if (bitwidth > 64) {
@@ -582,37 +581,44 @@ TEST(DecimalTest, DecimalDigitsTestInvalid) {
}
TEST(DecimalTest, DecimalRoundtripPowerOfTenTest) {
- struct ArrowDecimal decimal;
- ArrowDecimalInit(&decimal, 256, 76, 0);
-
- struct ArrowBuffer buffer;
- ArrowBufferInit(&buffer);
-
- // Generate test strings with positive and negative powers of 10 and check
- // roundtrip back to string.
- std::stringstream ss;
-
- for (const auto& sign : {"", "-"}) {
- for (int i = 0; i < 76; i++) {
- ss.str("");
- ss << sign;
- ss << "1";
- for (int j = 0; j < i; j++) {
- ss << "0";
+ std::vector<std::pair<int, int>> bitwidth_and_max_precision = {
+ {32, 9}, {64, 18}, {128, 38}, {256, 76}};
+
+ for (const auto& item : bitwidth_and_max_precision) {
+ SCOPED_TRACE(item.first);
+
+ struct ArrowDecimal decimal;
+ ArrowDecimalInit(&decimal, item.first, item.second, 0);
+
+ struct ArrowBuffer buffer;
+ ArrowBufferInit(&buffer);
+
+ // Generate test strings with positive and negative powers of 10 and check
+ // roundtrip back to string.
+ std::stringstream ss;
+
+ for (const auto& sign : {"", "-"}) {
+ for (int i = 0; i < item.second; i++) {
+ ss.str("");
+ ss << sign;
+ ss << "1";
+ for (int j = 0; j < i; j++) {
+ ss << "0";
+ }
+
+ SCOPED_TRACE(ss.str());
+ ASSERT_EQ(ArrowDecimalSetDigits(&decimal,
ArrowCharView(ss.str().c_str())),
+ NANOARROW_OK);
+
+ buffer.size_bytes = 0;
+ ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer),
NANOARROW_OK);
+ ASSERT_EQ(std::string(reinterpret_cast<char*>(buffer.data),
buffer.size_bytes),
+ ss.str());
}
-
- SCOPED_TRACE(ss.str());
- ASSERT_EQ(ArrowDecimalSetDigits(&decimal,
ArrowCharView(ss.str().c_str())),
- NANOARROW_OK);
-
- buffer.size_bytes = 0;
- ASSERT_EQ(ArrowDecimalAppendDigitsToBuffer(&decimal, &buffer),
NANOARROW_OK);
- EXPECT_EQ(std::string(reinterpret_cast<char*>(buffer.data),
buffer.size_bytes),
- ss.str());
}
- }
- ArrowBufferReset(&buffer);
+ ArrowBufferReset(&buffer);
+ }
}
TEST(DecimalTest, DecimalRoundtripBitshiftTest) {
@@ -665,7 +671,7 @@ TEST(DecimalTest, DecimalTestStringPositiveScale) {
using namespace nanoarrow::literals;
struct ArrowDecimal decimal;
- ArrowDecimalInit(&decimal, 32, 9, 3);
+ ArrowDecimalInit(&decimal, 128, 9, 3);
nanoarrow::UniqueBuffer buffer;
@@ -688,7 +694,7 @@ TEST(DecimalTest, DecimalTestStringZeroScale) {
using namespace nanoarrow::literals;
struct ArrowDecimal decimal;
- ArrowDecimalInit(&decimal, 32, 9, 0);
+ ArrowDecimalInit(&decimal, 128, 9, 0);
nanoarrow::UniqueBuffer buffer;
@@ -710,7 +716,7 @@ TEST(DecimalTest, DecimalTestStringNegativeScale) {
using namespace nanoarrow::literals;
struct ArrowDecimal decimal;
- ArrowDecimalInit(&decimal, 32, 9, -3);
+ ArrowDecimalInit(&decimal, 128, 9, -3);
nanoarrow::UniqueBuffer buffer;
diff --git a/src/nanoarrow/ipc/decoder_test.cc
b/src/nanoarrow/ipc/decoder_test.cc
index b3856656..eb084f0b 100644
--- a/src/nanoarrow/ipc/decoder_test.cc
+++ b/src/nanoarrow/ipc/decoder_test.cc
@@ -372,6 +372,8 @@ void TestDecodeInt32Batch(const uint8_t* batch, size_t
batch_len,
ASSERT_EQ(ArrowIpcDecoderInit(decoder.get()), NANOARROW_OK);
ASSERT_EQ(ArrowIpcDecoderSetSchema(decoder.get(), schema.get(), &error),
NANOARROW_OK);
+ ASSERT_EQ(ArrowIpcDecoderSetEndianness(decoder.get(),
NANOARROW_IPC_ENDIANNESS_LITTLE),
+ NANOARROW_OK);
ASSERT_EQ(ArrowIpcDecoderDecodeHeader(decoder.get(), data, &error),
NANOARROW_OK);
struct ArrowBufferView body;
diff --git a/src/nanoarrow/ipc/files_test.cc b/src/nanoarrow/ipc/files_test.cc
index 5d39ec41..55b988bb 100644
--- a/src/nanoarrow/ipc/files_test.cc
+++ b/src/nanoarrow/ipc/files_test.cc
@@ -526,14 +526,16 @@ TEST_P(TestFileFixture, NanoarrowIpcTestFileIPCCheckJSON)
{
param.TestIPCCheckJSON(dir_builder.str());
}
+// At least one Windows MSVC version does not allow the #if defined()
+// to be within a macro invocation, so we define these two cases
+// with some repetition.
+#if defined(NANOARROW_IPC_WITH_ZSTD)
INSTANTIATE_TEST_SUITE_P(
NanoarrowIpcTest, TestFileFixture,
::testing::Values(
-// Testing of other files
-#if defined(NANOARROW_IPC_WITH_ZSTD)
+ // Testing of other files
TestFile::OK("2.0.0-compression/generated_uncompressible_zstd.stream"),
TestFile::OK("2.0.0-compression/generated_zstd.stream"),
-#endif
TestFile::OK("0.17.1/generated_union.stream"),
TestFile::OK("0.14.1/generated_datetime.stream"),
TestFile::OK("0.14.1/generated_decimal.stream"),
@@ -545,5 +547,21 @@ INSTANTIATE_TEST_SUITE_P(
TestFile::OK("0.14.1/generated_primitive_zerolength.stream")
// Comment to keep line from wrapping
));
+#else
+INSTANTIATE_TEST_SUITE_P(NanoarrowIpcTest, TestFileFixture,
+ ::testing::Values(
+ // Testing of other files
+ TestFile::OK("0.17.1/generated_union.stream"),
+ TestFile::OK("0.14.1/generated_datetime.stream"),
+ TestFile::OK("0.14.1/generated_decimal.stream"),
+ TestFile::OK("0.14.1/generated_interval.stream"),
+ TestFile::OK("0.14.1/generated_map.stream"),
+ TestFile::OK("0.14.1/generated_nested.stream"),
+ TestFile::OK("0.14.1/generated_primitive.stream"),
+
TestFile::OK("0.14.1/generated_primitive_no_batches.stream"),
+
TestFile::OK("0.14.1/generated_primitive_zerolength.stream")
+ // Comment to keep line from wrapping
+ ));
+#endif
#endif
