Re: [PR] [Variant]: Implement `DataType::Decimal32/Decimal64/Decimal128/Decimal256` support for `cast_to_variant` kernel [arrow-rs]

[via GitHub]

alamb commented on code in PR #8101:
URL: https://github.com/apache/arrow-rs/pull/8101#discussion_r2271313344


##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -482,6 +568,302 @@ mod tests {
         )
     }
 
+    #[test]
+    fn test_cast_to_variant_decimal32() {
+        run_test(
+            Arc::new(
+                Decimal32Array::from(vec![
+                    Some(i32::MIN),
+                    Some(-max_unscaled_value!(32, DECIMAL32_MAX_PRECISION)),
+                    None,
+                    Some(-123),
+                    Some(0),
+                    Some(123),
+                    Some(max_unscaled_value!(32, DECIMAL32_MAX_PRECISION)),
+                    Some(i32::MAX),
+                ])
+                .with_precision_and_scale(DECIMAL32_MAX_PRECISION, 3)
+                .unwrap(),
+            ),
+            vec![
+                Some(Variant::Null),
+                Some(
+                    VariantDecimal4::try_new(-max_unscaled_value!(32, 
DECIMAL32_MAX_PRECISION), 3)
+                        .unwrap()
+                        .into(),
+                ),
+                None,
+                Some(VariantDecimal4::try_new(-123, 3).unwrap().into()),
+                Some(VariantDecimal4::try_new(0, 3).unwrap().into()),
+                Some(VariantDecimal4::try_new(123, 3).unwrap().into()),
+                Some(
+                    VariantDecimal4::try_new(max_unscaled_value!(32, 
DECIMAL32_MAX_PRECISION), 3)
+                        .unwrap()
+                        .into(),
+                ),
+                Some(Variant::Null),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_cast_to_variant_decimal32_negative_scale() {
+        run_test(
+            Arc::new(
+                Decimal32Array::from(vec![
+                    Some(i32::MIN),
+                    Some(-max_unscaled_value!(32, DECIMAL32_MAX_PRECISION)),
+                    None,
+                    Some(-123),
+                    Some(0),
+                    Some(123),
+                    Some(max_unscaled_value!(32, DECIMAL32_MAX_PRECISION)),
+                    Some(i32::MAX),
+                ])
+                .with_precision_and_scale(DECIMAL32_MAX_PRECISION, -3)
+                .unwrap(),
+            ),
+            vec![
+                Some(Variant::Null),
+                Some(Variant::Null),
+                None,
+                Some(VariantDecimal4::try_new(-123_000, 0).unwrap().into()),
+                Some(VariantDecimal4::try_new(0, 0).unwrap().into()),
+                Some(VariantDecimal4::try_new(123_000, 0).unwrap().into()),
+                Some(Variant::Null),
+                Some(Variant::Null),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_cast_to_variant_decimal64() {
+        run_test(
+            Arc::new(
+                Decimal64Array::from(vec![
+                    Some(i64::MIN),
+                    Some(-max_unscaled_value!(64, DECIMAL64_MAX_PRECISION)),
+                    None,
+                    Some(-123),
+                    Some(0),
+                    Some(123),
+                    Some(max_unscaled_value!(64, DECIMAL64_MAX_PRECISION)),
+                    Some(i64::MAX),
+                ])
+                .with_precision_and_scale(DECIMAL64_MAX_PRECISION, 3)
+                .unwrap(),
+            ),
+            vec![
+                Some(Variant::Null),
+                Some(
+                    VariantDecimal8::try_new(-max_unscaled_value!(64, 
DECIMAL64_MAX_PRECISION), 3)
+                        .unwrap()
+                        .into(),
+                ),
+                None,
+                Some(VariantDecimal8::try_new(-123, 3).unwrap().into()),
+                Some(VariantDecimal8::try_new(0, 3).unwrap().into()),
+                Some(VariantDecimal8::try_new(123, 3).unwrap().into()),
+                Some(
+                    VariantDecimal8::try_new(max_unscaled_value!(64, 
DECIMAL64_MAX_PRECISION), 3)
+                        .unwrap()
+                        .into(),
+                ),
+                Some(Variant::Null),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_cast_to_variant_decimal64_negative_scale() {
+        run_test(
+            Arc::new(
+                Decimal64Array::from(vec![
+                    Some(i64::MIN),
+                    Some(-max_unscaled_value!(64, DECIMAL64_MAX_PRECISION)),
+                    None,
+                    Some(-123),
+                    Some(0),
+                    Some(123),
+                    Some(max_unscaled_value!(64, DECIMAL64_MAX_PRECISION)),
+                    Some(i64::MAX),
+                ])
+                .with_precision_and_scale(DECIMAL64_MAX_PRECISION, -3)
+                .unwrap(),
+            ),
+            vec![
+                Some(Variant::Null),
+                Some(Variant::Null),
+                None,
+                Some(VariantDecimal8::try_new(-123_000, 0).unwrap().into()),
+                Some(VariantDecimal8::try_new(0, 0).unwrap().into()),
+                Some(VariantDecimal8::try_new(123_000, 0).unwrap().into()),
+                Some(Variant::Null),
+                Some(Variant::Null),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_cast_to_variant_decimal128() {
+        run_test(
+            Arc::new(
+                Decimal128Array::from(vec![
+                    Some(i128::MIN),
+                    Some(-max_unscaled_value!(128, DECIMAL128_MAX_PRECISION)),
+                    None,
+                    Some(-123),
+                    Some(0),
+                    Some(123),
+                    Some(max_unscaled_value!(128, DECIMAL128_MAX_PRECISION)),
+                    Some(i128::MAX),
+                ])
+                .with_precision_and_scale(DECIMAL128_MAX_PRECISION, 3)
+                .unwrap(),
+            ),
+            vec![
+                Some(Variant::Null),
+                Some(
+                    VariantDecimal16::try_new(
+                        -max_unscaled_value!(128, DECIMAL128_MAX_PRECISION),
+                        3,
+                    )
+                    .unwrap()
+                    .into(),
+                ),
+                None,
+                Some(VariantDecimal16::try_new(-123, 3).unwrap().into()),
+                Some(VariantDecimal16::try_new(0, 3).unwrap().into()),
+                Some(VariantDecimal16::try_new(123, 3).unwrap().into()),
+                Some(
+                    VariantDecimal16::try_new(
+                        max_unscaled_value!(128, DECIMAL128_MAX_PRECISION),
+                        3,
+                    )
+                    .unwrap()
+                    .into(),
+                ),
+                Some(Variant::Null),
+            ],
+        )
+    }
+
+    #[test]
+    fn test_cast_to_variant_decimal128_negative_scale() {
+        run_test(
+            Arc::new(
+                Decimal128Array::from(vec![
+                    Some(i128::MIN),
+                    Some(-max_unscaled_value!(128, DECIMAL128_MAX_PRECISION)),

Review Comment:
   Shouldn't this be `DECIMAL256_MAX_PRECISION`? 
   
   
   ```suggestion
                       Some(-max_unscaled_value!(128, 
DECIMAL256_MAX_PRECISION)),
   ```
   
   Same question for the other uses below too



##########
parquet-variant-compute/src/cast_to_variant.rs:
##########
@@ -148,6 +174,50 @@ pub fn cast_to_variant(input: &dyn Array) -> 
Result<VariantArray, ArrowError> {
         DataType::Float64 => {
             primitive_conversion!(Float64Type, input, builder);
         }
+        DataType::Decimal32(_, scale) => {
+            cast_conversion!(
+                Decimal32Type,
+                as_primitive,
+                |v| decimal_to_variant_decimal!(v, scale, i32, 
VariantDecimal4),
+                input,
+                builder
+            );
+        }
+        DataType::Decimal64(_, scale) => {
+            cast_conversion!(
+                Decimal64Type,
+                as_primitive,
+                |v| decimal_to_variant_decimal!(v, scale, i64, 
VariantDecimal8),
+                input,
+                builder
+            );
+        }
+        DataType::Decimal128(_, scale) => {
+            cast_conversion!(
+                Decimal128Type,
+                as_primitive,
+                |v| decimal_to_variant_decimal!(v, scale, i128, 
VariantDecimal16),
+                input,
+                builder
+            );
+        }
+        DataType::Decimal256(_, scale) => {
+            cast_conversion!(
+                Decimal256Type,
+                as_primitive,
+                |v: i256| {
+                    // Since `i128::MAX` is larger than the max value of 
`VariantDecimal16`,

Review Comment:
   Since DataType::Decimal256 can store values larger than can be stored in 
VariantDecimal16, I don't think this comment is accurate -- don't we need 
another check for overflow?
   
   But I see that the tests covert the case of `i256::MAX` so this seems 
reasonable to me



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org