GH-50915: [FORMAT] Allow TIMESTAMP logical type to annotate FIXED_LEN_BYTE_ARRAY(12) - #50916
GH-50915: [FORMAT] Allow TIMESTAMP logical type to annotate FIXED_LEN_BYTE_ARRAY(12)#50916divjotarora wants to merge 1 commit into
Conversation
|
|
github-actions
Bot
added
awaiting review
|
|
||
| return ::arrow::fixed_size_binary(physical_length); | ||
| case LogicalType::Type::TIMESTAMP: | ||
| return ::arrow::fixed_size_binary(physical_length); |
There was a problem hiding this comment.
I think this should probably be configurable. We should probably have a mode that takes returns the arrow timestamp type (and either errors on overflow or converts to MIN/MAX representable values., maybe a different config value?)
| ASSERT_EQ(kNumRows, levels_read); | ||
| ASSERT_EQ(kNumRows, values_read); | ||
| min_value.assign(reinterpret_cast<const char*>(values[kMinRow].ptr), 12); | ||
| max_value.assign(reinterpret_cast<const char*>(values[kMaxRow].ptr), 12); |
There was a problem hiding this comment.
would it also make sense to make sure our comparator is consistent with these values?
| const bool a_neg = (a.ptr[11] & 0x80) != 0; | ||
| const bool b_neg = (b.ptr[11] & 0x80) != 0; | ||
| if (a_neg != b_neg) return a_neg; | ||
| for (int i = 11; i >= 0; --i) if (a.ptr[i] != b.ptr[i]) return a.ptr[i] < b.ptr[i]; |
There was a problem hiding this comment.
do the two step integer comparison?
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
Some questions and comments I think the biggest one is scope and whether we should have an option to convert this value to a proper arrow type. Wemight also want to make it configurable the target of the arrow type
2 participants
Rationale for this change
See apache/parquet-format#600 for rationale.
What changes are included in this PR?
This PR adds support for using
TimestampTypeto annotateFIXED_LEN_BYTE_ARRAY(12)values.Are these changes tested?
Yes, via unit tests and an e2e test that reads the file added in parquet-testing (apache/parquet-testing#123).
Are there any user-facing changes?
No