Add 1.1 binary reader support for decimals #757
Conversation
src/lazy/binary/raw/v1_1/reader.rs
Outdated
| // 0d3 | ||
| 0x61, 0x07, | ||
|
|
||
| // -0 |
There was a problem hiding this comment.
| // -0 | |
| // -0d3 |
src/lazy/binary/raw/v1_1/reader.rs
Outdated
| use crate::types::decimal::Decimal; | ||
|
|
||
| #[rustfmt::skip] | ||
| let data: Vec<u8> = vec![ |
There was a problem hiding this comment.
We should add additional test cases for op code 0xF7 and for over-padded exponents and coefficients. You can find a fairly complete set of test cases here and here, and I'd recommend using them rather than spending time coming up with your own.
Also, it would be nice if you could use #[rstest] for a parameterized test here.
There was a problem hiding this comment.
Also, it would be nice if you could use #[rstest] for a parameterized test here.
I've been using the array-of-inputs approach a lot recently, largely because my IDE can't run #[rstest]s natively. I wish the Rust test harness would let you programmatically define a test (for individual PASS/FAIL indications) or Intellij would support macro-generated tests. 🫤
src/lazy/binary/raw/v1_1/value.rs
Outdated
| let coefficient = if coefficient_size > 0 { | ||
| FixedInt::read( | ||
| &value_bytes[exponent.size_in_bytes()..], | ||
| coefficient_size, | ||
| 0, | ||
| )? | ||
| } else { | ||
| 0i64.into() | ||
| }; |
There was a problem hiding this comment.
This makes me think that FixedInt::read should return zero if the size is zero. This seems like something that could come up in other places, and rather than handle this condition everywhere, we should probably just move it to FixedInt::read.
There was a problem hiding this comment.
Matt made this change in PR #762.
src/lazy/binary/raw/v1_1/reader.rs
Outdated
| use crate::types::decimal::Decimal; | ||
|
|
||
| #[rustfmt::skip] | ||
| let data: Vec<u8> = vec![ |
There was a problem hiding this comment.
Also, it would be nice if you could use #[rstest] for a parameterized test here.
I've been using the array-of-inputs approach a lot recently, largely because my IDE can't run #[rstest]s natively. I wish the Rust test harness would let you programmatically define a test (for individual PASS/FAIL indications) or Intellij would support macro-generated tests. 🫤
src/lazy/binary/raw/v1_1/value.rs
Outdated
| let coefficient = if coefficient_size > 0 { | ||
| FixedInt::read( | ||
| &value_bytes[exponent.size_in_bytes()..], | ||
| coefficient_size, | ||
| 0, | ||
| )? | ||
| } else { | ||
| 0i64.into() | ||
| }; |
There was a problem hiding this comment.
Matt made this change in PR #762.
nirosys
force-pushed
the
rgiliam/1_1_binary_reader_decimals
branch
from
1b4f4ed to
6d55b88
Compare
nirosys
force-pushed
the
rgiliam/1_1_binary_reader_decimals
branch
from
f6ed2e4 to
14fa06a
Compare
Issue #, if available: #662
Description of changes:
Add support for decimals to the 1.1 binary raw reader.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.