martin-g commented on code in PR #717:
URL: https://github.com/apache/avro-rs/pull/717#discussion_r4374804140
##########
avro/src/types.rs:
##########
@@ -1159,32 +1159,22 @@ impl Value {
}
InnerDecimalSchema::Bytes => (),
};
- match self {
- Value::Decimal(num) => {
- let num_bytes = num.len();
- if max_prec_for_len(num_bytes)? < precision.get() {
- Err(Details::ComparePrecisionAndSize {
- precision,
- num_bytes,
- }
- .into())
- } else {
- Ok(Value::Decimal(num))
- }
- // check num.bits() here
- }
- Value::Fixed(_, bytes) | Value::Bytes(bytes) => {
- if max_prec_for_len(bytes.len())? < precision.get() {
- Err(Details::ComparePrecisionAndSize {
- precision,
- num_bytes: bytes.len(),
- }
- .into())
- } else {
- // precision and scale match, can we assume the underlying
type can hold the data?
- Ok(Value::Decimal(Decimal::new(bytes)?))
+ // A value can be shorter than the precision allows (2.56 in a
`decimal(5, 2)` takes two
+ // bytes), but it can't have more digits.
+ let check_digits = |num: Decimal| {
+ if num.digits() > precision.get() as u64 {
+ Err(Details::ComparePrecisionAndSize {
+ precision,
+ num_bytes: num.len(),
Review Comment:
why `num.len()` ? The condition above uses `num.digits()`.
> The rejection reuses ComparePrecisionAndSize, whose message still talks
about bytes ("Precision 4 too small to hold decimal values with 2 bytes"). If
you'd prefer, I can add a separate variant that reports the digit count instead.
I think it would be clearer, yes.
##########
avro/src/decimal.rs:
##########
@@ -94,6 +95,11 @@ impl Decimal {
self.len.get()
}
+ /// The number of decimal digits of the unscaled value.
+ pub(crate) fn digits(&self) -> u64 {
+ BigDecimal::from_bigint(self.value.clone(), 0).digits()
Review Comment:
Would `self.value.to_str_radix(10).len()` work too ?
It would avoid the cloning+BigDecimal instantiation.
Please add unit tests with positive, negative, edge values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
