Skip to content
This repository has been archived by the owner on Jan 11, 2021. It is now read-only.

Patch record_reader_10k_collect benchmark that fails with Int96 conversion #202

Merged
merged 2 commits into from
Dec 7, 2018

Conversation

sadikovi
Copy link
Collaborator

@sadikovi sadikovi commented Dec 6, 2018

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 sadikovi requested a review from sunchao December 6, 2018 08:10
@sadikovi
Copy link
Collaborator Author

sadikovi commented Dec 6, 2018

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

Copy link
Owner

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM. Thansk! Please fix the style issue before committing. :-)

@sunchao
Copy link
Owner

sunchao commented Dec 6, 2018

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 sunchao merged commit 6b67aa1 into sunchao:master Dec 7, 2018
@sunchao
Copy link
Owner

sunchao commented Dec 7, 2018

Merged. Thanks @sadikovi !

@sadikovi
Copy link
Collaborator Author

sadikovi commented Dec 7, 2018

Thanks!

@sadikovi sadikovi deleted the patch-reader-benchmark branch December 7, 2018 08:24
@coveralls
Copy link

Pull Request Test Coverage Report for Build 696

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.776%

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

💛 - Coveralls

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

Successfully merging this pull request may close these issues.

3 participants