Repository navigation
fix: Accept short decimal values when resolving against a decimal schema - #717
Conversation
Resolving a value against a decimal schema failed unless the value's byte length could hold `precision` digits, so 2.56 (two bytes) could not be read as a `decimal(5, 2)` whenever a reader schema was given. The byte length only bounds how large a value can be; it does not limit the precision the value is read with. Remove the per-value check and the now unused `Decimal::len()`. The check that a fixed size can hold the precision stays.
|
I think allowing lower precision inputs is fine, but this now also allows higher precision inputs. The spec mentions the following: |
The precision is the maximum number of digits of a decimal, so a value that is shorter than the precision allows still resolves, but one with more digits (12345 for a precision of 4) is an error again. This uses the number of digits of the unscaled value instead of the byte length.
|
Good point, thanks. Resolving now checks the number of decimal digits of the unscaled value against the precision, so a value with more digits than the precision is rejected again, and shorter values still pass (589b0b7). The new test covers 12345 with precision 4 (rejected) and 5 (accepted), and -99999 (accepted) and -100000 (rejected) with precision 5, for both Two existing tests changed with it: The rejection reuses |
|
Small update in dc9a8b6: the new test now uses |
| if num.digits() > precision.get() as u64 { | ||
| Err(Details::ComparePrecisionAndSize { | ||
| precision, | ||
| num_bytes: num.len(), |
There was a problem hiding this 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.
There was a problem hiding this 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.
|
|
||
| /// The number of decimal digits of the unscaled value. | ||
| pub(crate) fn digits(&self) -> u64 { | ||
| BigDecimal::from_bigint(self.value.clone(), 0).digits() |
There was a problem hiding this 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.
There was a problem hiding this 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.
Resolving a decimal value with more digits than the precision now fails with the new ComparePrecisionAndDigits error, which reports the number of digits that was checked, instead of ComparePrecisionAndSize, whose message gave the length in bytes. ComparePrecisionAndSize is no longer generated and is deprecated. Decimal::digits now takes the length of the magnitude in base 10, which leaves out the sign of a negative value and avoids cloning the value into a BigDecimal.
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
|
The only thing I don't like about this is how we now calculate the amount of digits. But I also don't know a better way. |
Reading a bytes-backed decimal with a reader schema fails for most values. Writer and reader are both
{"type": "bytes", "logicalType": "decimal", "precision": 5, "scale": 2}, and the datum is 2.56 (unscaled 256, encoded as the two bytes01 00). Resolution fails:The same happens through
GenericDatumReaderandReaderwhenever a reader schema is given, and for any value whose byte length is small for the declared precision: 1.00 in adecimal(10, 2)(one byte) fails too. Reading without a reader schema works, and Java reads these values.Value::resolve_decimalcomparedmax_prec_for_len(value_length)with the schema's precision for every value. That bound says how many digits a fixed of that size can always hold, which is a constraint on the fixed size of a fixed-backed decimal. It says nothing about whether a particular value fits. A short value always has few digits, so the check rejected the values that are least likely to overflow. This removes the per-value check. The schema-level check, that a fixed size can hold the precision, stays.Decimal::len()had no other users and is removed with it.Test:
resolve_decimal_value_shorter_than_precisionresolves a two-byteValue::DecimalandValue::Bytesagainstdecimal(5, 2). It fails with the error above without the change and passes with it.cargo fmt --all --check,cargo clippy --all-features --all-targets --workspaceandcargo test --all-featurespass.