laskoviymishka commented on code in PR #2508:
URL: https://github.com/apache/iceberg-rust/pull/2508#discussion_r3406214058
##########
crates/iceberg/src/spec/values/decimal_utils.rs:
##########
@@ -120,6 +120,11 @@ pub fn decimal_mantissa(d: &Decimal) -> i128 {
}
}
+/// Get the decimal digit precision of a mantissa.
+pub fn decimal_precision(mantissa: i128) -> u32 {
+ mantissa.unsigned_abs().to_string().len() as u32
Review Comment:
This allocates a heap `String` on every call just to count digits, and it
now sits on the `Datum::to()` path that predicate `bind()` walks per-literal
during planning over decimal columns.
`checked_ilog10` gets the same answer alloc-free, including the `0`->`1`
case:
```rust
pub fn decimal_precision(mantissa: i128) -> u32 {
mantissa.unsigned_abs().checked_ilog10().map_or(1, |x| x + 1)
}
```
Worth a one-line doc noting `0` returns `1` while we're here.
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -1130,6 +1137,21 @@ impl Datum {
(PrimitiveLiteral::Int128(val), _, PrimitiveType::Long) =>
{
Ok(Datum::i128_to_i64(*val))
}
+ (
+ PrimitiveLiteral::Int128(val),
+ PrimitiveType::Decimal {
+ scale: self_scale, ..
+ },
+ PrimitiveType::Decimal {
+ precision,
+ scale: target_scale,
+ },
+ ) if self_scale == target_scale =>
Datum::decimal_from_mantissa(
Review Comment:
When scales differ this arm doesn't match and we fall through to the
catch-all, so `decimal(9,2)` -> `decimal(9,3)` surfaces as "Can't convert datum
from..." with no hint that scale conversion specifically is unsupported.
I'd add an explicit scale-differ arm returning something like "scale
conversion is not supported (decimal scale is fixed)". That fixes the
diagnostic and, since Java's `DecimalLiteral.to()` silently returns the
wrong-scale literal unchanged, it's a good place to document that we
deliberately diverge and reject instead. wdyt?
##########
crates/iceberg/src/spec/values/tests.rs:
##########
@@ -1424,3 +1424,115 @@ fn test_date_from_json_as_number() {
// Both formats should produce the same Literal value
}
+
+#[test]
+fn test_datum_to_decimal_narrows_precision_when_scale_matches() {
+ let target_type = Type::Primitive(PrimitiveType::Decimal {
+ precision: 9,
+ scale: 2,
+ });
+ let datum = Datum::decimal_from_str("123.45").unwrap();
+
+ let converted = datum.to(&target_type).unwrap();
+
+ assert_eq!(converted.data_type(), &PrimitiveType::Decimal {
+ precision: 9,
+ scale: 2,
+ });
+ assert_eq!(converted.literal(), &PrimitiveLiteral::Int128(12345));
+}
+
+#[test]
+fn test_datum_to_decimal_widens_precision_when_scale_matches() {
+ let target_type = Type::Primitive(PrimitiveType::Decimal {
+ precision: 38,
+ scale: 2,
+ });
+ let datum =
Datum::decimal_with_precision(decimal_from_i128_with_scale(12345, 2),
9).unwrap();
+
+ let converted = datum.to(&target_type).unwrap();
+
+ assert_eq!(converted.data_type(), &PrimitiveType::Decimal {
+ precision: 38,
+ scale: 2,
+ });
+ assert_eq!(converted.literal(), &PrimitiveLiteral::Int128(12345));
+}
+
+#[test]
+fn test_datum_to_decimal_rejects_precision_too_narrow() {
Review Comment:
These fixtures use `decimal(1,2)` -- scale greater than precision. Both Java
and Rust happen to let you construct it today (neither enforces `S <= P`), but
it's spec-invalid, and if `Type::decimal` ever grows that validation these
tests break at fixture construction. It also signals to readers that
`decimal(1,2)` is fine.
`decimal(1,1)` with value `1.5` (mantissa `15`, 2 digits) demonstrates the
same precision rejection without the `S > P` shape -- applies to the other
`decimal(1,2)` fixtures in this block too.
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -1130,6 +1137,21 @@ impl Datum {
(PrimitiveLiteral::Int128(val), _, PrimitiveType::Long) =>
{
Ok(Datum::i128_to_i64(*val))
}
+ (
+ PrimitiveLiteral::Int128(val),
+ PrimitiveType::Decimal {
+ scale: self_scale, ..
+ },
+ PrimitiveType::Decimal {
+ precision,
+ scale: target_scale,
+ },
+ ) if self_scale == target_scale =>
Datum::decimal_from_mantissa(
+ *val,
+ *precision,
+ *target_scale,
+ self.to_human_string(),
Review Comment:
`self.to_human_string()` is evaluated eagerly as an argument, so we allocate
a `String` on every successful cast, not just on the error path -- and that's
the whole reason `decimal_from_mantissa` carries the `T: Display` generic.
I'd drop the `value` param entirely and build the error message from
`mantissa`/`scale` inside the function (it has both). That kills the eager
alloc and removes the generic in one go.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
