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 2021/01/17 12:03:44 UTC
[GitHub] [arrow] ovr opened a new pull request #9232: ARROW-10818: [Rust] Implement DecimalType
ovr opened a new pull request #9232:
URL: https://github.com/apache/arrow/pull/9232
It's Draft, Still working on it.
Description will be completed, when I will finish with 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-795955901
Thank you @alamb @jorgecarleitao for your such detailed review. Appreciate your work! I am going to rework this PR to finish 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r561946613
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way in Rust to abstract representations of `DataType::Decimal` in Rust? Enum?
2. How should I work with `DecimalArray`? Should I make it as generic by passing representation type to it (but how should I detect type, match in everyplace? As I have known functions cannot return type) or make it generic on top of DecimalType and get byte size from it?
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r563278609
##########
File path: rust/arrow/src/compute/kernels/cast.rs
##########
@@ -443,6 +456,13 @@ pub fn cast(array: &ArrayRef, to_type: &DataType) -> Result<ArrayRef> {
))),
},
+ // start decimal casts
+ (Int8, Decimal(p, s)) => cast_numeric_to_decimal::<Int8Type>(array, *p, *s),
+ (Int16, Decimal(p, s)) => cast_numeric_to_decimal::<Int16Type>(array, *p, *s),
+ (Int32, Decimal(p, s)) => cast_numeric_to_decimal::<Int32Type>(array, *p, *s),
+ (Int64, Decimal(p, s)) => cast_numeric_to_decimal::<Int64Type>(array, *p, *s),
+ // end numeric casts
Review comment:
I think this means to me
```suggestion
// end decimal casts
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r561946613
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way in Rust to abstract representations of `DataType::Decimal` in Rust? Enum?
2. How should I work with `DecimalArray`? Should make it as generic by passing representation type to it (but how should I detect type, match in everyplace?) or make it generic on top of DecimalType and get byte size from it?
Thanks
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way in Rust to abstract representations of `DataType::Decimal` in Rust? Enum?
2. How should I work with `DecimalArray`? Should I make it as generic by passing representation type to it (but how should I detect type, match in everyplace? As I have known functions cannot return type) or make it generic on top of DecimalType and get byte size from it?
Thanks
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way to abstract representations of `DataType::Decimal in Rust`? Enum?
2. How should I work with `DecimalArray`? Should I make it as generic by passing representation type to it (but how should I detect type, match in everyplace? As I have known functions cannot return type) or make it generic on top of DecimalType and get byte size from it?
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r586437638
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
@alamb Should I store it as (i128, usize, usize) or `Decimal128Type` (which is a structure) for `ScalaraValue::Decimal128`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r590660386
##########
File path: rust/arrow/src/buffer/mutable.rs
##########
@@ -247,7 +247,7 @@ impl MutableBuffer {
/// # Safety
/// This function must only be used when this buffer was extended with items of type `T`.
/// Failure to do so results in undefined behavior.
- pub fn typed_data_mut<T: ArrowNativeType>(&mut self) -> &mut [T] {
Review comment:
If we do this, we must mark the function as unsafe.
The underlying design here is that every type representable in a buffer must be `ArrowNativeType`. This opens up the opportunity to transmute the buffer to any type, which is undefined behavior.
Instead, IMO we should write `NativeType` instead.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r586437638
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
@alamb Should I store it as (i128, usize, usize) or `Decimal128Type` (which is a structure)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-813079188
@jorgecarleitao Can you take a look at it again? Thanks. Did I understand you correctly? Now Decimal types has ::NATIVE types, which represents a native type. `DecimalArray` started to be generic and store values as native type (I didn't change builder for it, because PR is epic large). It doesn't allocate any new structure if it doesn't need it. BTW: I created a polyfill for `i256` to test requirements for tests correctly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r561946613
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way in Rust to abstract representations of `DataType::Decimal` in Rust? Enum?
2. How should I work with `DecimalArray`? Should make it as generic by passing representation type to it (but how should I detect type, match in everyplace?) or make it generic on top of DecimalType and get byte size from it?
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-761801061
https://issues.apache.org/jira/browse/ARROW-10818
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792324958
FYI I merged https://github.com/apache/arrow/pull/9653 / [ARROW-11896](https://issues.apache.org/jira/browse/ARROW-11896) for the Rust CI checks which may affect this PR. If you see "Rust / AMD64 Debian 10 Rust stable test workspace" failing with a linker error or no logs, rebasing against master will hopefully fix the problem
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792852299
> If we want to support Decimal(i128), can't we use Rust's native i128? E.g. does decimal128 + decimal128 correspond to Rust's i128 + i128, or is the math actually different?
@jorgecarleitao
It's using `i128` under the hood as expected (which generates to u64 and i64 by LLVM). As you know, `Decimal` has precision/scale and It's not possible to calculate the sum of two different decimals with different scales without rescaling, for example: `Decimal(1,1)` and `Decimal(1,5)`.
Yet another thing, it's overflowing.
`DECIMAL(1) + DECIMAL(1) = DECIMAL(1)`, example `1` + `9`.
I don't know how it should be done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r587848244
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
> your goal is to implement the Decimal128 and Decimal256 types
right. In this PR only `Decimal128 `, `Decimal256 ` will be in next PR
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
> your goal is to implement the Decimal128 and Decimal256 types
right. In this PR only `Decimal128 `, `Decimal256 ` will be in the next 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766319983
Hi @ovr , I went through what is here so far.
First of all, great stuff that you are taking this on.
Broadly speaking, this PR currently contains the following "steps":
1. make changes to the arrow crate to enable Decimal type without adding any new functionality (e.g. rename `datatypes.rs`)
2. add the native type to the arrow crate
3. add the Array type to the arrow crate
4. add support to parquet
5. add support to DataFusion
I suggest that 4-5 are done on a separate PR. 1-3 can be done on this PR, but I suggest that we split them on separate commits.
Concerning step 2, the two main checks in the list are:
1. the type's logical representation exists in the arrows' spec
2. the type's physical representation matches arrows' spec
Looking at [the spec](https://github.com/apache/arrow/blob/master/format/Schema.fbs#L186), a decimal type only supports 128 and 256 bits. So, I am do not understand why we are trying to add support for `Int32,Int64,Int128,LargeDecimal` here. Since Rust only supports `i128`, I would say that we should only go for `i128` (the same reason we do not support `f16`).
A related aspect is that `bigint`'s official documentation states
> DEPRECATED
> This crate is deprecated and will not be developed further. Users are invited to prefer the uint crate instead.
thus, I am not sure we want to take that as a dependency.
Could you clarify some of these points?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792997175
@alamb @jorgecarleitao Can you take a look and do a review? Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r593875606
##########
File path: rust/arrow/src/compute/kernels/cast.rs
##########
@@ -1511,6 +1641,65 @@ mod tests {
assert!(9.0 - c.value(4) < f64::EPSILON);
}
+ #[test]
+ fn test_cast_i64_to_decimal() {
+ let a = Int64Array::from(vec![1, 2]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
+ let c = b.as_any().downcast_ref::<Decimal128Array>().unwrap();
+
+ assert_eq!("1.0000000000", c.value(0).to_string());
+ assert_eq!("2.0000000000", c.value(1).to_string());
+ }
+
+ #[test]
+ fn test_cast_f64_to_decimal() {
+ let a = Float64Array::from(vec![1.0, 2.0]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
Review comment:
My understanding is that scale should be <= precision, such that we can't have `Decimal(5, 10)`. Is this correct, or is it me not understanding the rules?
##########
File path: rust/arrow/src/compute/kernels/cast.rs
##########
@@ -1511,6 +1641,65 @@ mod tests {
assert!(9.0 - c.value(4) < f64::EPSILON);
}
+ #[test]
+ fn test_cast_i64_to_decimal() {
+ let a = Int64Array::from(vec![1, 2]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
+ let c = b.as_any().downcast_ref::<Decimal128Array>().unwrap();
+
+ assert_eq!("1.0000000000", c.value(0).to_string());
+ assert_eq!("2.0000000000", c.value(1).to_string());
+ }
+
+ #[test]
+ fn test_cast_f64_to_decimal() {
+ let a = Float64Array::from(vec![1.0, 2.0]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
+ let c = b.as_any().downcast_ref::<Decimal128Array>().unwrap();
+
+ assert_eq!("1.0000000000", c.value(0).to_string());
+ assert_eq!("2.0000000000", c.value(1).to_string());
+ }
+
+ #[test]
+ fn test_cast_decimal_to_i64() {
+ let mut builder = DecimalBuilder::new(2, 5, 10);
+ builder
+ .append_value(Decimal128Type::new(10000000000_i128, 5, 10))
+ .unwrap();
+ builder
+ .append_value(Decimal128Type::new(20000000000_i128, 5, 10))
+ .unwrap();
+
+ let array = Arc::new(builder.finish()) as ArrayRef;
+ let b = cast(&array, &DataType::Int64).unwrap();
+ let c = b.as_any().downcast_ref::<Int64Array>().unwrap();
+
+ assert_eq!(1_i64, c.value(0));
+ assert_eq!(2_i64, c.value(1));
+ }
+
+ #[test]
+ fn test_cast_decimal_to_f64() {
+ let mut builder = DecimalBuilder::new(3, 3, 5);
+ builder
+ .append_value(Decimal128Type::new(123456_i128, 3, 5))
+ .unwrap();
+ builder
+ .append_value(Decimal128Type::new(654321_i128, 3, 5))
+ .unwrap();
+
+ let array = Arc::new(builder.finish()) as ArrayRef;
+ let b = cast(&array, &DataType::Float64).unwrap();
+ let c = b.as_any().downcast_ref::<Float64Array>().unwrap();
+
+ // string compare to skip clippy::float_cmp
Review comment:
You can also ignore the clippy lint here if it helps :)
##########
File path: rust/datafusion/src/scalar.rs
##########
@@ -210,6 +213,14 @@ impl ScalarValue {
/// Converts a scalar value into an array of `size` rows.
pub fn to_array_of_size(&self, size: usize) -> ArrayRef {
match self {
+ ScalarValue::Decimal128(e, p, s) => match e {
Review comment:
Perhaps @ovr hasn't gotten to it? I also can't think of what a limitation would be
##########
File path: rust/arrow/src/datatypes/i256_type.rs
##########
@@ -0,0 +1,297 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::alloc::NativeType;
+use crate::datatypes::{ArrowDecimalNativeType, DataType};
+use num::Zero;
+use std::cmp::Ordering;
+use std::fmt;
+use std::num::ParseIntError;
+use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Sub, SubAssign};
+use std::str::FromStr;
+
+/// Number of bits a signed 256-bit number occupies.
+pub const BITS: usize = 256;
+
+/// Number of bytes a signed 256-bit number occupies.
+pub const BYTES: usize = 32;
+
+/// An signed 256-bit number. Rust language doesnt provide i256, for now It's a polyfill.
Review comment:
```suggestion
/// An signed 256-bit number. Rust language does not provide i256, for now It is a polyfill.
```
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -132,10 +132,12 @@ impl RecordBatch {
if options.match_field_names {
for (i, column) in columns.iter().enumerate() {
if column.len() != len {
- return Err(ArrowError::InvalidArgumentError(
- "all columns in a record batch must have the same length"
- .to_string(),
- ));
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "all columns in a record batch must have the same length, expected {:?} but found {:?} at column {}",
+ len,
+ column.len(),
Review comment:
I think the order is fine, but we could use `schema.field(i).name()` instead of data type.
We're checking that all columns have the same number of rows.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-793103219
Thanks @ovr ! I have put this on my review queue -- but I will likely not be able to do so until tomorrow
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792728454
If we want to support `Decimal(i128)`, can't we use Rust's native `i128`? E.g. does `decimal128 + decimal128` correspond to Rust's `i128 + i128`, or is the math actually different?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766611408
> Arrow is used inside DF, which is used to build databases on to of it. If the user defines `DECIMAL(2,2)` it's a bit overhead to use i128 as representation for it.
@ovr, maybe there is some misunderstanding of the Arrow format and the scope of this crate.
Arrow format's goal is to address a common problem in performing analytics: interoperability of data between implementations. For example, the internal memory representation of data in spark is different from Python's numpy, which means that using some numpy function against that data is expensive because it incurs a complete serialization-deserialization roundtrip to handle spark datum.
The goal of this crate is to offer the necessary implementation in Rust to allow handling the arrow format in memory.
If we start adding extra types to the crate, we fall to the exact same problem that we were trying to solve in the first place: other implementations will need to implement a custom serialize and deserialize and therefore they will have to both implement it and incur the serialization-deserialization roundtrip cost.
This is the reason for @alamb 's comment that, if you think that arrow would benefit from other decimal types, then the right place to do it is on a broader forum that includes all other implementations (and the discussion of the spec itself).
Does this make sense?
> Memory usage, DecimalArray<Decimal128> will use 16 bytes for each element.
The argument is not about memory usage, but about memory layout and how it is consistent with a specification. Arrow specification is very specific about how memory is lay out so that it enables a [stable ABI](https://arrow.apache.org/docs/format/CDataInterface.html) to share data within the same process via foreign interfaces as well as across processes via the [flight protocol](https://arrow.apache.org/docs/format/Flight.html).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r587847557
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
I am not sure @ovr -- I have sort of lost the context of this PR in my head. When you are ready let me know and I can find time to review this again.
Just to be super clear, your goal is to implement the Decimal128 and Decimal256 types that are defined in the Arrow spec, right? Not an extension?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-822358809
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-767488331
Thank you for the clarification @jorgecarleitao -- that is a great way to summarize the rationale for my suggestions.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766338705
> Looking at the spec, a decimal type only supports 128 and 256 bits. So, I am not understanding why we are trying to add support for Int32,Int64,Int128,LargeDecimal here.
Arrow is used inside DF, which is used to build databases on to of it. If the user defines `DECIMAL(2,2)` it's a bit overhead to use i128 as representation for it.
1. Memory usage, `DecimalArray<Decimal128>` will use 16 bytes for each element.
2. Performance, Rust is using LLVM, which allows integers of any bit width from 1 to 2^23. Inside LLVM backend for x86-64, It will use rax, rcx to store it.
It's ok to store DECIMAL(2,2) as Int64 inside parquet (but anyway overhead), but it's not great to use it as memory type.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r561946613
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
@alamb @andygrove @Dandandan @nevi-me @jorgecarleitao
I created representations for Decimal as `Int32DecimalType`, `Int64DecimalType`, `Int128DecimalType` or `LargeDecimal`, which uses BigInt.
1. What is the best way to abstract representations of `DataType::Decimal in Rust`? Enum?
2. How should I work with `DecimalArray`? Should I make it as generic by passing representation type to it (but how should I detect type, match in everyplace? As I have known functions cannot return type) or make it generic on top of DecimalType and get byte size from it?
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792852299
> If we want to support Decimal(i128), can't we use Rust's native i128? E.g. does decimal128 + decimal128 correspond to Rust's i128 + i128, or is the math actually different?
@jorgecarleitao
It's using `i128` under the hood as expected (which generates to u64 and i64 by LLVM). As you know, `Decimal` has precision/scale and It's not possible to calculate the sum of two different decimals with different scales without rescaling, for example: `Decimal(1,1)` and `Decimal(1,5)`.
Yet another thing, it's overflowing.
`DECIMAL(1) + DECIMAL(1) = DECIMAL(1)`, example `1` + `9`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
nevi-me commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-808944104
Hey @ovr, do you mind separating further commits, instead of rebasing all changes into 1 commit? It makes it difficult to see what exactly has changed when you update the code.
We in any case squash everything into 1 commit. Thanks :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r562604038
##########
File path: rust/arrow/src/datatypes/mod.rs
##########
@@ -199,6 +207,81 @@ pub struct Field {
metadata: Option<BTreeMap<String, String>>,
}
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[derive(Debug)]
+pub enum DecimalType {
Review comment:
Hi @ovr -- sorry for the lack of response. I am not familiar with this code so I will need some non trivial time to review and think about your question. I hope to find some time this weekend to do so.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r586722375
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
Change it to
```
// Box is needed to reduce memory usage
Decimal128(Box<Decimal128Type>),
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766319983
Hi @ovr , I went through what is here so far.
First of all, great stuff that you are taking this on.
Broadly speaking, this PR currently contains the following "steps":
1. make changes to the arrow crate to enable Decimal type without adding any new functionality (e.g. rename `datatypes.rs`)
2. add the native type to the arrow crate
3. add the Array type to the arrow crate
4. add support to parquet
5. add support to DataFusion
I suggest that 4-5 are done on a separate PR. 1-3 can be done on this PR, but I suggest that we split them on separate commits.
Concerning step 2, the two main checks in the list are:
1. the type's logical representation exists in the arrows' spec
2. the type's physical representation matches arrows' spec
Looking at [the spec](https://github.com/apache/arrow/blob/master/format/Schema.fbs#L186), a decimal type only supports 128 and 256 bits. So, I am not understanding why we are trying to add support for `Int32,Int64,Int128,LargeDecimal` here. Since Rust only supports `i128`, I would say that we should only go for `i128` (the same reason we do not support `f16`).
EDIT: I searched for the wrong crate name ^_^
Could you clarify some of these points?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r587851545
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -22,10 +22,12 @@ use std::convert::{From, TryFrom};
use crate::error::{DataFusionError, Result};
use crate::scalar::ScalarValue;
+use arrow::datatypes::Decimal128Type;
/// Enumeration of types that can be used in a GROUP BY expression
#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub(crate) enum GroupByScalar {
+ Decimal128(i128, usize, usize),
Review comment:
Awesome -- thank you 👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792264052
BTW the test workspace check is failing on master. See more details on https://issues.apache.org/jira/browse/ARROW-11896; I don't want you to waste your time hunting down a bug that is not yours.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r590748878
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
Review comment:
perhaps this could be cleaned up?
##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -240,6 +242,48 @@ impl<'a, T: BinaryOffsetSizeTrait> std::iter::Iterator for GenericBinaryIter<'a,
}
}
+// In the feature, DecimalArray will become Generic and this iterator will use type of Decimal as T here
Review comment:
It seems to me as if the code already is generic over `T` so I am not quite sure what this comment is trying to say
##########
File path: rust/arrow/src/array/builder.rs
##########
@@ -1214,11 +1214,12 @@ impl DecimalBuilder {
}
}
- /// Appends a byte slice into the builder.
+ /// Append i128 to into the builder. For DecimalNType please use append_value.
+ /// This method is useful when you are reading a data and doesnt require to cast value to DecimalNType
///
/// Automatically calls the `append` method to delimit the slice appended in as a
/// distinct array element.
- pub fn append_value(&mut self, value: i128) -> Result<()> {
+ pub fn append_value_i128(&mut self, value: i128) -> Result<()> {
Review comment:
What about something like
```suggestion
pub fn append_value(&mut self, value: impl Into<ArrowDecimalType>) -> Result<()> {
let value: ArrowDecimalType = value.into();
```
And then as long as you implement `impl From<i128>` for Decimal128Type` it should be possible to call `append_value` with either a `Decimal128Type` or an `i128`
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
Review comment:
This seems like it will likely be quite slow (as it will convert the number to an allocated string, and then parse that string into a decimal, and then throw the string away)
Given that this is a new set of features, I think it would be ok potentially to merge this in (as it isn't a performance regression!) and optimize later, but I predict this will be needed as soon as anyone tries out the decimal type.
I bet you could do something in terms of `ArrowNumericType` instead of `T: ToString`, especially as `ArrowDecimalType::from_i32` etc exists
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
Review comment:
I feel like this is likely to `panic!` (at least in debug builds) if the lead part is greater than 128 / less than -128. Would it perhaps be better to check for overflow and return `None`?
##########
File path: rust/arrow/src/csv/reader.rs
##########
@@ -633,6 +636,45 @@ fn build_primitive_array<T: ArrowPrimitiveType + Parser>(
.map(|e| Arc::new(e) as ArrayRef)
}
+// parses a specific column (col_idx) into an Arrow Array.
+fn build_decimal_array(
+ line_number: usize,
+ rows: &[StringRecord],
+ col_idx: usize,
+ precision: usize,
+ scale: usize,
+) -> Result<ArrayRef> {
+ let mut builder = DecimalBuilder::new(rows.len(), precision, scale);
+
+ for (row_index, row) in rows.iter().enumerate() {
+ match row.get(col_idx) {
+ Some(s) => {
+ if s.is_empty() {
+ builder.append_null()?
+ }
+
+ let parsed = match Decimal128Type::parse(s, precision, scale) {
+ Ok(number) => number,
+ Err(_) => {
+ return Err(ArrowError::ParseError(format!(
+ // TODO: we should surface the underlying error here.
+ "Error while parsing value {} for column {} at line {}",
+ s,
+ col_idx,
+ line_number + row_index
Review comment:
```suggestion
"Error while parsing value {} for column {} at line {}: {}",
s,
col_idx,
line_number + row_index,
e
```
might be one way of surfacing the underlying error
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -366,6 +366,27 @@ async fn csv_query_group_by_float32() -> Result<()> {
Ok(())
}
+#[tokio::test]
+async fn csv_query_group_by_decimal() -> Result<()> {
+ let mut ctx = ExecutionContext::new();
+ register_aggregate_simple_csv(&mut ctx)?;
+
+ let sql =
+ "SELECT COUNT(*) as cnt, c4 FROM aggregate_simple GROUP BY c4 ORDER BY cnt DESC";
+ let actual = execute(&mut ctx, sql).await;
+
+ let expected = vec![
Review comment:
😮 👍
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
Review comment:
It looks like the comments could use some cleanup
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
Review comment:
I don't think this is parsing a string
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
+ }
+
+ fn to_i16(&self) -> Option<i16> {
+ Some(self.get_signed_lead_part() as i16)
+ }
+
+ fn to_i32(&self) -> Option<i32> {
+ Some(self.get_signed_lead_part() as i32)
+ }
+
+ fn to_i64(&self) -> Option<i64> {
+ Some(self.get_signed_lead_part() as i64)
+ }
+
+ fn to_i128(&self) -> Option<i128> {
+ Some(self.get_signed_lead_part())
+ }
+
+ fn to_u8(&self) -> Option<u8> {
+ Some(self.get_signed_lead_part() as u8)
+ }
+
+ fn to_u16(&self) -> Option<u16> {
+ Some(self.get_signed_lead_part() as u16)
+ }
+
+ fn to_u32(&self) -> Option<u32> {
+ Some(self.get_signed_lead_part() as u32)
+ }
+
+ fn to_u64(&self) -> Option<u64> {
+ Some(self.get_signed_lead_part() as u64)
+ }
+
+ fn to_u128(&self) -> Option<u128> {
+ Some(self.get_signed_lead_part() as u128)
+ }
+
+ fn to_f32(&self) -> Option<f32> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f32>().unwrap())
+ }
+
+ fn to_f64(&self) -> Option<f64> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f64>().unwrap())
+ }
+ }
+
+ impl ToString for $name {
+ fn to_string(&self) -> String {
+ println!("<{},{}>({})", self.digits, self.precision, self.scale);
+
+ // Skip sign, because we split string to lead, trail
+ let as_str = self.digits.abs().to_string();
+ let len = as_str.len();
+
+ let (lead, trail) =
+ (&as_str[..(len - self.scale)], &as_str[(len - self.scale)..]);
+
+ let mut result = String::new();
+
+ if self.digits < 0 {
+ result.push_str("-")
+ }
+
+ if lead == "" {
+ result.push_str(&"0");
+ } else {
+ result.push_str(&lead);
+ }
+
+ if !trail.is_empty() {
+ result.push_str(&".");
+ result.push_str(&trail);
+ }
+
+ result
+ }
+ }
+
+ impl Default for $name {
+ #[inline]
+ fn default() -> $name {
+ Zero::zero()
+ }
+ }
+
+ impl Zero for $name {
+ #[inline]
+ fn zero() -> $name {
+ $name::new(0, 1, 0)
+ }
+
+ #[inline]
+ fn is_zero(&self) -> bool {
+ self.digits == 0
+ }
+ }
+
+ impl Add<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn add(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits + rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) + rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) + self,
+ }
+ }
+ }
+
+ impl AddAssign<$name> for $name {
+ #[inline]
+ fn add_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits += rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits += rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits += rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Sub<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn sub(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits - rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) - rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) - self,
+ }
+ }
+ }
+
+ impl SubAssign<$name> for $name {
+ #[inline]
+ fn sub_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits -= rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits -= rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits -= rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Ord for $name {
+ fn cmp(&self, rhs: &Self) -> Ordering {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits.cmp(&rhs.digits),
+ Ordering::Less => {
+ self.rescale_to_new(rhs.scale).digits.cmp(&rhs.digits)
+ }
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits.cmp(&self.digits)
+ }
+ }
+ }
+ }
+
+ impl PartialOrd for $name {
+ fn partial_cmp(&self, rhs: &Self) -> Option<Ordering> {
+ Some(self.cmp(rhs))
+ }
+
+ fn lt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits < rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits < rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits < self.digits
+ }
+ }
+ }
+
+ fn le(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits <= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits <= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits <= self.digits
+ }
+ }
+ }
+
+ fn gt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits > rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits > rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits > self.digits
+ }
+ }
+ }
+
+ fn ge(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits >= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits >= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits >= self.digits
+ }
+ }
+ }
+ }
+
+ impl JsonSerializable for $name {
+ fn into_json_value(self) -> Option<Value> {
+ unimplemented!("Unimplemented JsonSerializable::into_json_value for {}", stringify!($name))
+ }
+ }
+
+ impl fmt::Debug for $name {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(
+ f,
+ "Decimal<{}, {}>(\"{}\")",
+ self.precision,
+ self.scale,
+ self.to_string()
+ )
+ }
+ }
+
+ // To fix clippy you are deriving `Hash` but have implemented `PartialEq` explicitly
+ impl Hash for $name {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.digits.hash(state);
+ self.precision.hash(state);
+ self.scale.hash(state);
+ }
+
+ fn hash_slice<H: Hasher>(_data: &[Self], _state: &mut H) where
+ Self: Sized, {
+ unimplemented!("Unimplemented hash_slice for {}", stringify!($name))
+ }
+ }
+
+ impl PartialEq<$name> for $name {
+ #[inline]
+ fn eq(&self, rhs: &$name) -> bool {
+ // @todo What is a correct behaviour for it? Rescaling?
Review comment:
this seems like an important TODO...
##########
File path: rust/datafusion/src/physical_plan/group_scalar.rs
##########
@@ -120,12 +131,9 @@ mod tests {
use crate::error::DataFusionError;
macro_rules! scalar_eq_test {
- ($TYPE:expr, $VALUE:expr) => {{
- let scalar_value = $TYPE($VALUE);
- let a = GroupByScalar::try_from(&scalar_value).unwrap();
-
- let scalar_value = $TYPE($VALUE);
- let b = GroupByScalar::try_from(&scalar_value).unwrap();
+ ($EXPR:expr) => {{
Review comment:
I don't understand this change -- it seems to now be testing that `try_from` returns the same value for the same input...
##########
File path: rust/arrow/src/record_batch.rs
##########
@@ -132,10 +132,12 @@ impl RecordBatch {
if options.match_field_names {
for (i, column) in columns.iter().enumerate() {
if column.len() != len {
- return Err(ArrowError::InvalidArgumentError(
- "all columns in a record batch must have the same length"
- .to_string(),
- ));
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "all columns in a record batch must have the same length, expected {:?} but found {:?} at column {}",
+ len,
+ column.len(),
Review comment:
I think the order of these arguments is not correct (data type should be second, right?)
##########
File path: rust/datafusion/src/scalar.rs
##########
@@ -210,6 +213,14 @@ impl ScalarValue {
/// Converts a scalar value into an array of `size` rows.
pub fn to_array_of_size(&self, size: usize) -> ArrayRef {
match self {
+ ScalarValue::Decimal128(e, p, s) => match e {
Review comment:
I don't understand this limitation
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
Review comment:
```suggestion
/// Decimal (precision, scale) = Decimal(1, 2) = 1.00
/// Note that this is a trait to support different types of Decimal implementations
/// in the future, not just those that are 128 bits.
pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
```
I think adding some comments about the rationale for this trait would also help.
I took an educated guess, but I am not sure that it is correct
##########
File path: rust/arrow/src/compute/kernels/take.rs
##########
@@ -524,6 +528,75 @@ where
Ok(BooleanArray::from(Arc::new(data)))
}
+fn take_decimal<I>(
+ values: &DecimalArray,
+ indices: &PrimitiveArray<I>,
+) -> Result<DecimalArray>
+where
+ I: ArrowNumericType,
+ I::Native: ToPrimitive,
+{
+ let data_len = indices.len();
+
+ // @todo This should be rewritten to try_from_trusted_len_iter when DecimalArray will be generic
+ let mut buffer =
+ MutableBuffer::from_len_zeroed(data_len * std::mem::size_of::<[u8; 16]>());
+ let data = buffer.typed_data_mut::<[u8; 16]>();
+
+ let nulls;
+
+ if values.null_count() == 0 {
+ // Take indices without null checking
+ for (i, elem) in data.iter_mut().enumerate() {
+ let index = ToPrimitive::to_usize(&indices.value(i)).ok_or_else(|| {
+ ArrowError::ComputeError("Cast to usize failed".to_string())
+ })?;
+
+ *elem = values.value(index).to_le_bytes();
+ }
+ nulls = indices.data_ref().null_buffer().cloned();
+ } else {
+ let num_bytes = bit_util::ceil(data_len, 8);
+ let mut null_buf = MutableBuffer::new(num_bytes).with_bitset(num_bytes, true);
+
+ let null_slice = null_buf.as_slice_mut();
+
+ for (i, elem) in data.iter_mut().enumerate() {
+ let index = ToPrimitive::to_usize(&indices.value(i)).ok_or_else(|| {
+ ArrowError::ComputeError("Cast to usize failed".to_string())
+ })?;
+
+ if values.is_null(index) {
+ bit_util::unset_bit(null_slice, i);
+ }
+
+ *elem = values.value(index).to_le_bytes();
+ }
+ nulls = match indices.data_ref().null_buffer() {
+ Some(buffer) => Some(buffer_bin_and(
+ buffer,
+ 0,
+ &null_buf.into(),
+ 0,
+ indices.len(),
+ )),
+ None => Some(null_buf.into()),
+ };
+ }
+
+ let data = ArrayData::new(
+ values.data_type().clone(),
+ indices.len(),
+ None,
+ nulls,
+ 0,
+ vec![buffer.into()],
+ vec![],
+ );
+
+ Ok(DecimalArray::from(Arc::new(data) as ArrayDataRef))
+}
+
/// `take` implementation for string arrays
Review comment:
I suggest adding some coverage for the `take` kernels with `DecimalArrays`
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
+ }
+
+ fn to_i16(&self) -> Option<i16> {
+ Some(self.get_signed_lead_part() as i16)
+ }
+
+ fn to_i32(&self) -> Option<i32> {
+ Some(self.get_signed_lead_part() as i32)
+ }
+
+ fn to_i64(&self) -> Option<i64> {
+ Some(self.get_signed_lead_part() as i64)
+ }
+
+ fn to_i128(&self) -> Option<i128> {
+ Some(self.get_signed_lead_part())
+ }
+
+ fn to_u8(&self) -> Option<u8> {
+ Some(self.get_signed_lead_part() as u8)
+ }
+
+ fn to_u16(&self) -> Option<u16> {
+ Some(self.get_signed_lead_part() as u16)
+ }
+
+ fn to_u32(&self) -> Option<u32> {
+ Some(self.get_signed_lead_part() as u32)
+ }
+
+ fn to_u64(&self) -> Option<u64> {
+ Some(self.get_signed_lead_part() as u64)
+ }
+
+ fn to_u128(&self) -> Option<u128> {
+ Some(self.get_signed_lead_part() as u128)
+ }
+
+ fn to_f32(&self) -> Option<f32> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f32>().unwrap())
+ }
+
+ fn to_f64(&self) -> Option<f64> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f64>().unwrap())
+ }
+ }
+
+ impl ToString for $name {
+ fn to_string(&self) -> String {
+ println!("<{},{}>({})", self.digits, self.precision, self.scale);
+
+ // Skip sign, because we split string to lead, trail
+ let as_str = self.digits.abs().to_string();
+ let len = as_str.len();
+
+ let (lead, trail) =
+ (&as_str[..(len - self.scale)], &as_str[(len - self.scale)..]);
+
+ let mut result = String::new();
+
+ if self.digits < 0 {
+ result.push_str("-")
+ }
+
+ if lead == "" {
+ result.push_str(&"0");
+ } else {
+ result.push_str(&lead);
+ }
+
+ if !trail.is_empty() {
+ result.push_str(&".");
+ result.push_str(&trail);
+ }
+
+ result
+ }
+ }
+
+ impl Default for $name {
+ #[inline]
+ fn default() -> $name {
+ Zero::zero()
+ }
+ }
+
+ impl Zero for $name {
+ #[inline]
+ fn zero() -> $name {
+ $name::new(0, 1, 0)
+ }
+
+ #[inline]
+ fn is_zero(&self) -> bool {
+ self.digits == 0
+ }
+ }
+
+ impl Add<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn add(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits + rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) + rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) + self,
+ }
+ }
+ }
+
+ impl AddAssign<$name> for $name {
+ #[inline]
+ fn add_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits += rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits += rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits += rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Sub<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn sub(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits - rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) - rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) - self,
+ }
+ }
+ }
+
+ impl SubAssign<$name> for $name {
+ #[inline]
+ fn sub_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits -= rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits -= rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits -= rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Ord for $name {
+ fn cmp(&self, rhs: &Self) -> Ordering {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits.cmp(&rhs.digits),
+ Ordering::Less => {
+ self.rescale_to_new(rhs.scale).digits.cmp(&rhs.digits)
+ }
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits.cmp(&self.digits)
+ }
+ }
+ }
+ }
+
+ impl PartialOrd for $name {
+ fn partial_cmp(&self, rhs: &Self) -> Option<Ordering> {
+ Some(self.cmp(rhs))
+ }
+
+ fn lt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits < rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits < rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits < self.digits
+ }
+ }
+ }
+
+ fn le(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits <= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits <= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits <= self.digits
+ }
+ }
+ }
+
+ fn gt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits > rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits > rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits > self.digits
+ }
+ }
+ }
+
+ fn ge(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits >= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits >= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits >= self.digits
+ }
+ }
+ }
+ }
+
+ impl JsonSerializable for $name {
+ fn into_json_value(self) -> Option<Value> {
+ unimplemented!("Unimplemented JsonSerializable::into_json_value for {}", stringify!($name))
+ }
+ }
+
+ impl fmt::Debug for $name {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(
+ f,
+ "Decimal<{}, {}>(\"{}\")",
+ self.precision,
+ self.scale,
+ self.to_string()
+ )
+ }
+ }
+
+ // To fix clippy you are deriving `Hash` but have implemented `PartialEq` explicitly
+ impl Hash for $name {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.digits.hash(state);
+ self.precision.hash(state);
+ self.scale.hash(state);
+ }
+
+ fn hash_slice<H: Hasher>(_data: &[Self], _state: &mut H) where
+ Self: Sized, {
+ unimplemented!("Unimplemented hash_slice for {}", stringify!($name))
+ }
+ }
+
+ impl PartialEq<$name> for $name {
+ #[inline]
+ fn eq(&self, rhs: &$name) -> bool {
+ // @todo What is a correct behaviour for it? Rescaling?
+ self.digits == rhs.digits
+ }
+ }
+
+ impl FromStr for $name {
+ type Err = ParseDecimalError;
+
+ fn from_str(s: &str) -> Result<Self, ParseDecimalError> {
+ let (digits, precision, scale) = match s.find('.') {
+ // Decimal with empty scale
+ None => {
+ let digits = s.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (s.parse::<$native_ty>()?, s.len() - 1, 0)
+ } else {
+ (s.parse::<$native_ty>()?, s.len(), 0)
+ }
+ }
+ Some(loc) => {
+ let (lead, trail) = (&s[..loc], &s[loc + 1..]);
+
+ // Concat both parts to make bigint from int
+ let mut parts = String::from(lead);
+ parts.push_str(trail);
+
+ let digits = parts.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (digits, lead.len() - 1, trail.len())
+ } else {
+ (digits, lead.len(), trail.len())
+ }
+ }
+ };
+
+ Ok($name::new(digits, precision, scale))
+ }
+ }
+ };
+}
+
+// This types are disabled, because Arrow doesnt declare Decimals for 32 / 64
+// i32 max - 2_147_483_647i32
+// make_type!(Decimal32Type, i32, 9);
+// i64 max - 9_223_372_036_854_775_807i64
+//make_type!(Decimal64Type, i64, 18);
+
+// i128 max - 170_141_183_460_469_231_731_687_303_715_884_105_727i128
+make_type!(Decimal128Type, i128, 38);
+
+impl From<Decimal128Type> for i128 {
+ fn from(d: Decimal128Type) -> Self {
+ d.digits
+ }
+}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ #[test]
+ fn decimal_test_from_and_to_str() {
+ let values = vec![
+ // None
+ ("0", Decimal128Type::new(0_i128, 1, 0)),
+ // Positive
+ ("1.0", Decimal128Type::new(10_i128, 1, 1)),
+ ("1.001", Decimal128Type::new(1001_i128, 1, 3)),
+ ("1", Decimal128Type::new(1_i128, 1, 0)),
+ ("0.98", Decimal128Type::new(98_i128, 1, 2)),
+ // Negative
+ ("-1.0", Decimal128Type::new(-10_i128, 1, 1)),
+ ("-1.001", Decimal128Type::new(-1001_i128, 1, 3)),
+ ("-1", Decimal128Type::new(-1_i128, 1, 0)),
+ //
+ (
+ "1.000000000001",
+ Decimal128Type::new(1_000_000_000_001_i128, 1, 12),
+ ),
+ (
+ "5.000000000005",
+ Decimal128Type::new(5_000_000_000_005_i128, 1, 12),
+ ),
+ ];
+
+ for (source, expected) in values {
+ let actual = Decimal128Type::from_str(source).unwrap();
+ assert_eq!(actual, expected);
+ assert_eq!(actual.to_string(), source);
+ }
+ }
+
+ #[test]
+ fn decimal_test_partial_eq() {
+ let values = vec![
+ // Eq scale
+ (
+ Decimal128Type::new(0_i128, 1, 0),
+ Decimal128Type::new(0_i128, 1, 0),
+ true,
+ ),
+ (
+ Decimal128Type::new(0_i128, 1, 0),
+ Decimal128Type::new(1_i128, 1, 0),
+ false,
+ ),
+ // Scaling is disabled in PartialEq, probably we will use it, but I dont know for now
+ // What is a correct behaviour
+ // > scale
+ // (
+ // Decimal128Type::new(10_i128, 1, 1),
+ // Decimal128Type::new(1_i128, 1, 0),
+ // true,
+ // ),
+ // (
+ // Decimal128Type::new(20_i128, 1, 1),
+ // Decimal128Type::new(1_i128, 1, 0),
+ // false,
+ // ),
+ // // < scale
+ // (
+ // Decimal128Type::new(1_i128, 1, 0),
+ // Decimal128Type::new(10_i128, 1, 1),
+ // true,
+ // ),
+ // (
+ // Decimal128Type::new(1_i128, 1, 0),
+ // Decimal128Type::new(20_i128, 1, 1),
+ // false,
+ // ),
+ ];
+
+ for (left, right, expected) in values {
+ assert_eq!(
+ left == right,
+ expected,
+ "{} == {}, expected {}",
+ left.to_string(),
+ right.to_string(),
+ expected
+ );
+ }
+ }
+
+ #[test]
+ fn decimal_test_add() {
+ let values = vec![
+ // without rescaling
+ (
+ Decimal128Type::new(0, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ ),
+ (
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(10, 2, 0),
+ ),
+ // with rescaling left
+ (
+ Decimal128Type::new(1, 1, 0),
+ Decimal128Type::new(101, 1, 2),
+ Decimal128Type::new(201, 1, 2),
+ ),
+ // with rescaling right
+ (
+ Decimal128Type::new(101, 1, 2),
+ Decimal128Type::new(1, 1, 0),
+ Decimal128Type::new(201, 1, 2),
+ ),
+ ];
+
+ for (left, right, result) in values {
+ assert_eq!(
+ left + right,
+ result,
+ "{} + {} = {}",
+ left.to_string(),
+ right.to_string(),
+ result.to_string()
+ );
+ }
+ }
+
+ #[test]
+ fn decimal_test_sub() {
+ let values = vec![
+ // without rescaling
+ (
+ Decimal128Type::new(10, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ ),
+ (
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(5, 2, 0),
+ Decimal128Type::new(0, 2, 0),
+ ),
+ // with rescaling left
+ (
+ Decimal128Type::new(2, 1, 0),
+ Decimal128Type::new(101, 1, 2),
+ Decimal128Type::new(99, 1, 2),
+ ),
+ ];
+
+ for (left, right, result) in values {
+ assert_eq!(
+ left - right,
+ result,
+ "{} - {} = {}",
+ left.to_string(),
+ right.to_string(),
+ result.to_string()
+ );
+ }
+ }
+
+ #[test]
+ fn decimal_test_cmp() {
+ let values = vec![
+ (
+ Decimal128Type::new(0_i128, 1, 0),
+ Decimal128Type::new(0_i128, 1, 0),
+ Ordering::Equal,
+ ),
+ (
+ Decimal128Type::new(1_i128, 1, 0),
+ Decimal128Type::new(0_i128, 1, 0),
+ Ordering::Greater,
+ ),
+ (
+ Decimal128Type::new(0_i128, 1, 0),
+ Decimal128Type::new(1_i128, 1, 0),
+ Ordering::Less,
+ ),
+ ];
+
+ for (left, right, expected) in values {
+ assert_eq!(left.cmp(&right), expected);
+ }
+ }
+
+ #[test]
+ fn decimal_test_cmp_lt() {
+ let values = vec![
+ (
+ Decimal128Type::new(0_i128, 1, 0),
+ Decimal128Type::new(1_i128, 1, 0),
+ true,
+ ),
+ (
+ Decimal128Type::new(1_i128, 1, 0),
+ Decimal128Type::new(1_i128, 1, 0),
+ false,
+ ),
+ ];
+
+ for (left, right, expected) in values {
+ assert_eq!(left < right, expected);
+ }
+ }
+
+ #[test]
Review comment:
tests for `==` (aka `PartialEq`) are probably important
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
+ }
+
+ fn to_i16(&self) -> Option<i16> {
+ Some(self.get_signed_lead_part() as i16)
+ }
+
+ fn to_i32(&self) -> Option<i32> {
+ Some(self.get_signed_lead_part() as i32)
+ }
+
+ fn to_i64(&self) -> Option<i64> {
+ Some(self.get_signed_lead_part() as i64)
+ }
+
+ fn to_i128(&self) -> Option<i128> {
+ Some(self.get_signed_lead_part())
+ }
+
+ fn to_u8(&self) -> Option<u8> {
+ Some(self.get_signed_lead_part() as u8)
+ }
+
+ fn to_u16(&self) -> Option<u16> {
+ Some(self.get_signed_lead_part() as u16)
+ }
+
+ fn to_u32(&self) -> Option<u32> {
+ Some(self.get_signed_lead_part() as u32)
+ }
+
+ fn to_u64(&self) -> Option<u64> {
+ Some(self.get_signed_lead_part() as u64)
+ }
+
+ fn to_u128(&self) -> Option<u128> {
+ Some(self.get_signed_lead_part() as u128)
+ }
+
+ fn to_f32(&self) -> Option<f32> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f32>().unwrap())
+ }
+
+ fn to_f64(&self) -> Option<f64> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f64>().unwrap())
+ }
+ }
+
+ impl ToString for $name {
+ fn to_string(&self) -> String {
+ println!("<{},{}>({})", self.digits, self.precision, self.scale);
+
+ // Skip sign, because we split string to lead, trail
+ let as_str = self.digits.abs().to_string();
+ let len = as_str.len();
+
+ let (lead, trail) =
+ (&as_str[..(len - self.scale)], &as_str[(len - self.scale)..]);
+
+ let mut result = String::new();
+
+ if self.digits < 0 {
+ result.push_str("-")
+ }
+
+ if lead == "" {
+ result.push_str(&"0");
+ } else {
+ result.push_str(&lead);
+ }
+
+ if !trail.is_empty() {
+ result.push_str(&".");
+ result.push_str(&trail);
+ }
+
+ result
+ }
+ }
+
+ impl Default for $name {
+ #[inline]
+ fn default() -> $name {
+ Zero::zero()
+ }
+ }
+
+ impl Zero for $name {
+ #[inline]
+ fn zero() -> $name {
+ $name::new(0, 1, 0)
+ }
+
+ #[inline]
+ fn is_zero(&self) -> bool {
+ self.digits == 0
+ }
+ }
+
+ impl Add<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn add(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits + rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) + rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) + self,
+ }
+ }
+ }
+
+ impl AddAssign<$name> for $name {
+ #[inline]
+ fn add_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits += rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits += rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits += rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Sub<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn sub(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits - rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) - rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) - self,
+ }
+ }
+ }
+
+ impl SubAssign<$name> for $name {
+ #[inline]
+ fn sub_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits -= rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits -= rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits -= rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Ord for $name {
+ fn cmp(&self, rhs: &Self) -> Ordering {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits.cmp(&rhs.digits),
+ Ordering::Less => {
+ self.rescale_to_new(rhs.scale).digits.cmp(&rhs.digits)
+ }
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits.cmp(&self.digits)
+ }
+ }
+ }
+ }
+
+ impl PartialOrd for $name {
+ fn partial_cmp(&self, rhs: &Self) -> Option<Ordering> {
+ Some(self.cmp(rhs))
+ }
+
+ fn lt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits < rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits < rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits < self.digits
+ }
+ }
+ }
+
+ fn le(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits <= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits <= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits <= self.digits
+ }
+ }
+ }
+
+ fn gt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits > rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits > rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits > self.digits
+ }
+ }
+ }
+
+ fn ge(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits >= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits >= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits >= self.digits
+ }
+ }
+ }
+ }
+
+ impl JsonSerializable for $name {
+ fn into_json_value(self) -> Option<Value> {
+ unimplemented!("Unimplemented JsonSerializable::into_json_value for {}", stringify!($name))
+ }
+ }
+
+ impl fmt::Debug for $name {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(
+ f,
+ "Decimal<{}, {}>(\"{}\")",
+ self.precision,
+ self.scale,
+ self.to_string()
+ )
+ }
+ }
+
+ // To fix clippy you are deriving `Hash` but have implemented `PartialEq` explicitly
+ impl Hash for $name {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.digits.hash(state);
+ self.precision.hash(state);
+ self.scale.hash(state);
+ }
+
+ fn hash_slice<H: Hasher>(_data: &[Self], _state: &mut H) where
+ Self: Sized, {
+ unimplemented!("Unimplemented hash_slice for {}", stringify!($name))
+ }
+ }
+
+ impl PartialEq<$name> for $name {
+ #[inline]
+ fn eq(&self, rhs: &$name) -> bool {
+ // @todo What is a correct behaviour for it? Rescaling?
+ self.digits == rhs.digits
+ }
+ }
+
+ impl FromStr for $name {
+ type Err = ParseDecimalError;
+
+ fn from_str(s: &str) -> Result<Self, ParseDecimalError> {
+ let (digits, precision, scale) = match s.find('.') {
+ // Decimal with empty scale
+ None => {
+ let digits = s.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (s.parse::<$native_ty>()?, s.len() - 1, 0)
+ } else {
+ (s.parse::<$native_ty>()?, s.len(), 0)
+ }
+ }
+ Some(loc) => {
+ let (lead, trail) = (&s[..loc], &s[loc + 1..]);
+
+ // Concat both parts to make bigint from int
+ let mut parts = String::from(lead);
+ parts.push_str(trail);
+
+ let digits = parts.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (digits, lead.len() - 1, trail.len())
+ } else {
+ (digits, lead.len(), trail.len())
+ }
+ }
+ };
+
+ Ok($name::new(digits, precision, scale))
+ }
+ }
+ };
+}
+
+// This types are disabled, because Arrow doesnt declare Decimals for 32 / 64
+// i32 max - 2_147_483_647i32
+// make_type!(Decimal32Type, i32, 9);
+// i64 max - 9_223_372_036_854_775_807i64
+//make_type!(Decimal64Type, i64, 18);
+
+// i128 max - 170_141_183_460_469_231_731_687_303_715_884_105_727i128
+make_type!(Decimal128Type, i128, 38);
Review comment:
```suggestion
/// Type which stores a decimal number using 128 bits
make_type!(Decimal128Type, i128, 38);
```
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
+ }
+
+ fn to_i16(&self) -> Option<i16> {
+ Some(self.get_signed_lead_part() as i16)
+ }
+
+ fn to_i32(&self) -> Option<i32> {
+ Some(self.get_signed_lead_part() as i32)
+ }
+
+ fn to_i64(&self) -> Option<i64> {
+ Some(self.get_signed_lead_part() as i64)
+ }
+
+ fn to_i128(&self) -> Option<i128> {
+ Some(self.get_signed_lead_part())
+ }
+
+ fn to_u8(&self) -> Option<u8> {
+ Some(self.get_signed_lead_part() as u8)
+ }
+
+ fn to_u16(&self) -> Option<u16> {
+ Some(self.get_signed_lead_part() as u16)
+ }
+
+ fn to_u32(&self) -> Option<u32> {
+ Some(self.get_signed_lead_part() as u32)
+ }
+
+ fn to_u64(&self) -> Option<u64> {
+ Some(self.get_signed_lead_part() as u64)
+ }
+
+ fn to_u128(&self) -> Option<u128> {
+ Some(self.get_signed_lead_part() as u128)
+ }
+
+ fn to_f32(&self) -> Option<f32> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f32>().unwrap())
+ }
+
+ fn to_f64(&self) -> Option<f64> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f64>().unwrap())
+ }
+ }
+
+ impl ToString for $name {
+ fn to_string(&self) -> String {
+ println!("<{},{}>({})", self.digits, self.precision, self.scale);
+
+ // Skip sign, because we split string to lead, trail
+ let as_str = self.digits.abs().to_string();
+ let len = as_str.len();
+
+ let (lead, trail) =
+ (&as_str[..(len - self.scale)], &as_str[(len - self.scale)..]);
+
+ let mut result = String::new();
+
+ if self.digits < 0 {
+ result.push_str("-")
+ }
+
+ if lead == "" {
+ result.push_str(&"0");
+ } else {
+ result.push_str(&lead);
+ }
+
+ if !trail.is_empty() {
+ result.push_str(&".");
+ result.push_str(&trail);
+ }
+
+ result
+ }
+ }
+
+ impl Default for $name {
+ #[inline]
+ fn default() -> $name {
+ Zero::zero()
+ }
+ }
+
+ impl Zero for $name {
+ #[inline]
+ fn zero() -> $name {
+ $name::new(0, 1, 0)
+ }
+
+ #[inline]
+ fn is_zero(&self) -> bool {
+ self.digits == 0
+ }
+ }
+
+ impl Add<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn add(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits + rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) + rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) + self,
+ }
+ }
+ }
+
+ impl AddAssign<$name> for $name {
+ #[inline]
+ fn add_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits += rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits += rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits += rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Sub<$name> for $name {
+ type Output = $name;
+
+ #[inline]
+ fn sub(self, rhs: $name) -> $name {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ $name::new(self.digits - rhs.digits, self.precision, self.scale)
+ }
+ Ordering::Less => self.rescale_to_new(rhs.scale) - rhs,
+ Ordering::Greater => rhs.rescale_to_new(self.scale) - self,
+ }
+ }
+ }
+
+ impl SubAssign<$name> for $name {
+ #[inline]
+ fn sub_assign(&mut self, rhs: $name) {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => {
+ self.digits -= rhs.digits;
+ }
+ Ordering::Less => {
+ self.rescale(rhs.scale);
+ self.digits -= rhs.digits;
+ }
+ Ordering::Greater => {
+ self.rescale(self.scale);
+ self.digits -= rhs.digits;
+ }
+ }
+ }
+ }
+
+ impl Ord for $name {
+ fn cmp(&self, rhs: &Self) -> Ordering {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits.cmp(&rhs.digits),
+ Ordering::Less => {
+ self.rescale_to_new(rhs.scale).digits.cmp(&rhs.digits)
+ }
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits.cmp(&self.digits)
+ }
+ }
+ }
+ }
+
+ impl PartialOrd for $name {
+ fn partial_cmp(&self, rhs: &Self) -> Option<Ordering> {
+ Some(self.cmp(rhs))
+ }
+
+ fn lt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits < rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits < rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits < self.digits
+ }
+ }
+ }
+
+ fn le(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits <= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits <= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits <= self.digits
+ }
+ }
+ }
+
+ fn gt(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits > rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits > rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits > self.digits
+ }
+ }
+ }
+
+ fn ge(&self, rhs: &Self) -> bool {
+ match self.scale.cmp(&rhs.scale) {
+ Ordering::Equal => self.digits >= rhs.digits,
+ Ordering::Less => self.rescale_to_new(rhs.scale).digits >= rhs.digits,
+ Ordering::Greater => {
+ rhs.rescale_to_new(self.scale).digits >= self.digits
+ }
+ }
+ }
+ }
+
+ impl JsonSerializable for $name {
+ fn into_json_value(self) -> Option<Value> {
+ unimplemented!("Unimplemented JsonSerializable::into_json_value for {}", stringify!($name))
+ }
+ }
+
+ impl fmt::Debug for $name {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ write!(
+ f,
+ "Decimal<{}, {}>(\"{}\")",
+ self.precision,
+ self.scale,
+ self.to_string()
+ )
+ }
+ }
+
+ // To fix clippy you are deriving `Hash` but have implemented `PartialEq` explicitly
+ impl Hash for $name {
+ fn hash<H: Hasher>(&self, state: &mut H) {
+ self.digits.hash(state);
+ self.precision.hash(state);
+ self.scale.hash(state);
+ }
+
+ fn hash_slice<H: Hasher>(_data: &[Self], _state: &mut H) where
+ Self: Sized, {
+ unimplemented!("Unimplemented hash_slice for {}", stringify!($name))
+ }
+ }
+
+ impl PartialEq<$name> for $name {
+ #[inline]
+ fn eq(&self, rhs: &$name) -> bool {
+ // @todo What is a correct behaviour for it? Rescaling?
+ self.digits == rhs.digits
+ }
+ }
+
+ impl FromStr for $name {
+ type Err = ParseDecimalError;
+
+ fn from_str(s: &str) -> Result<Self, ParseDecimalError> {
+ let (digits, precision, scale) = match s.find('.') {
+ // Decimal with empty scale
+ None => {
+ let digits = s.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (s.parse::<$native_ty>()?, s.len() - 1, 0)
+ } else {
+ (s.parse::<$native_ty>()?, s.len(), 0)
+ }
+ }
+ Some(loc) => {
+ let (lead, trail) = (&s[..loc], &s[loc + 1..]);
+
+ // Concat both parts to make bigint from int
+ let mut parts = String::from(lead);
+ parts.push_str(trail);
+
+ let digits = parts.parse::<$native_ty>()?;
+
+ if digits < 0 {
+ (digits, lead.len() - 1, trail.len())
+ } else {
+ (digits, lead.len(), trail.len())
+ }
+ }
+ };
+
+ Ok($name::new(digits, precision, scale))
+ }
+ }
+ };
+}
+
+// This types are disabled, because Arrow doesnt declare Decimals for 32 / 64
+// i32 max - 2_147_483_647i32
+// make_type!(Decimal32Type, i32, 9);
+// i64 max - 9_223_372_036_854_775_807i64
+//make_type!(Decimal64Type, i64, 18);
+
+// i128 max - 170_141_183_460_469_231_731_687_303_715_884_105_727i128
+make_type!(Decimal128Type, i128, 38);
Review comment:
👍 for comments explaining why this macro is as it is
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal (Decimal128Type only)
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r590660386
##########
File path: rust/arrow/src/buffer/mutable.rs
##########
@@ -247,7 +247,7 @@ impl MutableBuffer {
/// # Safety
/// This function must only be used when this buffer was extended with items of type `T`.
/// Failure to do so results in undefined behavior.
- pub fn typed_data_mut<T: ArrowNativeType>(&mut self) -> &mut [T] {
Review comment:
If we do this, we must mark the function as unsafe.
The underlying design here is that every type representable in a buffer must be `ArrowNativeType`. This opens up the opportunity to transmute the buffer to any type, which is undefined behavior.
I do not think we should do this because it violates the design of this crate whereby every type that can be written into a Buffer implements `ArrowNativeType`.
##########
File path: rust/arrow/src/datatypes/decimal.rs
##########
@@ -0,0 +1,926 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use crate::datatypes::JsonSerializable;
+use num::{NumCast, ToPrimitive, Zero};
+use serde_json::Value;
+use std::hash::{Hash, Hasher};
+use std::{
+ cmp::Ordering,
+ convert::TryInto,
+ fmt,
+ mem::size_of,
+ num::ParseIntError,
+ ops::{Add, AddAssign, Sub, SubAssign},
+ str::FromStr,
+};
+
+#[derive(Debug, PartialEq)]
+pub enum ParseDecimalError {
+ ParseIntError(ParseIntError),
+ Other(String),
+}
+
+impl From<ParseIntError> for ParseDecimalError {
+ fn from(err: ParseIntError) -> ParseDecimalError {
+ ParseDecimalError::ParseIntError(err)
+ }
+}
+
+// Decimal (precision, scale) = Decimal(1, 2) = 1.00
+pub trait ArrowDecimalType: fmt::Debug + Send + Sync + FromStr + PartialEq {
+ const MAX_DIGITS: usize;
+
+ // fn into_json_value(self) -> Option<Value>;
+
+ fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> usize;
+
+ // Rescale scale part
+ fn rescale(&mut self, scale: usize);
+
+ fn rescale_to_new(self, scale: usize) -> Self;
+
+ // Try to parse string with precision, scale
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ // Try to parse string with precision, scale
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> std::result::Result<Self, ParseDecimalError>;
+
+ fn to_le_bytes(&self) -> [u8; 16];
+
+ fn to_be_bytes(&self) -> [u8; 16];
+
+ fn to_byte_slice(&self) -> Vec<u8>;
+
+ fn get_signed_lead_part(&self) -> i128;
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> Self;
+}
+
+#[inline]
+pub fn numeric_to_decimal<T: ToString, U: ArrowDecimalType>(
+ n: T,
+ p: usize,
+ s: usize,
+) -> Option<U> {
+ Some(U::parse(n.to_string().as_str(), p, s).unwrap_or_else(|_e| {
+ panic!("unable to represent");
+ }))
+}
+
+#[inline]
+pub fn decimal_to_numeric<U: DecimalCast, T: NumCast>(n: U) -> Option<T> {
+ T::from(n)
+}
+
+pub trait DecimalCast: Sized + ArrowDecimalType + ToPrimitive {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self>;
+}
+
+impl DecimalCast for Decimal128Type {
+ fn from<T: ToPrimitive>(n: T, p: usize, s: usize) -> Option<Self> {
+ Some(Decimal128Type::from_f64(n.to_f64().unwrap(), p, s).unwrap())
+ }
+}
+
+#[inline(always)]
+fn pow_ten(pow: usize) -> u64 {
+ 10u64.pow(pow as u32)
+}
+
+macro_rules! make_type {
+ ($name:ident, $native_ty:ty, $max_digits:expr) => {
+ #[derive(Copy, Clone, Eq)]
+ pub struct $name {
+ pub digits: $native_ty,
+ pub precision: usize,
+ pub scale: usize,
+ }
+
+ impl $name {
+ pub fn new(digits: $native_ty, precision: usize, scale: usize) -> $name {
+ assert!(
+ (precision + scale) <= $max_digits,
+ "Unable to use {} to represent Decimal({}, {}), max digits reached ({}).",
+ stringify!($name),
+ precision,
+ scale,
+ stringify!($max_digits),
+ );
+
+ $name {
+ digits,
+ precision,
+ scale,
+ }
+ }
+ }
+
+ impl ArrowDecimalType for $name {
+ const MAX_DIGITS: usize = $max_digits;
+
+ /// Returns the byte width of this primitive type.
+ fn get_byte_width_for_precision_scale(
+ _precision: usize,
+ _scale: usize,
+ ) -> usize {
+ size_of::<$native_ty>()
+ }
+
+ #[inline(always)]
+ fn rescale(&mut self, scale: usize) {
+ if self.digits.is_zero() {
+ self.scale = scale;
+ } else {
+ match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits /= pow_ten(self.scale - scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Less => {
+ self.digits *= pow_ten(scale - self.scale) as $native_ty;
+ self.scale = scale;
+ }
+ Ordering::Equal => {}
+ };
+ }
+ }
+
+ #[inline(always)]
+ fn get_signed_lead_part(&self) -> i128 {
+ self.rescale_to_new(0).digits
+ }
+
+ #[inline(always)]
+ fn rescale_to_new(self, scale: usize) -> $name {
+ if self.digits.is_zero() {
+ return $name::new(0, 0, scale);
+ }
+
+ let digits = match self.scale.cmp(&scale) {
+ Ordering::Greater => {
+ self.digits / pow_ten(self.scale - scale) as $native_ty
+ }
+ Ordering::Less => {
+ self.digits * pow_ten(scale - self.scale) as $native_ty
+ }
+ Ordering::Equal => self.digits,
+ };
+
+ $name::new(digits, self.precision, scale)
+ }
+
+ fn from_i128(
+ n: i128,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_i64(
+ n: i64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::new(n as $native_ty, precision, 0);
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn from_f64(
+ n: f64,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ $name::parse(n.to_string().as_str(), precision, scale)
+ }
+
+ fn parse(
+ string: &str,
+ precision: usize,
+ scale: usize,
+ ) -> Result<Self, ParseDecimalError> {
+ let mut as_decimal = $name::from_str(string)?;
+
+ if as_decimal.scale != scale {
+ as_decimal.rescale(scale)
+ }
+
+ as_decimal.precision = precision;
+
+ Ok(as_decimal)
+ }
+
+ fn to_le_bytes(&self) -> [u8; 16] {
+ self.digits.to_le_bytes()
+ }
+
+ fn to_be_bytes(&self) -> [u8; 16] {
+ self.digits.to_be_bytes()
+ }
+
+ fn to_byte_slice(&self) -> Vec<u8> {
+ self.digits.to_le_bytes().to_vec()
+ }
+
+ fn from_bytes_with_precision_scale(
+ bytes: &[u8],
+ precision: usize,
+ scale: usize,
+ ) -> $name {
+ let as_array = bytes.try_into();
+ match as_array {
+ Ok(v) if bytes.len() == 16 => $name {
+ digits: <$native_ty>::from_le_bytes(v),
+ precision,
+ scale,
+ },
+ Err(e) => panic!(
+ "Unable to load Decimal from bytes slice ({}): {}",
+ bytes.len(),
+ e
+ ),
+ _ => panic!(
+ "Unable to load Decimal from bytes slice with length {}",
+ bytes.len()
+ ),
+ }
+ }
+ }
+
+ impl ToPrimitive for $name {
+ fn to_isize(&self) -> Option<isize> {
+ unimplemented!("Unimplemented to_isize for {}", stringify!($name))
+ }
+
+ fn to_usize(&self) -> Option<usize> {
+ unimplemented!("Unimplemented to_usize for {}", stringify!($name))
+ }
+
+ fn to_i8(&self) -> Option<i8> {
+ Some(self.get_signed_lead_part() as i8)
+ }
+
+ fn to_i16(&self) -> Option<i16> {
+ Some(self.get_signed_lead_part() as i16)
+ }
+
+ fn to_i32(&self) -> Option<i32> {
+ Some(self.get_signed_lead_part() as i32)
+ }
+
+ fn to_i64(&self) -> Option<i64> {
+ Some(self.get_signed_lead_part() as i64)
+ }
+
+ fn to_i128(&self) -> Option<i128> {
+ Some(self.get_signed_lead_part())
+ }
+
+ fn to_u8(&self) -> Option<u8> {
+ Some(self.get_signed_lead_part() as u8)
+ }
+
+ fn to_u16(&self) -> Option<u16> {
+ Some(self.get_signed_lead_part() as u16)
+ }
+
+ fn to_u32(&self) -> Option<u32> {
+ Some(self.get_signed_lead_part() as u32)
+ }
+
+ fn to_u64(&self) -> Option<u64> {
+ Some(self.get_signed_lead_part() as u64)
+ }
+
+ fn to_u128(&self) -> Option<u128> {
+ Some(self.get_signed_lead_part() as u128)
+ }
+
+ fn to_f32(&self) -> Option<f32> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f32>().unwrap())
+ }
+
+ fn to_f64(&self) -> Option<f64> {
+ // @todo Optimize this
+ Some(self.to_string().parse::<f64>().unwrap())
+ }
+ }
+
+ impl ToString for $name {
+ fn to_string(&self) -> String {
+ println!("<{},{}>({})", self.digits, self.precision, self.scale);
Review comment:
print.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb closed pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9232:
URL: https://github.com/apache/arrow/pull/9232
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766319983
Hi @ovr , I went through what is here so far.
First of all, great stuff that you are taking this on.
Broadly speaking, this PR currently contains the following "steps":
1. make changes to the arrow crate to enable Decimal type without adding any new functionality (e.g. rename `datatypes.rs`)
2. add the native type to the arrow crate
3. add the Array type to the arrow crate
4. add support to parquet
5. add support to DataFusion
I suggest that 4-5 are done on a separate PR. 1-3 can be done on this PR, but I suggest that we split them on separate commits.
Concerning step 2, the two main checks in the list are:
1. the type's logical representation exists in the arrows' spec
2. the type's physical representation matches arrows' spec
Looking at [the spec](https://github.com/apache/arrow/blob/master/format/Schema.fbs#L186), a decimal type only supports 128 and 256 bits. So, I am do not understand why we are trying to add support for `Int32,Int64,Int128,LargeDecimal` here. Since Rust only supports `i128`, I would say that we should only go for `i128` (the same reason we do not support `f16`).
EDIT: I searched for the wrong crate name ^_^
Could you clarify some of these points?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-831242849
https://github.com/apache/arrow/pull/10096 has removed the arrow implementation from this repository (it now resides in https://github.com/apache/arrow-rs and https://github.com/apache/arrow-datafusion) in the hopes of streamlining the development process
Please re-target this PR (let us know if you need help doing so) to one/both of the new repositories.
Thank you for understanding and helping to make arrow-rs and datafusion better
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-789647138
@ovr what do you think we should do with this PR? Is it worth keeping open as a Draft or shall we close it for now?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792852299
> If we want to support Decimal(i128), can't we use Rust's native i128? E.g. does decimal128 + decimal128 correspond to Rust's i128 + i128, or is the math actually different?
@jorgecarleitao
It's using `i128` under the hood as expected (which generates to u64 and i64 by LLVM). As you know, `Decimal` has precision/scale and It's not possible to calculate the sum of two different decimals with different scales without rescaling, for example: `Decimal(1,1)` and `Decimal(1,5)`.
Yet another thing, it's overflowing.
`DECIMAL(1) + DECIMAL(1) = DECIMAL(1)`, example `1` + `9`.
I don't know how it should be done.
Related to `Decimal256`, probably it will use (but it will be done in another PR)
`[i64, u64, u64, u64]` or `[i128, u128]`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r606837080
##########
File path: rust/arrow/src/compute/kernels/cast.rs
##########
@@ -1511,6 +1641,65 @@ mod tests {
assert!(9.0 - c.value(4) < f64::EPSILON);
}
+ #[test]
+ fn test_cast_i64_to_decimal() {
+ let a = Int64Array::from(vec![1, 2]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
+ let c = b.as_any().downcast_ref::<Decimal128Array>().unwrap();
+
+ assert_eq!("1.0000000000", c.value(0).to_string());
+ assert_eq!("2.0000000000", c.value(1).to_string());
+ }
+
+ #[test]
+ fn test_cast_f64_to_decimal() {
+ let a = Float64Array::from(vec![1.0, 2.0]);
+ let array = Arc::new(a) as ArrayRef;
+ let b = cast(&array, &DataType::Decimal(5, 10)).unwrap();
Review comment:
Not, it can be. I don't see any limitations.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] jorgecarleitao commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766611408
> Arrow is used inside DF, which is used to build databases on to of it. If the user defines `DECIMAL(2,2)` it's a bit overhead to use i128 as representation for it.
@ovr, maybe there is some misunderstanding of the Arrow format and the scope of this crate.
Arrow format's goal is to address a common problem in performing analytics: interoperability of data between implementations. For example, the internal memory representation of data in spark is different from Python's numpy, which means that using some numpy function against that data is expensive because it incurs a complete serialization-deserialization roundtrip to handle spark datum.
The goal of this crate is to offer the necessary implementation in Rust to allow handling the arrow format in memory.
If we start adding extra types to the crate, we fall to the exact same problem that we were trying to solve in the first place: other implementations will need to implement a custom serialize and deserialize and therefore they will have to both implement it and incur the serialization-deserialization roundtrip cost.
This is the reason for @alamb 's comment that, if you think that arrow would benefit from other decimal types, then the right place to do it is on a broader forum that includes all other implementations (and the discussion of the spec itself).
Does this make sense?
> Memory usage, DecimalArray<Decimal128> will use 16 bytes for each element.
The argument is not about memory usage, but about memory layout and how it is consistent with a specification. Arrow specification is very specific about how memory is lay out so that it enables a [stable ABI](https://arrow.apache.org/docs/format/CDataInterface.html) to share data within the same process via foreign interfaces as well as across processes via the [flight protocol](https://arrow.apache.org/docs/format/Flight.html).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on a change in pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#discussion_r563282158
##########
File path: rust/arrow/src/compute/kernels/cast.rs
##########
@@ -443,6 +456,13 @@ pub fn cast(array: &ArrayRef, to_type: &DataType) -> Result<ArrayRef> {
))),
},
+ // start decimal casts
+ (Int8, Decimal(p, s)) => cast_numeric_to_decimal::<Int8Type>(array, *p, *s),
+ (Int16, Decimal(p, s)) => cast_numeric_to_decimal::<Int16Type>(array, *p, *s),
+ (Int32, Decimal(p, s)) => cast_numeric_to_decimal::<Int32Type>(array, *p, *s),
+ (Int64, Decimal(p, s)) => cast_numeric_to_decimal::<Int64Type>(array, *p, *s),
+ // end numeric casts
Review comment:
Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-792852299
> If we want to support Decimal(i128), can't we use Rust's native i128? E.g. does decimal128 + decimal128 correspond to Rust's i128 + i128, or is the math actually different?
@jorgecarleitao
It's using `i128` under the hood as expected (which generates to u64 and i64 by LLVM). As you know, `Decimal` has precision/scale and It's not possible to calculate the sum of two different decimals with different scales without rescaling, for example: `Decimal(1,1)` and `Decimal(1,5)`.
Yet another thing, it's overflowing.
`DECIMAL(1) + DECIMAL(1) = DECIMAL(1)`, example `1` + `9`.
I don't know how it should be done.
Related to `Decimal256`, probably it will use (but it will be done in another PR)
`[i64, u64, u64, u64]` or `[i128, u128]` or `BigInt`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr edited a comment on pull request #9232: ARROW-10818: [Rust] Initial implementation for Decimal 128/256
Posted by GitBox <gi...@apache.org>.
ovr edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-813079188
@jorgecarleitao Can you take a look at it again? Thanks. Did I understand you correctly? Now Decimal types has ::NATIVE types, which represents a native type. `DecimalArray` started to be generic and store values as native type (I didn't change builder for it, because PR is epic large). It doesn't allocate any new structure if it doesn't need it. BTW: I created a polyfill for `i256` to test requirements for tests correctly.
For `i128`/`i256` I created `ArrowDecimalNativeType` which is `ArrowNativeType` + two additional methods: `pow_to` and `as_string` which helps to generate string from native representation, without creation of DecimalType structure.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr edited a comment on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr edited a comment on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-766338705
> Looking at the spec, a decimal type only supports 128 and 256 bits. So, I am not understanding why we are trying to add support for Int32,Int64,Int128,LargeDecimal here.
Arrow is used inside DF, which is used to build databases on to of it. If the user defines `DECIMAL(2,2)` it's a bit overhead to use i128 as representation for it.
1. Memory usage, `DecimalArray<Decimal128>` will use 16 bytes for each element.
2. Performance, Rust is using LLVM, which allows integers of any bit width from 1 to 2^23. Inside LLVM backend for x86-64, It will use rax, rcx to store 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ovr commented on pull request #9232: ARROW-10818: [Rust] Implement DecimalType
Posted by GitBox <gi...@apache.org>.
ovr commented on pull request #9232:
URL: https://github.com/apache/arrow/pull/9232#issuecomment-789732012
> @ovr what do you think we should do with this PR? Is it worth keeping open as a Draft or shall we close it for now?
It's time to finish it. So, I've rebased PR and drop `Decimal256` from it (will send it in another PR). Planing to finish it on weekends.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org