You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "ftelnov (via GitHub)" <gi...@apache.org> on 2023/06/20 10:36:18 UTC
[GitHub] [avro] ftelnov opened a new pull request, #2299: AVRO-3781: [Rust] Enhance Decimal resolve
ftelnov opened a new pull request, #2299:
URL: https://github.com/apache/avro/pull/2299
This PR solves [issue](https://issues.apache.org/jira/browse/AVRO-3781).
What was done:
1) Now decimals could be resolved from strings and floats;
2) Add special algorithm for string parsing into appropriate bigint;
3) Floats parsing is done via converting to string first, then to the bigint through the string parsing algorithm.
4) Add method for estimating the precision of the decimal - in other words, added a method to check the amount of bigint digits.
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "cmpute (via GitHub)" <gi...@apache.org>.
cmpute commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1783650626
Thanks for your interest! Some of the benefits of choosing dashu over bigdecimal:
1. dashu is faster: benchmark of exp indicates that `dashu::Decimal` calculates exp(1) with 100 digits in 10ms while `bigdecimal::BigDecimal` spends 15ms (on my laptop). You can try [the benchmark](https://github.com/cmpute/dashu/tree/bench/benchmark) here.
2. dashu has thorough context and rounding support: dashu supports arbitrary precision limit at runtime, while bigdecimal only partially support setting precision at compile time. The correctness of bigdecimal with higher precision is not well tested. Dashu supports rounding mode and precision selection for each operation, while bigdecimal's context type only allows you to select them on the very basic operation (addition) yet.
3. dashu supports arbitrary float base: dashu has decimal float type `dashu::Decimal`, it also has the binary float type `dashu::Real`. The decimal type can be converted from and to the binary type efficiently with rounding mode with the method [`with_base`](https://docs.rs/dashu/0.4.0/dashu/float/type.DBig.html#method.with_base) and [`with_base_and_precision`](https://docs.rs/dashu/0.4.0/dashu/float/type.DBig.html#method.with_base_and_precision). Therefore you can have a complete control when you want to convert f32/f64/other binary floats to decimal outputs (you can specify the precision or the rounding mode you want)
4. dashu has seamless and opaque support for conversion between big integers and big decimals through [`from_parts`](https://docs.rs/dashu/0.4.0/dashu/float/type.DBig.html#method.from_parts) and [`into_repr`](https://docs.rs/dashu/0.4.0/dashu/float/type.DBig.html#method.into_repr). After all, dashu is a collection of arbitrary precision integers, rationals and floats.
I can't say that dashu has a better API right now, but I'm working on a guide book in the next release.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1632157345
@ftelnov Do you think you could improve the current support for `decimal` based on bigdecimal/rustdecimal crates ?
This would be a good approach, I think !
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1604178351
> The other day I started reviewing this PR but then I stopped for the following reasons:
>
> * @RyanSkraba said that he is going to release 1.11.2 in the coming days and I didn't want to make such changes in the last moment
>
> * then @clesaec suggested [AVRO-3779: using rust bigdecimal #2302](https://github.com/apache/avro/pull/2302) - using a proper BigDecimal type as a backing type is something that I wanted to explore some day. With it it is much easier to parse from and format to different types (String, floating number, byte array). The drawback here is that this is something new (to be added in the spec) and it may not work for someone who wants to use Avro SDKs which does not support it yet
I think we must switch to BigDecimal anyway. It wraps BigInt and that is what we need to use and not to write ourselves. Therefore, you can freely close this PR, and I'll get with another issue of rewriting decimal to bigdecimal usage, if that is what you would prefer.
I thought @clesaec suggested adding new schema type for some purpose - which is not good in my opinion. His idea to include serialization to the resulted decimal is, indeed, pretty good. It's a complete pain holding schema every time we want to operate with decimal - as without schema you can't understand what is the scale and precision. Maybe there was some idea behind such a decision. However, as you mentioned, it would require spec update.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "Gordon-F (via GitHub)" <gi...@apache.org>.
Gordon-F commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-2270062189
> I think I can move decimals to bigdecimal usage - indeed, that would be an easy task, as they are bigint internally. So it's almost like drop-in replacement.
@ftelnov @martin-g Is it still planned?
It's not an easy task to work with floating point decimal values with current API. Even in the readme examples, there is only an example of working with integer values. And there is no way to represent value (for reader/writer) with a schema like this (please, correct me if I'm wrong):
```json
{
"name": "decimal",
"type": "bytes",
"logicalType": "decimal",
"scale": 8,
"precision": 28
}
```
All of this can be fixed with an internal representation of `Value::Decimal` as `bigdecimal::BigDecimal`. If there is a performance issue, I think it can be fixed by compile-time feature (decimal_as_bigdecimal?).
If these changes are ok, I can work on this and send a PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] martin-g commented on a diff in pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #2299:
URL: https://github.com/apache/avro/pull/2299#discussion_r1235078836
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -53,6 +52,40 @@ impl Decimal {
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ fn from_bigint(bigint: BigInt) -> Self {
+ let len = (bigint.bits() as f64 / 8.0).ceil() as usize;
+ Self { value: bigint, len }
+ }
+
+ // NOTE: conversion implementations below might be not that performant.
+ // It might be best to rewrite them using bits extraction and float number unpacking.
+
+ /// Converts from f32 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f32(num: f32) -> Result<Self, DecimalParsingError> {
+ if !num.is_finite() {}
Review Comment:
What is the idea here ?
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -53,6 +52,40 @@ impl Decimal {
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ fn from_bigint(bigint: BigInt) -> Self {
+ let len = (bigint.bits() as f64 / 8.0).ceil() as usize;
+ Self { value: bigint, len }
+ }
+
+ // NOTE: conversion implementations below might be not that performant.
+ // It might be best to rewrite them using bits extraction and float number unpacking.
+
+ /// Converts from f32 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f32(num: f32) -> Result<Self, DecimalParsingError> {
+ if !num.is_finite() {}
+ let string = num.to_string();
+ string.parse()
+ }
+
+ /// Converts from f64 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f64(num: f64) -> Result<Self, DecimalParsingError> {
+ let string = num.to_string();
+ string.parse()
+ }
+
+ /// Returns digits amount of the `self`.
+ pub(crate) fn digits(&self) -> u64 {
+ // Since `num_bigint` crate has such an absurd amount of the encapsulation,
Review Comment:
Language please!
There is no need to express your personal opinion all over the place! It does not help anyhow!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on a diff in pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on code in PR #2299:
URL: https://github.com/apache/avro/pull/2299#discussion_r1239712902
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -53,6 +52,40 @@ impl Decimal {
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ fn from_bigint(bigint: BigInt) -> Self {
+ let len = (bigint.bits() as f64 / 8.0).ceil() as usize;
+ Self { value: bigint, len }
+ }
+
+ // NOTE: conversion implementations below might be not that performant.
+ // It might be best to rewrite them using bits extraction and float number unpacking.
+
+ /// Converts from f32 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f32(num: f32) -> Result<Self, DecimalParsingError> {
+ if !num.is_finite() {}
+ let string = num.to_string();
+ string.parse()
+ }
+
+ /// Converts from f64 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f64(num: f64) -> Result<Self, DecimalParsingError> {
+ let string = num.to_string();
+ string.parse()
+ }
+
+ /// Returns digits amount of the `self`.
+ pub(crate) fn digits(&self) -> u64 {
+ // Since `num_bigint` crate has such an absurd amount of the encapsulation,
Review Comment:
Yea, sorry, just got a little frustrated with such a limitation ;c
fixed!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1604187696
I would be happy to hear the decision of the maintainers about Decimal's future - and start implementing that future :)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1631578835
Hello, @martin-g, please, provide an information what is the future of rust decimals and how can I participate in it
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1639647250
I think I can move decimals to bigdecimal usage - indeed, that would be an easy task, as they are bigint internally. So it's almost like drop-in replacement. What I think is that it is generally a good idea - we have bunch of other crates, like malachite for bigdecimals, but they have restrictive licenses usually. bigdecimal, on the other hand, is available and does what we trying to do by hands - like it supports parsing string/f32/f64 to bigdecimals, AND you can construct BigInt(for example, from bytes) and use it with scale to construct bigdecimal.
So, I'd happy to try!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-2416521811
Closing this PR in favor of the new BigDecimal type.
Please (re-)open the issue/PR at https://github.com/apache/avro-rs if the BigDecimal type is not good enough!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1776835368
Thank you, @cmpute !
With https://github.com/apache/avro/commit/c91b8876b1c21c93ee9021abe6d0d04cf1f50a32 we introduced usage of https://crates.io/crates/bigdecimal
There is no release for this yet, so we are open to replace it if your crate is better (performance, maintainance, nicer API, etc.)!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "cmpute (via GitHub)" <gi...@apache.org>.
cmpute commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-1776821778
FYI, the crate [`dashu`](https://crates.io/crates/dashu) created by me supports big ints (through type [`dashu::Integer`](https://docs.rs/dashu/latest/dashu/type.Integer.html)) and big decimals (through type [`dashu::Decimal`](https://docs.rs/dashu/latest/dashu/type.Decimal.html)) with arbitrary precisions. These types also have complete parsing capabilities.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [avro] ftelnov commented on a diff in pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
Posted by "ftelnov (via GitHub)" <gi...@apache.org>.
ftelnov commented on code in PR #2299:
URL: https://github.com/apache/avro/pull/2299#discussion_r1239713747
##########
lang/rust/avro/src/decimal.rs:
##########
@@ -53,6 +52,40 @@ impl Decimal {
decimal_bytes[start_byte_index..].copy_from_slice(&raw_bytes);
Ok(decimal_bytes)
}
+
+ fn from_bigint(bigint: BigInt) -> Self {
+ let len = (bigint.bits() as f64 / 8.0).ceil() as usize;
+ Self { value: bigint, len }
+ }
+
+ // NOTE: conversion implementations below might be not that performant.
+ // It might be best to rewrite them using bits extraction and float number unpacking.
+
+ /// Converts from f32 by converting number to string firstly, then parsing it.
+ pub(crate) fn try_from_f32(num: f32) -> Result<Self, DecimalParsingError> {
+ if !num.is_finite() {}
Review Comment:
Oh, wasn't completed somewhy. Fixed - it's simply a check for malformed float
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #2299:
URL: https://github.com/apache/avro/pull/2299#issuecomment-2270354980
@Gordon-F please send a PR!
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-3781: [Rust] Enhance Decimal resolve [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g closed pull request #2299: AVRO-3781: [Rust] Enhance Decimal resolve
URL: https://github.com/apache/avro/pull/2299
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org