Remove unnecessary Option from Int96 #2471
Conversation
| @@ -843,7 +843,7 @@ mod tests { | |||
| run_test::<Int96Type>( | |||
| -1, | |||
| &[Int96::from(vec![1, 2, 3]), Int96::from(vec![2, 3, 4])], | |||
| 32, | |||
| 24, | |||
There was a problem hiding this comment.
This changes as there is no longer 8 bytes taken up by the Option
| @@ -36,29 +36,27 @@ use crate::util::{ | |||
|
|
|||
| /// Rust representation for logical type INT96, value is backed by an array of `u32`. | |||
| /// The type only takes 12 bytes, without extra padding. | |||
| #[derive(Clone, Debug, PartialOrd, Default)] | |||
| #[derive(Clone, Debug, PartialOrd, Default, PartialEq, Eq)] | |||
There was a problem hiding this comment.
Not a question about this issue.
How to compare the INT96 in parquet? How to ensure the order of two INT64?
From the parquet spec https://github.com/apache/parquet-format/blob/54e53e5d7794d383529dd30746378f19a12afd58/src/main/thrift/parquet.thrift#L36, the type of INT96 has be deprecated.
There was a problem hiding this comment.
It should be signed little endian comparison, not sure this is what is actually implemented though
There was a problem hiding this comment.
It should be signed little endian comparison, not sure this is what is actually implemented though
Maybe I can read the codebase of the parquet-mr of java-version, and answer this.
There was a problem hiding this comment.
https://github.com/apache/arrow-rs/blob/master/parquet/src/column/writer/mod.rs#L1036 doesn't appear to special case this type, so this crate is currently performing unsigned little endian comparison. I don't think this is correct
There was a problem hiding this comment.
https://github.com/apache/arrow-rs/blob/master/parquet/src/column/writer/mod.rs#L1036 doesn't appear to special case this type
Do you think the logic of compare_greater_byte_array_decimals is wrong? From the current implementation and test, it use the signed comparator with little endian byte order.
There was a problem hiding this comment.
Int96 is not a decimal type, but used for timestamps by some older systems such as Spark. 64-bit nanoseconds since midnight, followed by 32-bit Julian day.
| } | ||
|
|
||
| /// Returns underlying data as slice of [`u32`]. | ||
| #[inline] | ||
| pub fn data(&self) -> &[u32] { | ||
| self.value | ||
| .as_ref() | ||
| .expect("set_data should have been called") |
There was a problem hiding this comment.
I guess that it originally wants to make sure data always returns something set instead of an initial value.
There was a problem hiding this comment.
I don't follow why we would care about this for Int96 and not for all the other types. This feels like an unnecessary quirk?
|
Benchmark runs are scheduled for baseline = 4e4902f and contender = 5e8586a. 5e8586a is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #1661
Rationale for this change
I can't see an obvious reason for this Option existing, as nothing seems to be exploiting it. Removing it will allow reading Int96 as a regular "primitive" type
What changes are included in this PR?
Are there any user-facing changes?
Theoretically now you can call
Int96::databefore callingset_dataand it won't panic, not sure this counts...