You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/04 09:32:32 UTC
[GitHub] [arrow-rs] HaoYang670 opened a new issue, #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
HaoYang670 opened a new issue, #2001:
URL: https://github.com/apache/arrow-rs/issues/2001
**Is your feature request related to a problem or challenge? Please describe what you are trying to do.**
This is actually a question or a discussion.
Our current may to implement `Decimal128` and `Decimal256` is that:
```rust
trait BasicDecimal: ... {
const BIT_LENGTH: i32;
}
struct Decimal128 {}
struct Decimal256 {}
impl BasicDecimal for Decimal128 {}
impl BasicDecimal for Decimla256{}
impl Decimal128 {}
```
Actually I find an alternative way to implement this.
```rust
struct Decimal<const BYTE_LENGTH: usize> {
precision: usize,
scale: usize,
value: [u8; BYTE_LENGTH],
}
impl<BYTE_LENGTH> Decimal<BYTE_LENGTH> {}
type Decimal128 = Decimal<16>;
type Decimal256 = Decimal<32>;
impl Decimal128 {}
```
I am not sure which one is better, but these are some pros and cons I thought:
pros: don't need the basic trait, reducing using macro when coding (better debugging)
cons: cannot restrict the bit_length to be 128 or 258 because `const_generic_exprs` is an unstable feature.
--
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: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1173587766
cc @viirya. Looking forward to your opinions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174462413
Oh, that's `const_generic_exprs`. I don't know we can use const as generic parameter.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174489633
>didn't you already say it is unstable feature?
Oh, that's `const_generic_exprs`. I don't know we can use const as generic parameter.
Sorry for the confusion.
You are right. The`const_generic` is stable but the `const_generic_exprs` is ubstable, which means we can't add bound to the const generic so far:
(not tested)
```rust
#![feature(const_generic_exprs)]
impl<const BYTE_LENGTH> Decimal<{BYTE_LENGTH}> // const generic is stable
where {BYTE_LENGTH == 16 || BYTE_LENGTH == 32} // const generic expr is unstable
{...}
```
This is just an alternative way to implment Decimal, DecimalArray and DecimalArrayBuilder.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 closed issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 closed issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
URL: https://github.com/apache/arrow-rs/issues/2001
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1173588162
related to #1999
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174447895
> But as I cannot use 16, 32 as generic parameter, I don't go for this direction.
Why can't 16, 32 be used as generic parameter? Rust has supported `const generic`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174460835
didn't you already say it is unstable feature?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
alamb commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1175334063
I believe I remember that `Decimal64` and `Decimal32` were data types under discussion for addition to arrow, so making it easier to support additional lengths in the rust implementation would be great.
I looked at arrow2 for som inspiration but it appears to only support `Decimal` as a wrapper around `i128`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1175232892
Not fully thought through, but you could maybe do something like
```
struct Decimal128Type {}
struct Decimal256Type {}
trait DecimalType {
...
}
impl DecimalType for Decimal128Type {}
impl DecimalType for Decimal256Type {}
struct GenericDecimalArray<T: DecimalType> {}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174540208
@sunchao @alamb @tustvold Welcome to have a look and look forward to your opinions.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1178363504
Thank you for everyone's discussion! 😀
I will close this issue.
If anyone has some new ideas, we can reopen 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on issue #2001: [Discussion] Refactor the `Decimal`s by using constant generic.
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2001:
URL: https://github.com/apache/arrow-rs/issues/2001#issuecomment-1174136585
Oh, I have considered generic. But as I cannot use 16, 32 as generic parameter, I don't go for this direction.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org