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

Implement statistics::reduce for int96 #223

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

little-arhat
Copy link

to avoid failing with "not implemented".

Copy link
Owner

@jorgecarleitao jorgecarleitao left a 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)))
Copy link
Owner

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?

Copy link
Author

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?

@little-arhat
Copy link
Author

Tests seem to be broken in main?

(r@futu ~/prj/f/parquet2)>>= git br
  feauture-do-not-fail-on-i96-stat
* main
(r@futu ~/prj/f/parquet2)>>= cargo test
   Compiling parquet2 v0.17.0 (/Users/r/prj/f/parquet2)
error[E0063]: missing field `primitive_type` in initializer of `statistics::primitive::PrimitiveStatistics<_>`
   --> src/write/statistics.rs:300:13
    |
300 |             PrimitiveStatistics {
    |             ^^^^^^^^^^^^^^^^^^^ missing `primitive_type`

error[E0063]: missing field `primitive_type` in initializer of `statistics::primitive::PrimitiveStatistics<_>`
   --> src/write/statistics.rs:306:13
    |
306 |             PrimitiveStatistics {
    |             ^^^^^^^^^^^^^^^^^^^ missing `primitive_type`

error[E0063]: missing field `primitive_type` in initializer of `statistics::primitive::PrimitiveStatistics<_>`
   --> src/write/statistics.rs:317:13
    |
317 |             PrimitiveStatistics {
    |             ^^^^^^^^^^^^^^^^^^^ missing `primitive_type`

For more information about this error, try `rustc --explain E0063`.
error: could not compile `parquet2` (lib test) due to 3 previous errors
warning: build failed, waiting for other jobs to finish...
cargo test  5.03s user 0.90s system 392% cpu 1.510 total

Do you have any preferences on how to impl test for int96 timestamps? Should I add completely new series of wite_file/read_ routines, or shall I add new column somewhere?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants