-
Notifications
You must be signed in to change notification settings - Fork 59
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
Implement statistics::reduce for int96 #223
base: main
Are you sure you want to change the base?
Implement statistics::reduce for int96 #223
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR! Could we add a test confirming that the order is correct for timestamps backed by int96?
}, | ||
PhysicalType::Int96 => { | ||
let stats = stats.iter().map(|x| x.as_any().downcast_ref().unwrap()); | ||
Some(Arc::new(reduce_primitive::<[u32; 3], _>(stats))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the order of [u32; 3] the same as its corresponding representation of a timestamp? I.e. don't we need to transform it and then apply the reduce?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming reading is handled before we get to fold:
deserialize_statistics
-> primitive::read::<[u32; 3]>
-> types::decode(x)
-> T::from_le_bytes
.
Then, once we've got NativeType<[u32; 3]>
we can use ord it implements ( int96_to_i64_ns(*self).ord(&int96_to_i64_ns(*other))
).
Am I missing something?
Tests seem to be broken in
Do you have any preferences on how to impl test for int96 timestamps? Should I add completely new series of |
to avoid failing with "not implemented".