sunchao / parquet-rs

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

Patch record_reader_10k_collect benchmark that fails with Int96 conversion #202

Closed sadikovi closed 5 years ago

sadikovi commented 5 years ago

This PR tries to mitigate the failure in record_reader_10k_collect benchmark by passing projection that excludes Int96 column. The error that I got was:

running 1 test
test record_reader_10k_collect             ... FAILED

failures:

---- record_reader_10k_collect stdout ----
thread 'main' panicked at 'Expected non-negative milliseconds when converting Int96, found -210866803200000', src/record/api.rs:504:7
note: Run with `RUST_BACKTRACE=1` for a backtrace.

I simply pass projection on every iteration of the benchmark. Unfortunately this adds an overhead of parsing the string into schema every time we run benchmark, but IMHO it is negligible compared to actual reading of records.

I will revert the patch once Int96 conversion works correctly for negative timestamps.

The benchmark runs after the patch.

Closes #201

sadikovi commented 5 years ago

How come we did not see the failure earlier? Does CI run benchmarks?

sunchao commented 5 years ago

How come we did not see the failure earlier? Does CI run benchmarks?

CI does run the benchmark. It's at the bottom of the output, and you'll need to expand it to examine. Seems the benchmark started to fail since the int96 code change.

sunchao commented 5 years ago

Merged. Thanks @sadikovi !

sadikovi commented 5 years ago

Thanks!

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 696


Totals Coverage Status
Change from base Build 690: 0.0%
Covered Lines: 13195
Relevant Lines: 13777

💛 - Coveralls