Copilot commented on code in PR #998:
URL: https://github.com/apache/iceberg-cpp/pull/998#discussion_r4250938702
##########
src/iceberg/avro/avro_data_util.cc:
##########
@@ -669,6 +671,27 @@ constexpr int64_t kValueBranch = 1;
} // namespace
+Status EncodeDecimalToFixed(const ::arrow::Decimal128& value, size_t
fixed_size,
+ std::vector<uint8_t>* bytes) {
+ ICEBERG_PRECHECK(fixed_size > 0, "Avro decimal fixed size must be positive");
+ // Arrow's bytes are native-endian; Avro fixed decimals are big-endian.
+ auto big_endian = value.ToBytes();
+ if constexpr (std::endian::native == std::endian::little) {
+ std::ranges::reverse(big_endian);
+ }
Review Comment:
`::arrow::Decimal128::ToBytes()` is defined by Arrow to return a
little-endian two’s-complement byte array (Arrow’s in-memory format is
little-endian regardless of host). Reversing conditionally on
`std::endian::native` will produce incorrect Avro output on big-endian hosts
(it would skip the needed reversal). Prefer treating `ToBytes()` as
little-endian unconditionally and always converting to big-endian for Avro
fixed decimals (e.g., always reverse, or use an explicit little->big endian
conversion utility) and update the comment accordingly.
##########
src/iceberg/arrow_row_builder.cc:
##########
@@ -123,6 +124,15 @@ Status AppendDouble(ArrowArray* array, double value) {
return {};
}
+Status AppendDecimal(ArrowArray* array, const Decimal& value) {
+ // Append the unscaled integer; the array schema supplies precision and
scale.
+ ArrowDecimal decimal;
+ ArrowDecimalInit(&decimal, Decimal::kBitWidth, Decimal::kMaxPrecision, 0);
+ ArrowDecimalSetBytes(&decimal, value.native_endian_bytes());
+ ICEBERG_NANOARROW_RETURN_UNEXPECTED(ArrowArrayAppendDecimal(array,
&decimal));
+ return {};
+}
Review Comment:
This currently passes `value.native_endian_bytes()` into
`ArrowDecimalSetBytes()`. Since Arrow’s buffer format for decimals is
little-endian regardless of host endianness, using native-endian bytes risks
producing incorrect values on big-endian platforms. Prefer supplying bytes in
Arrow’s required little-endian order (or using a nanoarrow API that sets from
an `int128`/word representation with defined ordering). Also, initializing
`ArrowDecimal` with `Decimal::kMaxPrecision` and scale `0` obscures the
intended precision/scale; if nanoarrow validation tightens, it may start
rejecting mismatches—consider initializing with the actual decimal’s
precision/scale (or the builder schema’s precision/scale) for correctness and
future compatibility.
##########
src/iceberg/manifest/manifest_reader.cc:
##########
@@ -416,6 +417,16 @@ Status ParsePartitionValues(ArrowArrayView* view, int64_t
row_idx,
case ArrowType::NANOARROW_TYPE_DOUBLE:
partition.AddValue(Literal::Double(ArrowArrayViewGetDoubleUnsafe(view,
row_idx)));
break;
+ case ArrowType::NANOARROW_TYPE_DECIMAL128: {
+ const auto& type = static_cast<const DecimalType&>(*field_type);
+ ArrowDecimal value;
+ ArrowDecimalInit(&value, Decimal::kBitWidth, type.precision(),
type.scale());
+ ArrowArrayViewGetDecimalUnsafe(view, row_idx, &value);
+ int128_t unscaled;
+ std::memcpy(&unscaled, value.words, sizeof(unscaled));
+ partition.AddValue(Literal::Decimal(unscaled, type.precision(),
type.scale()));
+ break;
+ }
Review Comment:
Decoding `ArrowDecimal` via `memcpy` from `value.words` into `int128_t` is
endianness/word-order dependent and can break on big-endian architectures
(Arrow/nanoarrow decimal storage is little-endian per Arrow’s format). To make
this portable, decode from a well-defined byte order (e.g., use nanoarrow
helpers to extract decimal bytes in a known endianness, then convert to
`int128_t`, or explicitly assemble from `words[]` with a defined word order and
byte-swap when needed). This also avoids relying on the in-memory layout of
`int128_t`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
