pitrou commented on code in PR #50916:
URL: https://github.com/apache/arrow/pull/50916#discussion_r4122960757
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -964,6 +1028,22 @@ Status TransferColumnData(RecordReader* reader,
if (descr->physical_type() == ::parquet::Type::INT96) {
RETURN_NOT_OK(
TransferInt96(reader, pool, value_field, &result,
timestamp_type.unit()));
+ } else if (descr->physical_type() ==
::parquet::Type::FIXED_LEN_BYTE_ARRAY) {
+ // Validate that the provided Arrow timestamp unit matches the Parquet
unit.
+ DCHECK(descr->logical_type()->is_timestamp());
+ const auto& ts_logical =
+ checked_cast<const TimestampLogicalType&>(*descr->logical_type());
+ ARROW_ASSIGN_OR_RAISE(auto expected_unit,
+
ArrowTimeUnitFromParquet(ts_logical.time_unit()));
+ if (timestamp_type.unit() != expected_unit) {
Review Comment:
When can this happen?
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -853,6 +854,69 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool*
pool,
return Status::OK();
}
+// Decode a little-endian 96-bit FLBA(12) TIMESTAMP value into a 64-bit Arrow
timestamp.
+// Values that do not fit in the int64 range either error or clamp to the
minimum or
+// maximum int64 value, depending on clamp_on_overflow.
+inline Result<int64_t> FlbaTimestampToInt64(const uint8_t* bytes,
+ bool clamp_on_overflow) {
+ const uint64_t low = bit_util::FromLittleEndian(SafeLoadAs<uint64_t>(bytes));
+ const uint32_t high = bit_util::FromLittleEndian(SafeLoadAs<uint32_t>(bytes
+ 8));
+ const int32_t high_signed = static_cast<int32_t>(high);
+ const int64_t low_signed = static_cast<int64_t>(low);
+ const int32_t sign_extension = (low_signed < 0) ? -1 : 0;
+ // Fits in int64 iff the high part is a pure sign-extension of the low part.
+ if (ARROW_PREDICT_FALSE(high_signed != sign_extension)) {
+ if (!clamp_on_overflow) {
+ return Status::Invalid(
+ "FLBA(12) TIMESTAMP value does not fit in a 64-bit Arrow timestamp");
+ }
+ return high_signed < 0 ? std::numeric_limits<int64_t>::min()
+ : std::numeric_limits<int64_t>::max();
+ }
+ return low_signed;
+}
+
+// Read a TIMESTAMP-annotated FLBA(12) column as a 64-bit Arrow timestamp.
+Status TransferFlbaTimestamp(RecordReader* reader, MemoryPool* pool,
Review Comment:
Here as well, let's just return `Result<Datum>` instead of taking a
out-pointer parameter.
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -37,6 +37,20 @@ using ::arrow::Result;
using ::arrow::Status;
using ::arrow::internal::checked_cast;
+Result<::arrow::TimeUnit::type> ArrowTimeUnitFromParquet(
+ LogicalType::TimeUnit::unit unit) {
+ switch (unit) {
+ case LogicalType::TimeUnit::MILLIS:
+ return ::arrow::TimeUnit::MILLI;
+ case LogicalType::TimeUnit::MICROS:
+ return ::arrow::TimeUnit::MICRO;
+ case LogicalType::TimeUnit::NANOS:
+ return ::arrow::TimeUnit::NANO;
+ default:
+ return Status::Invalid("Unrecognized Parquet time unit");
Review Comment:
I would keep it a `TypeError` as in the original code below.
--
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]
