emkornfield commented on a change in pull request #8897:
URL: https://github.com/apache/arrow/pull/8897#discussion_r542619050
##########
File path: cpp/src/parquet/arrow/reader_internal.cc
##########
@@ -369,225 +371,134 @@ Status TransferBinary(RecordReader* reader, MemoryPool*
pool,
}
// ----------------------------------------------------------------------
-// INT32 / INT64 / BYTE_ARRAY / FIXED_LEN_BYTE_ARRAY -> Decimal128
-
-static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t
stop) {
- const int32_t length = stop - start;
-
- DCHECK_GE(length, 0);
- DCHECK_LE(length, 8);
-
- switch (length) {
- case 0:
- return 0;
- case 1:
- return bytes[start];
- case 2:
- return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
- case 3: {
- const uint64_t first_two_bytes =
FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
- const uint64_t last_byte = bytes[stop - 1];
- return first_two_bytes << 8 | last_byte;
- }
- case 4:
- return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- case 5: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t last_byte = bytes[stop - 1];
- return first_four_bytes << 8 | last_byte;
- }
- case 6: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t last_two_bytes =
- FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
- return first_four_bytes << 16 | last_two_bytes;
- }
- case 7: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t second_two_bytes =
- FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
- const uint64_t last_byte = bytes[stop - 1];
- return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
- }
- case 8:
- return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
- default: {
- DCHECK(false);
- return UINT64_MAX;
- }
- }
-}
-
-static constexpr int32_t kMinDecimalBytes = 1;
-static constexpr int32_t kMaxDecimalBytes = 16;
-
-/// \brief Convert a sequence of big-endian bytes to one int64_t (high bits)
and one
-/// uint64_t (low bits).
-static void BytesToIntegerPair(const uint8_t* bytes, const int32_t length,
- int64_t* out_high, uint64_t* out_low) {
- DCHECK_GE(length, kMinDecimalBytes);
- DCHECK_LE(length, kMaxDecimalBytes);
-
- // XXX This code is copied from Decimal::FromBigEndian
-
- int64_t high, low;
-
- // Bytes are coming in big-endian, so the first byte is the MSB and
therefore holds the
- // sign bit.
- const bool is_negative = static_cast<int8_t>(bytes[0]) < 0;
-
- // 1. Extract the high bytes
- // Stop byte of the high bytes
- const int32_t high_bits_offset = std::max(0, length - 8);
- const auto high_bits = BytesToInteger(bytes, 0, high_bits_offset);
-
- if (high_bits_offset == 8) {
- // Avoid undefined shift by 64 below
- high = high_bits;
- } else {
- high = -1 * (is_negative && length < kMaxDecimalBytes);
- // Shift left enough bits to make room for the incoming int64_t
- high = SafeLeftShift(high, high_bits_offset * CHAR_BIT);
- // Preserve the upper bits by inplace OR-ing the int64_t
- high |= high_bits;
- }
-
- // 2. Extract the low bytes
- // Stop byte of the low bytes
- const int32_t low_bits_offset = std::min(length, 8);
- const auto low_bits = BytesToInteger(bytes, high_bits_offset, length);
-
- if (low_bits_offset == 8) {
- // Avoid undefined shift by 64 below
- low = low_bits;
- } else {
- // Sign extend the low bits if necessary
- low = -1 * (is_negative && length < 8);
- // Shift left enough bits to make room for the incoming int64_t
- low = SafeLeftShift(low, low_bits_offset * CHAR_BIT);
- // Preserve the upper bits by inplace OR-ing the int64_t
- low |= low_bits;
- }
-
- *out_high = high;
- *out_low = static_cast<uint64_t>(low);
+// INT32 / INT64 / BYTE_ARRAY / FIXED_LEN_BYTE_ARRAY -> Decimal128 ||
Decimal256
+
+template <typename DecimalType>
+Status RawBytesToDecimalBytes(const uint8_t* value, int32_t byte_width,
+ uint8_t* out_buf) {
+ ARROW_ASSIGN_OR_RAISE(DecimalType t, DecimalType::FromBigEndian(value,
byte_width));
+ t.ToBytes(out_buf);
+ return ::arrow::Status::OK();
}
-static inline void RawBytesToDecimalBytes(const uint8_t* value, int32_t
byte_width,
- uint8_t* out_buf) {
- // view the first 8 bytes as an unsigned 64-bit integer
- auto low = reinterpret_cast<uint64_t*>(out_buf);
+template <typename DecimalArrayType>
+struct DecimalTypeTrait;
- // view the second 8 bytes as a signed 64-bit integer
- auto high = reinterpret_cast<int64_t*>(out_buf + sizeof(uint64_t));
-
- // Convert the fixed size binary array bytes into a Decimal128 compatible
layout
- BytesToIntegerPair(value, byte_width, high, low);
-}
-
-template <typename T>
-Status ConvertToDecimal128(const Array& array, const
std::shared_ptr<DataType>&,
- MemoryPool* pool, std::shared_ptr<Array>*) {
- return Status::NotImplemented("not implemented");
-}
+template <>
+struct DecimalTypeTrait<::arrow::Decimal128Array> {
+ using value = ::arrow::Decimal128;
+};
template <>
-Status ConvertToDecimal128<FLBAType>(const Array& array,
- const std::shared_ptr<DataType>& type,
- MemoryPool* pool, std::shared_ptr<Array>*
out) {
- const auto& fixed_size_binary_array =
- static_cast<const ::arrow::FixedSizeBinaryArray&>(array);
-
- // The byte width of each decimal value
- const int32_t type_length =
- static_cast<const ::arrow::Decimal128Type&>(*type).byte_width();
-
- // number of elements in the entire array
- const int64_t length = fixed_size_binary_array.length();
-
- // Get the byte width of the values in the FixedSizeBinaryArray. Most of the
time
- // this will be different from the decimal array width because we write the
minimum
- // number of bytes necessary to represent a given precision
- const int32_t byte_width =
- static_cast<const
::arrow::FixedSizeBinaryType&>(*fixed_size_binary_array.type())
- .byte_width();
- if (byte_width < kMinDecimalBytes || byte_width > kMaxDecimalBytes) {
- return Status::Invalid("Invalid FIXED_LEN_BYTE_ARRAY length for
Decimal128");
+struct DecimalTypeTrait<::arrow::Decimal256Array> {
+ using value = ::arrow::Decimal256;
+};
+
+template <typename DecimalArrayType, typename ParquetType>
+struct DecimalConverter {
+ static inline Status ConvertToDecimal(const Array& array,
+ const std::shared_ptr<DataType>&,
+ MemoryPool* pool,
std::shared_ptr<Array>*) {
+ return Status::NotImplemented("not implemented");
}
-
- // allocate memory for the decimal array
- ARROW_ASSIGN_OR_RAISE(auto data, ::arrow::AllocateBuffer(length *
type_length, pool));
-
- // raw bytes that we can write to
- uint8_t* out_ptr = data->mutable_data();
-
- // convert each FixedSizeBinary value to valid decimal bytes
- const int64_t null_count = fixed_size_binary_array.null_count();
- if (null_count > 0) {
- for (int64_t i = 0; i < length; ++i, out_ptr += type_length) {
- if (!fixed_size_binary_array.IsNull(i)) {
- RawBytesToDecimalBytes(fixed_size_binary_array.GetValue(i),
byte_width, out_ptr);
+};
+
+template <typename DecimalArrayType>
+struct DecimalConverter<DecimalArrayType, FLBAType> {
+ static inline Status ConvertToDecimal(const Array& array,
+ const std::shared_ptr<DataType>& type,
+ MemoryPool* pool,
std::shared_ptr<Array>* out) {
+ const auto& fixed_size_binary_array =
+ static_cast<const ::arrow::FixedSizeBinaryArray&>(array);
Review comment:
done.
##########
File path: cpp/src/parquet/arrow/reader_internal.cc
##########
@@ -369,225 +371,134 @@ Status TransferBinary(RecordReader* reader, MemoryPool*
pool,
}
// ----------------------------------------------------------------------
-// INT32 / INT64 / BYTE_ARRAY / FIXED_LEN_BYTE_ARRAY -> Decimal128
-
-static uint64_t BytesToInteger(const uint8_t* bytes, int32_t start, int32_t
stop) {
- const int32_t length = stop - start;
-
- DCHECK_GE(length, 0);
- DCHECK_LE(length, 8);
-
- switch (length) {
- case 0:
- return 0;
- case 1:
- return bytes[start];
- case 2:
- return FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
- case 3: {
- const uint64_t first_two_bytes =
FromBigEndian(SafeLoadAs<uint16_t>(bytes + start));
- const uint64_t last_byte = bytes[stop - 1];
- return first_two_bytes << 8 | last_byte;
- }
- case 4:
- return FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- case 5: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t last_byte = bytes[stop - 1];
- return first_four_bytes << 8 | last_byte;
- }
- case 6: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t last_two_bytes =
- FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
- return first_four_bytes << 16 | last_two_bytes;
- }
- case 7: {
- const uint64_t first_four_bytes =
- FromBigEndian(SafeLoadAs<uint32_t>(bytes + start));
- const uint64_t second_two_bytes =
- FromBigEndian(SafeLoadAs<uint16_t>(bytes + start + 4));
- const uint64_t last_byte = bytes[stop - 1];
- return first_four_bytes << 24 | second_two_bytes << 8 | last_byte;
- }
- case 8:
- return FromBigEndian(SafeLoadAs<uint64_t>(bytes + start));
- default: {
- DCHECK(false);
- return UINT64_MAX;
- }
- }
-}
-
-static constexpr int32_t kMinDecimalBytes = 1;
-static constexpr int32_t kMaxDecimalBytes = 16;
-
-/// \brief Convert a sequence of big-endian bytes to one int64_t (high bits)
and one
-/// uint64_t (low bits).
-static void BytesToIntegerPair(const uint8_t* bytes, const int32_t length,
- int64_t* out_high, uint64_t* out_low) {
- DCHECK_GE(length, kMinDecimalBytes);
- DCHECK_LE(length, kMaxDecimalBytes);
-
- // XXX This code is copied from Decimal::FromBigEndian
-
- int64_t high, low;
-
- // Bytes are coming in big-endian, so the first byte is the MSB and
therefore holds the
- // sign bit.
- const bool is_negative = static_cast<int8_t>(bytes[0]) < 0;
-
- // 1. Extract the high bytes
- // Stop byte of the high bytes
- const int32_t high_bits_offset = std::max(0, length - 8);
- const auto high_bits = BytesToInteger(bytes, 0, high_bits_offset);
-
- if (high_bits_offset == 8) {
- // Avoid undefined shift by 64 below
- high = high_bits;
- } else {
- high = -1 * (is_negative && length < kMaxDecimalBytes);
- // Shift left enough bits to make room for the incoming int64_t
- high = SafeLeftShift(high, high_bits_offset * CHAR_BIT);
- // Preserve the upper bits by inplace OR-ing the int64_t
- high |= high_bits;
- }
-
- // 2. Extract the low bytes
- // Stop byte of the low bytes
- const int32_t low_bits_offset = std::min(length, 8);
- const auto low_bits = BytesToInteger(bytes, high_bits_offset, length);
-
- if (low_bits_offset == 8) {
- // Avoid undefined shift by 64 below
- low = low_bits;
- } else {
- // Sign extend the low bits if necessary
- low = -1 * (is_negative && length < 8);
- // Shift left enough bits to make room for the incoming int64_t
- low = SafeLeftShift(low, low_bits_offset * CHAR_BIT);
- // Preserve the upper bits by inplace OR-ing the int64_t
- low |= low_bits;
- }
-
- *out_high = high;
- *out_low = static_cast<uint64_t>(low);
+// INT32 / INT64 / BYTE_ARRAY / FIXED_LEN_BYTE_ARRAY -> Decimal128 ||
Decimal256
+
+template <typename DecimalType>
+Status RawBytesToDecimalBytes(const uint8_t* value, int32_t byte_width,
+ uint8_t* out_buf) {
+ ARROW_ASSIGN_OR_RAISE(DecimalType t, DecimalType::FromBigEndian(value,
byte_width));
+ t.ToBytes(out_buf);
+ return ::arrow::Status::OK();
}
-static inline void RawBytesToDecimalBytes(const uint8_t* value, int32_t
byte_width,
- uint8_t* out_buf) {
- // view the first 8 bytes as an unsigned 64-bit integer
- auto low = reinterpret_cast<uint64_t*>(out_buf);
+template <typename DecimalArrayType>
+struct DecimalTypeTrait;
- // view the second 8 bytes as a signed 64-bit integer
- auto high = reinterpret_cast<int64_t*>(out_buf + sizeof(uint64_t));
-
- // Convert the fixed size binary array bytes into a Decimal128 compatible
layout
- BytesToIntegerPair(value, byte_width, high, low);
-}
-
-template <typename T>
-Status ConvertToDecimal128(const Array& array, const
std::shared_ptr<DataType>&,
- MemoryPool* pool, std::shared_ptr<Array>*) {
- return Status::NotImplemented("not implemented");
-}
+template <>
+struct DecimalTypeTrait<::arrow::Decimal128Array> {
+ using value = ::arrow::Decimal128;
+};
template <>
-Status ConvertToDecimal128<FLBAType>(const Array& array,
- const std::shared_ptr<DataType>& type,
- MemoryPool* pool, std::shared_ptr<Array>*
out) {
- const auto& fixed_size_binary_array =
- static_cast<const ::arrow::FixedSizeBinaryArray&>(array);
-
- // The byte width of each decimal value
- const int32_t type_length =
- static_cast<const ::arrow::Decimal128Type&>(*type).byte_width();
-
- // number of elements in the entire array
- const int64_t length = fixed_size_binary_array.length();
-
- // Get the byte width of the values in the FixedSizeBinaryArray. Most of the
time
- // this will be different from the decimal array width because we write the
minimum
- // number of bytes necessary to represent a given precision
- const int32_t byte_width =
- static_cast<const
::arrow::FixedSizeBinaryType&>(*fixed_size_binary_array.type())
- .byte_width();
- if (byte_width < kMinDecimalBytes || byte_width > kMaxDecimalBytes) {
- return Status::Invalid("Invalid FIXED_LEN_BYTE_ARRAY length for
Decimal128");
+struct DecimalTypeTrait<::arrow::Decimal256Array> {
+ using value = ::arrow::Decimal256;
+};
+
+template <typename DecimalArrayType, typename ParquetType>
+struct DecimalConverter {
+ static inline Status ConvertToDecimal(const Array& array,
+ const std::shared_ptr<DataType>&,
+ MemoryPool* pool,
std::shared_ptr<Array>*) {
+ return Status::NotImplemented("not implemented");
}
-
- // allocate memory for the decimal array
- ARROW_ASSIGN_OR_RAISE(auto data, ::arrow::AllocateBuffer(length *
type_length, pool));
-
- // raw bytes that we can write to
- uint8_t* out_ptr = data->mutable_data();
-
- // convert each FixedSizeBinary value to valid decimal bytes
- const int64_t null_count = fixed_size_binary_array.null_count();
- if (null_count > 0) {
- for (int64_t i = 0; i < length; ++i, out_ptr += type_length) {
- if (!fixed_size_binary_array.IsNull(i)) {
- RawBytesToDecimalBytes(fixed_size_binary_array.GetValue(i),
byte_width, out_ptr);
+};
+
+template <typename DecimalArrayType>
+struct DecimalConverter<DecimalArrayType, FLBAType> {
+ static inline Status ConvertToDecimal(const Array& array,
+ const std::shared_ptr<DataType>& type,
+ MemoryPool* pool,
std::shared_ptr<Array>* out) {
+ const auto& fixed_size_binary_array =
+ static_cast<const ::arrow::FixedSizeBinaryArray&>(array);
+
+ // The byte width of each decimal value
+ const int32_t type_length =
+ static_cast<const ::arrow::Decimal128Type&>(*type).byte_width();
+
+ // number of elements in the entire array
+ const int64_t length = fixed_size_binary_array.length();
+
+ // Get the byte width of the values in the FixedSizeBinaryArray. Most of
the time
+ // this will be different from the decimal array width because we write
the minimum
+ // number of bytes necessary to represent a given precision
+ const int32_t byte_width =
+ static_cast<const
::arrow::FixedSizeBinaryType&>(*fixed_size_binary_array.type())
+ .byte_width();
+ // allocate memory for the decimal array
+ ARROW_ASSIGN_OR_RAISE(auto data, ::arrow::AllocateBuffer(length *
type_length, pool));
+
+ // raw bytes that we can write to
+ uint8_t* out_ptr = data->mutable_data();
+
+ // convert each FixedSizeBinary value to valid decimal bytes
+ const int64_t null_count = fixed_size_binary_array.null_count();
+
+ using DecimalType = typename DecimalTypeTrait<DecimalArrayType>::value;
+ if (null_count > 0) {
+ for (int64_t i = 0; i < length; ++i, out_ptr += type_length) {
+ if (!fixed_size_binary_array.IsNull(i)) {
Review comment:
yes, nice catch, this appears to be an existing bug. Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
