sunchao / parquet-rs

Apache Parquet implementation in Rust
Apache License 2.0
149 stars 20 forks source link

Add clear error message when Int96 conversion fails #184

Closed sadikovi closed 5 years ago

sadikovi commented 5 years ago

This PR modifies the code to remove overflow error and instead panic with user friendly message.

Right now, due to limitations of chrono library we cannot convert negative timestamps into string dates, and just overall handle negative timestamps, therefore we would panic with an appropriate message.

$ cargo run --bin parquet-read data.rs.bk/timestamps.parquet 
   Compiling parquet v0.4.2 (/parquet-rs)                                                                          
    Finished dev [unoptimized + debuginfo] target(s) in 2.31s                                                                               
     Running `target/debug/parquet-read data.rs.bk/timestamps.parquet`
thread 'main' panicked at 'Expected non-negative milliseconds when converting Int96, found 
-17987529600000', src/record/api.rs:475:7
note: Run with `RUST_BACKTRACE=1` for a backtrace.
sadikovi commented 5 years ago

Relates to #148

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 652


Files with Coverage Reduction New Missed Lines %
record/api.rs 12 97.27%
<!-- Total: 12 -->
Totals Coverage Status
Change from base Build 650: 0.01%
Covered Lines: 12504
Relevant Lines: 13087

đź’› - Coveralls
sadikovi commented 5 years ago

Thanks!

I don’t know, actually. I am thinking if it could be easier to have a library that simply copies Java code for Date and a Calendar, since we only need conversions.

I thought about printing the “unknown” string instead of panicking, but decided to have an explicit error instead. On Sun, 4 Nov 2018 at 6:50 PM, Chao Sun notifications@github.com wrote:

@sunchao approved this pull request.

LGTM. It'd be best if chrono can be fixed to display the timestamps before 01/01/1970, but not sure how hard it is...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sunchao/parquet-rs/pull/184#pullrequestreview-171372140, or mute the thread https://github.com/notifications/unsubscribe-auth/AHbY3iwQyd9QCosSQVqBdlhgY9arwtF3ks5uryjygaJpZM4YNXGd .

sunchao commented 5 years ago

Merged. Thanks @sadikovi .

I don’t know, actually. I am thinking if it could be easier to have a library that simply copies Java code for Date and a Calendar, since we only need conversions.

Yes I feel what we need is much simpler than chrono offers. Feel free to log a ticket. :)