wleeaf commented on code in PR #717:
URL: https://github.com/apache/avro-rs/pull/717#discussion_r4376399035
##########
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:
The check and the error now use the same digit count: `check_digits`
computes `num.digits()` once, compares it with the precision and reports it
(ed394df). The rejection uses a new `ComparePrecisionAndDigits { precision,
digits }` variant, e.g. "Precision 5 too small to hold a decimal value with 6
digits". `ComparePrecisionAndSize` isn't generated anymore, so it's marked
deprecated like the other unused variants; I can drop that attribute if you'd
rather keep it as is.
##########
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:
Yes. `digits()` is now `self.value.magnitude().to_str_radix(10).len()`
(ed394df). It uses the magnitude because `to_str_radix` on a negative `BigInt`
includes the `-`. The new `decimal_digits` test covers 0, ±1, ±99999, ±100000,
the i64 and i128 bounds and sign-extended input, and the resolve test now also
checks 0 and 100000 for decimal(5, 2). `Decimal::len()` had no callers left, so
it's removed.
--
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]
