[
https://issues.apache.org/jira/browse/AVRO-3331?focusedWorklogId=719355&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-719355
]
ASF GitHub Bot logged work on AVRO-3331:
----------------------------------------
Author: ASF GitHub Bot
Created on: 02/Feb/22 13:14
Start Date: 02/Feb/22 13:14
Worklog Time Spent: 10m
Work Description: tschilling-natzka commented on a change in pull request
#1501:
URL: https://github.com/apache/avro/pull/1501#discussion_r797587079
##########
File path: lang/rust/src/decimal.rs
##########
@@ -45,30 +67,271 @@ impl Decimal {
let sign_byte = 0xFF * u8::from(self.value.sign() == Sign::Minus);
let mut decimal_bytes = vec![sign_byte; len];
let raw_bytes = self.value.to_signed_bytes_be();
- let num_raw_bytes = raw_bytes.len();
- let start_byte_index =
len.checked_sub(num_raw_bytes).ok_or(Error::SignExtend {
+ let raw_bytes_len = raw_bytes.len();
+ let start_byte_index =
len.checked_sub(raw_bytes_len).ok_or(Error::SignExtend {
requested: len,
- needed: num_raw_bytes,
+ needed: raw_bytes_len,
})?;
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ pub fn from_f64(decimal: f64) -> AvroResult<Self> {
+ if decimal.is_nan() {
+ return Err(Error::DecimalPrecisionNAN);
+ }
+
+ let as_str = format!("{}", decimal);
+ let (decimal_str, precision, scale) = match as_str.find('.') {
+ Some(index) => {
+ let lhs = as_str.get(..index).unwrap();
+ let rhs = as_str.get(index + 1..).unwrap();
+ let number = format!("{}{}", lhs, rhs);
+ let rhs_len = rhs.len();
+ (number, lhs.len() + rhs_len, rhs_len)
+ }
+ None => (as_str.clone(), as_str.len(), 0),
+ };
+
+ // f64::MAX is 309 digits, so we can't have more than 308.
+ if precision > 308 {
+ return Err(Error::DecimalPrecisionTooLarge { precision });
+ }
+
+ Ok(Self {
+ value: BigInt::from_str(decimal_str.as_str()).unwrap(),
+ len: decimal_str.len(),
+ precision,
+ scale,
+ })
+ }
+
+ /// Create a new decimal from a signed Big-Endian encoded byte array
+ pub fn from_bytes<T: AsRef<[u8]>>(
+ bytes: T,
+ precision: usize,
+ scale: usize,
+ ) -> AvroResult<Self> {
+ let bytes_ref = bytes.as_ref();
+ let big_int = BigInt::from_signed_bytes_be(bytes_ref);
+
+ let as_string = big_int.to_string();
Review comment:
This seems very inefficient. You only need to compare it to `abs(x) <
BigInt(10).powi(precision + 1)` which should be much faster to compute than the
printed string.
##########
File path: lang/rust/src/decimal.rs
##########
@@ -45,30 +67,271 @@ impl Decimal {
let sign_byte = 0xFF * u8::from(self.value.sign() == Sign::Minus);
let mut decimal_bytes = vec![sign_byte; len];
let raw_bytes = self.value.to_signed_bytes_be();
- let num_raw_bytes = raw_bytes.len();
- let start_byte_index =
len.checked_sub(num_raw_bytes).ok_or(Error::SignExtend {
+ let raw_bytes_len = raw_bytes.len();
+ let start_byte_index =
len.checked_sub(raw_bytes_len).ok_or(Error::SignExtend {
requested: len,
- needed: num_raw_bytes,
+ needed: raw_bytes_len,
})?;
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ pub fn from_f64(decimal: f64) -> AvroResult<Self> {
+ if decimal.is_nan() {
+ return Err(Error::DecimalPrecisionNAN);
+ }
+
+ let as_str = format!("{}", decimal);
+ let (decimal_str, precision, scale) = match as_str.find('.') {
+ Some(index) => {
+ let lhs = as_str.get(..index).unwrap();
+ let rhs = as_str.get(index + 1..).unwrap();
+ let number = format!("{}{}", lhs, rhs);
+ let rhs_len = rhs.len();
+ (number, lhs.len() + rhs_len, rhs_len)
+ }
+ None => (as_str.clone(), as_str.len(), 0),
+ };
+
+ // f64::MAX is 309 digits, so we can't have more than 308.
+ if precision > 308 {
+ return Err(Error::DecimalPrecisionTooLarge { precision });
+ }
+
+ Ok(Self {
+ value: BigInt::from_str(decimal_str.as_str()).unwrap(),
+ len: decimal_str.len(),
+ precision,
+ scale,
+ })
+ }
+
+ /// Create a new decimal from a signed Big-Endian encoded byte array
+ pub fn from_bytes<T: AsRef<[u8]>>(
+ bytes: T,
+ precision: usize,
+ scale: usize,
+ ) -> AvroResult<Self> {
+ let bytes_ref = bytes.as_ref();
+ let big_int = BigInt::from_signed_bytes_be(bytes_ref);
+
+ let as_string = big_int.to_string();
+ let digits = as_string.len();
+
+ if digits > precision {
+ return Err(Error::ComparePrecisionAndSize {
+ precision,
+ num_bytes: digits,
+ });
+ }
+
+ Ok(Self {
+ value: big_int,
+ len: bytes_ref.len(),
+ precision,
+ scale,
+ })
+ }
+
+ pub fn scale(&self) -> usize {
+ self.scale
+ }
+
+ pub fn precision(&self) -> usize {
+ self.precision
+ }
+
+ pub fn to_f64(&self) -> f64 {
+ let float = self.value.to_f64().unwrap();
+
+ if self.scale == 0 {
+ float
+ } else {
+ let mut f = float;
+ let mut scale = self.scale;
+ while scale > 0 {
+ f /= 10.0;
Review comment:
I think the while loop should be replaced by `f /= (10.0).powi(scale)`.
Again, `rust_decimal` seems to choose a [more complicated
implementation](https://docs.rs/rust_decimal/1.21.0/src/rust_decimal/decimal.rs.html#2148),
which may improve precision.
So, I'm not sure this `avro` should provide this. It should just provide
means to extract the mantissa (the bigint) and the scale (and maybe precision).
It can be convenient, but I don't think it's really needed. The whole point
of `Decimal` is not to lose precision due to rounding.
##########
File path: lang/rust/src/decimal.rs
##########
@@ -45,30 +67,271 @@ impl Decimal {
let sign_byte = 0xFF * u8::from(self.value.sign() == Sign::Minus);
let mut decimal_bytes = vec![sign_byte; len];
let raw_bytes = self.value.to_signed_bytes_be();
- let num_raw_bytes = raw_bytes.len();
- let start_byte_index =
len.checked_sub(num_raw_bytes).ok_or(Error::SignExtend {
+ let raw_bytes_len = raw_bytes.len();
+ let start_byte_index =
len.checked_sub(raw_bytes_len).ok_or(Error::SignExtend {
requested: len,
- needed: num_raw_bytes,
+ needed: raw_bytes_len,
})?;
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ pub fn from_f64(decimal: f64) -> AvroResult<Self> {
Review comment:
I'm not convinced this method should be provided. Looking at the
`rust_decimal` library it seems to be rather complicated to implement
correctly:
`https://docs.rs/rust_decimal/1.21.0/src/rust_decimal/decimal.rs.html#1861`
I think `from_bytes(bytes, scale, precision)` makes sense. And something
based on integers would make sense:
```rust
from_i128(mantissa: i128, scale: u32, precision: u32)
```
If users want to convert from `f64` they can go via `rust_decimal`.
--
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]
Issue Time Tracking
-------------------
Worklog Id: (was: 719355)
Time Spent: 20m (was: 10m)
> Rust: Cannot extract Decimal value
> ----------------------------------
>
> Key: AVRO-3331
> URL: https://issues.apache.org/jira/browse/AVRO-3331
> Project: Apache Avro
> Issue Type: Bug
> Components: rust
> Reporter: Thomas Schilling
> Assignee: Martin Tzvetanov Grigorov
> Priority: Major
> Labels: pull-request-available
> Time Spent: 20m
> Remaining Estimate: 0h
>
> The [Decimal|https://docs.rs/avro-rs/latest/avro_rs/struct.Decimal.html] type
> (as of 0.13) doesn't have any methods or trait implementations that allow
> users to extract the value. The only workaround I could think of would be to
> parse the output of the `Debug` implementation. But that's clearly very
> inefficient and fragile.
> The (unreleased?) version 0.14 also seems to have the same issue
> ([source|https://github.com/apache/avro/blob/master/lang/rust/src/decimal.rs]).
> It might just be enough to make the {{to_vec }}method public and specify how
> to interpret the result vector.
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
