divjotarora commented on code in PR #50916:
URL: https://github.com/apache/arrow/pull/50916#discussion_r3926399112
##########
cpp/src/parquet/properties.h:
##########
@@ -1157,7 +1157,9 @@ class PARQUET_EXPORT ArrowReaderProperties {
list_type_(kArrowDefaultListType),
arrow_extensions_enabled_(false),
should_load_statistics_(false),
- smallest_decimal_enabled_(false) {}
+ smallest_decimal_enabled_(false),
+ convert_flba_timestamps_(false),
Review Comment:
SGTM, changed
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -855,6 +856,49 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool*
pool,
return Status::OK();
}
+// Read a TIMESTAMP-annotated FLBA(12) column as a 64-bit Arrow timestamp.
Values that do
+// not fit in the 64 bit range either error or clamp to min/max int64,
depending on
+// configuration.
+Status TransferFlbaTimestamp(RecordReader* reader, MemoryPool* pool,
+ const std::shared_ptr<Field>& field, Datum* out,
+ bool clamp_on_overflow) {
+ static const auto binary_type = ::arrow::fixed_size_binary(12);
+ std::shared_ptr<ChunkedArray> chunked_array;
+ RETURN_NOT_OK(
Review Comment:
I did this originally because FLBA values can't use `reader->values()` as
it's not populated in the FLBA reader, but I changed it now to use
`reader->GetBuilderChunks()` similar to the decimal code.
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -855,6 +856,49 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool*
pool,
return Status::OK();
}
+// Read a TIMESTAMP-annotated FLBA(12) column as a 64-bit Arrow timestamp.
Values that do
+// not fit in the 64 bit range either error or clamp to min/max int64,
depending on
+// configuration.
+Status TransferFlbaTimestamp(RecordReader* reader, MemoryPool* pool,
+ const std::shared_ptr<Field>& field, Datum* out,
+ bool clamp_on_overflow) {
+ static const auto binary_type = ::arrow::fixed_size_binary(12);
+ std::shared_ptr<ChunkedArray> chunked_array;
+ RETURN_NOT_OK(
+ TransferBinary(reader, pool, field->WithType(binary_type),
&chunked_array));
+
+ ::arrow::TimestampBuilder builder(field->type(), pool);
+ RETURN_NOT_OK(builder.Reserve(chunked_array->length()));
+ for (const auto& chunk : chunked_array->chunks()) {
+ const auto& values = checked_cast<const
::arrow::FixedSizeBinaryArray&>(*chunk);
+ for (int64_t i = 0; i < values.length(); ++i) {
+ if (values.IsNull(i)) {
+ builder.UnsafeAppendNull();
+ continue;
+ }
+ const uint8_t* bytes = values.GetValue(i);
+ const uint64_t low =
bit_util::FromLittleEndian(SafeLoadAs<uint64_t>(bytes));
+ const uint32_t high =
bit_util::FromLittleEndian(SafeLoadAs<uint32_t>(bytes + 8));
+ const int64_t low_signed = static_cast<int64_t>(low);
+ // Fits in int64 iff the high part is a pure sign-extension of the low
part.
+ if (static_cast<int32_t>(high) != (low_signed < 0 ? -1 : 0)) {
+ if (!clamp_on_overflow) {
+ return Status::Invalid(
+ "FLBA(12) TIMESTAMP value does not fit in a 64-bit Arrow
timestamp");
+ }
+ const bool negative = (bytes[11] & 0x80) != 0;
Review Comment:
This is comparing the high byte, not the low. Added a `high_signed =
static_cast<int32_t>(high)` variable, which removes the cast in the conditional
and can be reused here.
##########
cpp/src/parquet/arrow/schema_internal.cc:
##########
@@ -207,6 +207,13 @@ Result<std::shared_ptr<ArrowType>> FromFLBA(
return ::arrow::extension::uuid();
}
+ return ::arrow::fixed_size_binary(physical_length);
+ case LogicalType::Type::TIMESTAMP:
+ // If configured, convert to a potentially lossy Arrow timestamp.
Otherwise, return
Review Comment:
Removed the second sentence
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -855,6 +856,49 @@ Status TransferHalfFloat(RecordReader* reader, MemoryPool*
pool,
return Status::OK();
}
+// Read a TIMESTAMP-annotated FLBA(12) column as a 64-bit Arrow timestamp.
Values that do
+// not fit in the 64 bit range either error or clamp to min/max int64,
depending on
+// configuration.
+Status TransferFlbaTimestamp(RecordReader* reader, MemoryPool* pool,
+ const std::shared_ptr<Field>& field, Datum* out,
+ bool clamp_on_overflow) {
+ static const auto binary_type = ::arrow::fixed_size_binary(12);
+ std::shared_ptr<ChunkedArray> chunked_array;
+ RETURN_NOT_OK(
+ TransferBinary(reader, pool, field->WithType(binary_type),
&chunked_array));
+
+ ::arrow::TimestampBuilder builder(field->type(), pool);
+ RETURN_NOT_OK(builder.Reserve(chunked_array->length()));
+ for (const auto& chunk : chunked_array->chunks()) {
+ const auto& values = checked_cast<const
::arrow::FixedSizeBinaryArray&>(*chunk);
+ for (int64_t i = 0; i < values.length(); ++i) {
+ if (values.IsNull(i)) {
+ builder.UnsafeAppendNull();
+ continue;
+ }
+ const uint8_t* bytes = values.GetValue(i);
+ const uint64_t low =
bit_util::FromLittleEndian(SafeLoadAs<uint64_t>(bytes));
+ const uint32_t high =
bit_util::FromLittleEndian(SafeLoadAs<uint32_t>(bytes + 8));
+ const int64_t low_signed = static_cast<int64_t>(low);
+ // Fits in int64 iff the high part is a pure sign-extension of the low
part.
+ if (static_cast<int32_t>(high) != (low_signed < 0 ? -1 : 0)) {
+ if (!clamp_on_overflow) {
+ return Status::Invalid(
+ "FLBA(12) TIMESTAMP value does not fit in a 64-bit Arrow
timestamp");
+ }
+ const bool negative = (bytes[11] & 0x80) != 0;
+ builder.UnsafeAppend(negative ? INT64_MIN : INT64_MAX);
+ } else {
+ builder.UnsafeAppend(low_signed);
Review Comment:
After further investigation, the Parquet --> Arrow type conversion always
yields an Arrow timestamp with the same time unit as the Parquet one, so
scaling is not needed. I added a defensive check up before
`TransferFlbaTimestamp` is called to codify this, but it just returns an error
if they don't match.
--
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]
