leaves12138 commented on code in PR #907:
URL: https://github.com/apache/paimon-rust/pull/907#discussion_r4067815358
##########
crates/paimon/src/arrow/nested_evolution.rs:
##########
@@ -64,6 +66,62 @@ pub(crate) fn evolve_column(
return Ok(source.clone());
}
+ if let (
+ ArrowDataType::Decimal128(_, source_scale),
+ ArrowDataType::Decimal128(target_precision, target_scale),
+ ) = (source.data_type(), &target_arrow)
+ {
+ if source_scale > target_scale {
+ // Arrow rounds Decimal128 scale reductions. Paimon schema casts,
+ // like Python's unsafe Arrow cast, truncate toward zero instead.
+ let shift = u32::try_from(i16::from(*source_scale) -
i16::from(*target_scale))
+ .map_err(|_| crate::Error::DataInvalid {
+ message: "invalid decimal scale reduction".to_string(),
+ source: None,
+ })?;
+ let divisor = 10_i128
+ .checked_pow(shift)
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: "decimal scale reduction exceeds Decimal128
range".to_string(),
+ source: None,
+ })?;
+ let decimals = source
+ .as_any()
+ .downcast_ref::<Decimal128Array>()
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: format!("expected Decimal128 array, got {:?}",
source.data_type()),
+ source: None,
+ })?;
+ let reduced = Decimal128Array::from(
+ (0..decimals.len())
+ .map(|index| {
+ decimals
+ .is_valid(index)
+ .then(|| decimals.value(index) / divisor)
+ })
+ .collect::<Vec<_>>(),
+ )
+ .with_precision_and_scale(*target_precision, *target_scale)
Review Comment:
[P2] Enforce the target precision on the reduced decimal values
`Decimal128Array::with_precision_and_scale` validates the type parameters,
not whether the array's values fit the declared precision. Therefore this
`map_err` does not catch value overflow, and the new branch can return an
invalid decimal array.
Independent of the rounding policy, a valid `DECIMAL(6,3)` value `999.999`
converted to `DECIMAL(3,2)` currently returns `999.99` (unscaled `99999`) in an
array declared to have precision 3; the maximum representable value is `9.99`.
Calling `validate_decimal_precision(3)` on the result fails. The previous
Arrow-cast path returns NULL for this overflow, as does Java's decimal cast.
Please check the reduced values against the target precision and preserve
overflow-to-NULL behavior, including when precision and scale both decrease. A
regression test should validate the resulting array, not just its declared data
type.
##########
crates/paimon/src/arrow/nested_evolution.rs:
##########
@@ -64,6 +66,62 @@ pub(crate) fn evolve_column(
return Ok(source.clone());
}
+ if let (
+ ArrowDataType::Decimal128(_, source_scale),
+ ArrowDataType::Decimal128(target_precision, target_scale),
+ ) = (source.data_type(), &target_arrow)
+ {
+ if source_scale > target_scale {
+ // Arrow rounds Decimal128 scale reductions. Paimon schema casts,
+ // like Python's unsafe Arrow cast, truncate toward zero instead.
+ let shift = u32::try_from(i16::from(*source_scale) -
i16::from(*target_scale))
+ .map_err(|_| crate::Error::DataInvalid {
+ message: "invalid decimal scale reduction".to_string(),
+ source: None,
+ })?;
+ let divisor = 10_i128
+ .checked_pow(shift)
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: "decimal scale reduction exceeds Decimal128
range".to_string(),
+ source: None,
+ })?;
+ let decimals = source
+ .as_any()
+ .downcast_ref::<Decimal128Array>()
+ .ok_or_else(|| crate::Error::DataInvalid {
+ message: format!("expected Decimal128 array, got {:?}",
source.data_type()),
+ source: None,
+ })?;
+ let reduced = Decimal128Array::from(
+ (0..decimals.len())
+ .map(|index| {
+ decimals
+ .is_valid(index)
+ .then(|| decimals.value(index) / divisor)
Review Comment:
[P2] Preserve Java's HALF_UP semantics for decimal scale reductions
The integer division changes a previously supported core read from rounding
to truncation. Java's `DecimalToDecimalCastRule` calls
`DecimalUtils.castToDecimal`, which calls `Decimal.fromBigDecimal`; that method
uses `setScale(scale, RoundingMode.HALF_UP)`, not truncation.
I reproduced `DECIMAL(6,3) -> DECIMAL(6,2)` with `4.567` and `-4.567`: the
existing Arrow-cast path returns `4.57` and `-4.57`, while this branch returns
`4.56` and `-4.56`. This silently changes the values read from existing files
and makes Rust disagree with Java.
Python's current unsafe Arrow cast does truncate, but matching that behavior
in the shared Rust read path does not preserve Java semantics. Could we keep
HALF_UP here and reconcile the Python expectation separately, with
positive/negative rounding cases in the regression coverage?
--
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]
