[GitHub] [arrow] pitrou commented on a change in pull request #10651: ARROW-11748: [C++] Ensure Decimal fields are in native endian order

[GitBox <gi...@apache.org>]

pitrou commented on a change in pull request #10651:
URL: https://github.com/apache/arrow/pull/10651#discussion_r674152096



##########
File path: cpp/src/arrow/util/endian.h
##########
@@ -124,12 +127,23 @@ static inline T ToBigEndian(T value) {
   return ByteSwap(value);
 }
 
+template <typename T, size_t N>
+static inline std::array<T, N> ToBigEndian(std::array<T, N> array) {

Review comment:
       This API is a bit ambiguous. Should it also byteswap the `T`s?

##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -572,20 +593,23 @@ struct uint128_t {
 
 // Multiplies two N * 64 bit unsigned integer types, represented by a uint64_t
 // array into a same sized output. Elements in the array should be in
-// little endian order, and output will be the same. Overflow in multiplication
+// native endian order, and output will be the same. Overflow in multiplication
 // will result in the lower N * 64 bits of the result being set.
 template <int N>
 inline void MultiplyUnsignedArray(const std::array<uint64_t, N>& lh,
                                   const std::array<uint64_t, N>& rh,
                                   std::array<uint64_t, N>* result) {
+  BitUtil::LittleEndianArrayReader<uint64_t, N> lh_le(lh), rh_le(rh);

Review comment:
       `const`?

##########
File path: cpp/src/arrow/util/basic_decimal.h
##########
@@ -193,23 +204,25 @@ class ARROW_EXPORT BasicDecimal256 {
   static constexpr int bit_width = 256;
 
   /// \brief Create a BasicDecimal256 from the two's complement representation.
-  constexpr BasicDecimal256(const std::array<uint64_t, 4>& little_endian_array) noexcept
-      : little_endian_array_(little_endian_array) {}
+  constexpr BasicDecimal256(const std::array<uint64_t, 4>& array) noexcept
+      : array_(array) {}

Review comment:
       Add a docstring comment about endianness?

##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -121,217 +121,238 @@ static const BasicDecimal128 ScaleMultipliersHalf[] = {
     BasicDecimal128(271050543121376108LL, 9257742014424809472ULL),
     BasicDecimal128(2710505431213761085LL, 343699775700336640ULL)};
 
+#define BasicDecimal256FromLE(v1, v2, v3, v4) \
+  BasicDecimal256(BitUtil::FromLittleEndian<uint64_t, 4>(v1, v2, v3, v4))

Review comment:
       Can you `undef` this after?

##########
File path: cpp/src/arrow/util/endian.h
##########
@@ -169,13 +205,47 @@ static inline T FromBigEndian(T value) {
   return value;
 }
 
+template <typename T, size_t N>
+static inline std::array<T, N> FromBigEndian(std::array<T, N> array) {
+  return array;
+}
+
 template <typename T, typename = internal::EnableIfIsOneOf<
                           T, int64_t, uint64_t, int32_t, uint32_t, int16_t, uint16_t,
                           uint8_t, int8_t, float, double>>
 static inline T FromLittleEndian(T value) {
   return ByteSwap(value);
 }
+
+template <typename T, size_t N>
+static inline std::array<T, N> FromLittleEndian(std::array<T, N> array) {
+  std::reverse(array.begin(), array.end());
+  return array;
+}
 #endif
 
+// Read a native endian array as little endian
+template <typename T, size_t N>
+struct LittleEndianArrayReader {

Review comment:
       Perhaps merge this and the writer as a single `LittleEndianArray` class?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org