Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary Option from Int96 #2471

Merged
merged 2 commits into from
Aug 17, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions parquet/src/data_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
pub struct Int96 {
value: Option<[u32; 3]>,
value: [u32; 3],
}

impl Int96 {
/// Creates new INT96 type struct with no data set.
pub fn new() -> Self {
Self { value: None }
Self { value: [0; 3] }
}

/// Returns underlying data as slice of [`u32`].
#[inline]
pub fn data(&self) -> &[u32] {
self.value
.as_ref()
.expect("set_data should have been called")
Copy link
Member

@viirya viirya Aug 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that it originally wants to make sure data always returns something set instead of an initial value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

&self.value
}

/// Sets data for this INT96 type.
#[inline]
pub fn set_data(&mut self, elem0: u32, elem1: u32, elem2: u32) {
self.value = Some([elem0, elem1, elem2]);
self.value = [elem0, elem1, elem2];
}

/// Converts this INT96 into an i64 representing the number of MILLISECONDS since Epoch
Expand All @@ -75,16 +73,6 @@ impl Int96 {
}
}

impl PartialEq for Int96 {
fn eq(&self, other: &Int96) -> bool {
match (&self.value, &other.value) {
(Some(v1), Some(v2)) => v1 == v2,
(None, None) => true,
_ => false,
}
}
}

impl From<Vec<u32>> for Int96 {
fn from(buf: Vec<u32>) -> Self {
assert_eq!(buf.len(), 3);
Expand Down
2 changes: 1 addition & 1 deletion parquet/src/encodings/encoding/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ mod tests {
run_test::<Int96Type>(
-1,
&[Int96::from(vec![1, 2, 3]), Int96::from(vec![2, 3, 4])],
32,
24,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes as there is no longer 8 bytes taken up by the Option

);
run_test::<ByteArrayType>(
-1,
Expand Down