Jefffrey commented on code in PR #7062:
URL: https://github.com/apache/arrow-rs/pull/7062#discussion_r1944545894
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -359,6 +329,34 @@ where
Ok(Arc::new(b.finish()))
}
+pub(crate) fn pack_decimal_to_dictionary<K, D, M>(
+ array: &dyn Array,
+ dict_value_type: &DataType,
+ precision: u8,
+ scale: i8,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError>
+where
+ K: ArrowDictionaryKeyType,
+ D: DecimalType + ArrowPrimitiveType<Native = M>,
+ M: ArrowNativeTypeOp + DecimalCast,
+{
+ let dict = pack_numeric_to_dictionary::<K, D>(array, dict_value_type,
cast_options)?;
+ let dict = dict
+ .as_dictionary::<K>()
+ .downcast_dict::<PrimitiveArray<D>>()
+ .ok_or_else(|| {
+ ArrowError::ComputeError(format!("Internal Error: Cannot cast dict
to {}", D::PREFIX))
Review Comment:
```suggestion
ArrowError::ComputeError(format!("Internal Error: Cannot cast
dict to {}Array", D::PREFIX))
```
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -25,6 +25,8 @@ pub(crate) trait DecimalCast: Sized {
fn to_i256(self) -> Option<i256>;
fn from_decimal<T: DecimalCast>(n: T) -> Option<Self>;
+
+ fn from_f64<T: DecimalCast>(n: f64) -> Option<Self>;
Review Comment:
```suggestion
fn from_f64(n: f64) -> Option<Self>;
```
Generic unused
##########
arrow-cast/src/cast/decimal.rs:
##########
@@ -464,86 +474,41 @@ where
Ok(Arc::new(result))
}
-pub(crate) fn cast_floating_point_to_decimal128<T: ArrowPrimitiveType>(
+pub(crate) fn cast_floating_point_to_decimal<T: ArrowPrimitiveType, D, M>(
array: &PrimitiveArray<T>,
precision: u8,
scale: i8,
cast_options: &CastOptions,
) -> Result<ArrayRef, ArrowError>
where
<T as ArrowPrimitiveType>::Native: AsPrimitive<f64>,
+ D: DecimalType + ArrowPrimitiveType<Native = M>,
+ M: ArrowNativeTypeOp + DecimalCast,
Review Comment:
```suggestion
pub(crate) fn cast_floating_point_to_decimal<T: ArrowPrimitiveType, D>(
array: &PrimitiveArray<T>,
precision: u8,
scale: i8,
cast_options: &CastOptions,
) -> Result<ArrayRef, ArrowError>
where
<T as ArrowPrimitiveType>::Native: AsPrimitive<f64>,
D: DecimalType + ArrowPrimitiveType,
<D as ArrowPrimitiveType>::Native: DecimalCast,
```
Personally I find having `M` there confusing as in the callsite, you'd never
specify it anyway. Though I see this is used in other similar functions so I
guess this is more of a stylistic choice. Fine with either way, just noting my
thoughts.
##########
arrow-cast/src/cast/dictionary.rs:
##########
@@ -359,6 +329,34 @@ where
Ok(Arc::new(b.finish()))
}
+pub(crate) fn pack_decimal_to_dictionary<K, D, M>(
+ array: &dyn Array,
+ dict_value_type: &DataType,
+ precision: u8,
+ scale: i8,
+ cast_options: &CastOptions,
+) -> Result<ArrayRef, ArrowError>
+where
+ K: ArrowDictionaryKeyType,
+ D: DecimalType + ArrowPrimitiveType<Native = M>,
+ M: ArrowNativeTypeOp + DecimalCast,
Review Comment:
```suggestion
pub(crate) fn pack_decimal_to_dictionary<K, D>(
array: &dyn Array,
dict_value_type: &DataType,
precision: u8,
scale: i8,
cast_options: &CastOptions,
) -> Result<ArrayRef, ArrowError>
where
K: ArrowDictionaryKeyType,
D: DecimalType + ArrowPrimitiveType,
```
Seems to work fine with just this
##########
arrow-schema/src/ffi.rs:
##########
@@ -523,10 +520,11 @@ impl TryFrom<&FFI_ArrowSchema> for DataType {
"The decimal type requires an integer
scale".to_string(),
)
})?;
- if *bits == "128" {
- DataType::Decimal128(parsed_precision,
parsed_scale)
- } else {
- DataType::Decimal256(parsed_precision,
parsed_scale)
+ let parsed_bits =
bits.parse::<u16>().unwrap_or(0);
+ match parsed_bits {
+ 128 =>
DataType::Decimal128(parsed_precision, parsed_scale),
+ 256 =>
DataType::Decimal256(parsed_precision, parsed_scale),
+ _ => return
Err(ArrowError::CDataInterface("Only 128- and 256- bit wide decimals are
supported in the Rust implementation".to_string())),
Review Comment:
```suggestion
match *bits {
"128" =>
DataType::Decimal128(parsed_precision, parsed_scale),
"256" =>
DataType::Decimal256(parsed_precision, parsed_scale),
_ => return
Err(ArrowError::CDataInterface("Only 128- and 256- bit wide decimals are
supported in the Rust implementation".to_string())),
}
```
This also works, without need of parsing bits
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
