[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType

[GitBox]

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:
us...@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType

[GitBox]

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:
us...@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType

[GitBox]

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: , to_type: ) -> 
Result {
 ))),
 },
 
+// start decimal casts
+(Int8, Decimal(p, s)) => cast_numeric_to_decimal::(array, 
*p, *s),
+(Int16, Decimal(p, s)) => cast_numeric_to_decimal::(array, 
*p, *s),
+(Int32, Decimal(p, s)) => cast_numeric_to_decimal::(array, 
*p, *s),
+(Int64, Decimal(p, s)) => cast_numeric_to_decimal::(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:
us...@infra.apache.org

[GitHub] [arrow] alamb commented on a change in pull request #9232: ARROW-10818: [Rust] Implement DecimalType

[GitBox]

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>,
 }
 
+// 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;
+
+fn get_byte_width_for_precision_scale(precision: usize, scale: usize) -> 
usize;
+
+// Rescale scale part
+fn rescale( self, scale: usize);
+
+// Try to parse string with precision, scale
+fn parse(
+string: ,
+precision: usize,
+scale: usize,
+) -> std::result::Result;
+
+fn to_byte_slice() -> Vec;
+
+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:
us...@infra.apache.org