This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3243-b55cda1cb9e503336bdd41e2b2489415216577d7 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit b154603a710d730d2f43f8af4db257cc137237ef Author: advancedxy <[email protected]> AuthorDate: Mon Oct 12 19:41:02 2026 +0000 fix(transform): correct decimal and binary truncate literals (#3243) * fix(transform): correct decimal and binary truncate literals The Arrow array path already preserves decimal precision and scale and supports binary truncation. The literal path instead reset decimal scale to zero and rejected binary values. For example, truncate[50](10.65) returned 1050 instead of 10.50. Align literal truncation with the Arrow array path: preserve the source decimal precision and scale, and return the first W bytes of binary values, or the entire value when shorter. Decimal projection boundary adjustments must also preserve precision and scale to keep their values correct before truncation. * fix(transform): address truncate literal review feedback --- .../iceberg/src/expr/visitors/strict_projection.rs | 24 +- crates/iceberg/src/spec/transform.rs | 13 +- crates/iceberg/src/spec/values/datum.rs | 16 ++ crates/iceberg/src/transform/truncate.rs | 241 +++++++++++++++++++-- 4 files changed, 251 insertions(+), 43 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/strict_projection.rs b/crates/iceberg/src/expr/visitors/strict_projection.rs index ebc6212c7..2188e6df0 100644 --- a/crates/iceberg/src/expr/visitors/strict_projection.rs +++ b/crates/iceberg/src/expr/visitors/strict_projection.rs @@ -2761,7 +2761,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 10000)".to_string() + "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 100.00)".to_string() ); // test less or eq @@ -2780,7 +2780,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 10000)".to_string() + "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 100.00)".to_string() ); // test greater @@ -2799,7 +2799,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 > 100) AND (pcol2 > 100)) AND (pcol3 > 10000)".to_string() + "((pcol1 > 100) AND (pcol2 > 100)) AND (pcol3 > 100.00)".to_string() ); // test greater or eq @@ -2818,7 +2818,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 9990)".to_string() + "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 99.90)".to_string() ); // test not eq @@ -2837,7 +2837,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 != 100) AND (pcol2 != 100)) AND (pcol3 != 10000)".to_string() + "((pcol1 != 100) AND (pcol2 != 100)) AND (pcol3 != 100.00)".to_string() ); // test not in @@ -2880,7 +2880,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 NOT IN (100, 90)) AND (pcol2 NOT IN (100, 90))) AND (pcol3 NOT IN (10000, 10100, 9900))" + "((pcol1 NOT IN (100, 90)) AND (pcol2 NOT IN (100, 90))) AND (pcol3 NOT IN (100.00, 99.00, 101.00))" .to_string() ); @@ -2984,7 +2984,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 < 90) AND (pcol2 < 90)) AND (pcol3 < 9990)".to_string() + "((pcol1 < 90) AND (pcol2 < 90)) AND (pcol3 < 99.90)".to_string() ); // test less or eq @@ -3003,7 +3003,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 10000)".to_string() + "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 100.00)".to_string() ); // test greater @@ -3022,7 +3022,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 9990)".to_string() + "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 99.90)".to_string() ); // test greater or eq @@ -3041,7 +3041,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 9990)".to_string() + "((pcol1 > 90) AND (pcol2 > 90)) AND (pcol3 > 99.90)".to_string() ); // test not eq @@ -3060,7 +3060,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 != 90) AND (pcol2 != 90)) AND (pcol3 != 9990)".to_string() + "((pcol1 != 90) AND (pcol2 != 90)) AND (pcol3 != 99.90)".to_string() ); // test not in @@ -3103,7 +3103,7 @@ mod tests { let result = strict_projection.strict_project(&predicate).unwrap(); assert_eq!( result.to_string(), - "((pcol1 NOT IN (100, 90)) AND (pcol2 NOT IN (100, 90))) AND (pcol3 NOT IN (9890, 9990, 10090))" + "((pcol1 NOT IN (100, 90)) AND (pcol2 NOT IN (100, 90))) AND (pcol3 NOT IN (98.90, 100.90, 99.90))" .to_string() ); diff --git a/crates/iceberg/src/spec/transform.rs b/crates/iceberg/src/spec/transform.rs index d87054ab7..c28dc28c1 100644 --- a/crates/iceberg/src/spec/transform.rs +++ b/crates/iceberg/src/spec/transform.rs @@ -25,7 +25,6 @@ use chrono::Datelike; use fnv::FnvHashSet; use serde::{Deserialize, Deserializer, Serialize, Serializer}; -use super::values::decimal_utils::decimal_from_i128_with_scale; use super::values::temporal::date; use super::{Datum, PrimitiveLiteral}; use crate::ErrorKind; @@ -762,8 +761,8 @@ impl Transform { PredicateOperator::LessThan => match (datum.data_type(), datum.literal()) { (PrimitiveType::Int, PrimitiveLiteral::Int(v)) => Some(Datum::int(v - 1)), (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => Some(Datum::long(v - 1)), - (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(v)) => { - Some(Datum::decimal(decimal_from_i128_with_scale(v - 1, 0))?) + (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(_)) => { + Some(Self::try_decrement_number(datum)?) } (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => Some(Datum::date(v - 1)), (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => { @@ -783,8 +782,8 @@ impl Transform { PredicateOperator::GreaterThan => match (datum.data_type(), datum.literal()) { (PrimitiveType::Int, PrimitiveLiteral::Int(v)) => Some(Datum::int(v + 1)), (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => Some(Datum::long(v + 1)), - (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(v)) => { - Some(Datum::decimal(decimal_from_i128_with_scale(v + 1, 0))?) + (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(_)) => { + Some(Self::try_increment_number(datum)?) } (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => Some(Datum::date(v + 1)), (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => { @@ -934,7 +933,7 @@ impl Transform { (PrimitiveType::Int, PrimitiveLiteral::Int(v)) => Ok(Datum::int(v + 1)), (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => Ok(Datum::long(v + 1)), (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(v)) => { - Datum::decimal(decimal_from_i128_with_scale(v + 1, 0)) + datum.with_decimal_mantissa(v + 1) } (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => Ok(Datum::date(v + 1)), (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => { @@ -972,7 +971,7 @@ impl Transform { (PrimitiveType::Int, PrimitiveLiteral::Int(v)) => Ok(Datum::int(v - 1)), (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => Ok(Datum::long(v - 1)), (PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(v)) => { - Datum::decimal(decimal_from_i128_with_scale(v - 1, 0)) + datum.with_decimal_mantissa(v - 1) } (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => Ok(Datum::date(v - 1)), (PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => { diff --git a/crates/iceberg/src/spec/values/datum.rs b/crates/iceberg/src/spec/values/datum.rs index 57d967573..c4c835f61 100644 --- a/crates/iceberg/src/spec/values/datum.rs +++ b/crates/iceberg/src/spec/values/datum.rs @@ -1099,6 +1099,22 @@ impl Datum { } } + /// Replaces a decimal's unscaled value while preserving its precision and scale. + /// + /// The value is not checked against precision because transform projections + /// may use boundaries outside the source type's range. + pub(crate) fn with_decimal_mantissa(&self, mantissa: i128) -> Result<Self> { + ensure_data_valid!( + matches!(self.r#type, PrimitiveType::Decimal { .. }), + "Expected a decimal datum, got: {:?}", + self.r#type + ); + Ok(Self::new( + self.r#type.clone(), + PrimitiveLiteral::Int128(mantissa), + )) + } + /// Convert the datum to `target_type`. pub fn to(self, target_type: &Type) -> Result<Datum> { match target_type { diff --git a/crates/iceberg/src/transform/truncate.rs b/crates/iceberg/src/transform/truncate.rs index 0f95854ee..2bb7fdf22 100644 --- a/crates/iceberg/src/transform/truncate.rs +++ b/crates/iceberg/src/transform/truncate.rs @@ -23,8 +23,7 @@ use arrow_schema::DataType; use super::TransformFunction; use crate::Error; use crate::error::invalid_data; -use crate::spec::decimal_utils::decimal_from_i128_with_scale; -use crate::spec::{Datum, PrimitiveLiteral}; +use crate::spec::{Datum, PrimitiveLiteral, PrimitiveType}; #[derive(Debug)] pub struct Truncate { @@ -159,15 +158,20 @@ impl TransformFunction for Truncate { })), PrimitiveLiteral::Int128(v) => Ok(Some({ let width = self.width as i128; - Datum::decimal(decimal_from_i128_with_scale( - Self::truncate_decimal_i128(*v, width), - 0, - ))? + input.with_decimal_mantissa(Self::truncate_decimal_i128(*v, width))? })), PrimitiveLiteral::String(v) => Ok(Some({ let len = self.width as usize; Datum::string(Self::truncate_str(v, len).to_string()) })), + // Fixed shares this literal variant with Binary but does not support truncate. + PrimitiveLiteral::Binary(v) if matches!(input.data_type(), PrimitiveType::Binary) => { + Ok(Some(Datum::binary( + Self::truncate_binary(v, self.width as usize) + .iter() + .copied(), + ))) + } _ => Err(Error::new( crate::ErrorKind::FeatureUnsupported, format!( @@ -194,8 +198,7 @@ mod test { TimestampNs, Timestamptz, TimestamptzNs, Uuid, }; use crate::spec::Type::{Primitive, Struct}; - use crate::spec::decimal_utils::decimal_new; - use crate::spec::{Datum, NestedField, PrimitiveType, StructType, Transform}; + use crate::spec::{Datum, NestedField, PrimitiveLiteral, PrimitiveType, StructType, Transform}; use crate::transform::TransformFunction; use crate::transform::test::{TestProjectionFixture, TestTransformFixture}; @@ -250,6 +253,128 @@ mod test { fixture.assert_transform(trans); } + #[test] + fn test_projection_truncate_binary() -> Result<()> { + let fixture = TestProjectionFixture::new( + Transform::Truncate(3), + "name", + NestedField::required(1, "value", Primitive(Binary)), + ); + + for (value, prefix) in [ + (vec![0x00, 0xff, 0x80, 0x01], "00FF80"), + (vec![0x00, 0xff, 0x80], "00FF80"), + (vec![0x00, 0xff], "00FF"), + ] { + for (op, projected_op) in [ + (PredicateOperator::Eq, "="), + (PredicateOperator::LessThanOrEq, "<="), + ] { + fixture.assert_projection( + &fixture.binary_predicate(op, Datum::binary(value.iter().copied())), + Some(&format!("name {projected_op} {prefix}")), + )?; + } + } + + // Both source values share one truncated prefix, so the projected set has one value. + fixture.assert_projection( + &fixture.set_predicate(PredicateOperator::In, vec![ + Datum::binary([0x00, 0xff, 0x80, 0x01]), + Datum::binary([0x00, 0xff, 0x80, 0x02]), + ]), + Some("name IN (00FF80)"), + )?; + + Ok(()) + } + + #[test] + fn test_strict_projection_truncate_binary() -> Result<()> { + let transform = Transform::Truncate(3); + let fixture = TestProjectionFixture::new( + transform, + "name", + NestedField::required(1, "value", Primitive(Binary)), + ); + + for (op, expected) in [ + (PredicateOperator::LessThanOrEq, Some("name < 00FF80")), + (PredicateOperator::NotEq, Some("name != 00FF80")), + (PredicateOperator::Eq, None), + ] { + let predicate = fixture.binary_predicate(op, Datum::binary([0x00, 0xff, 0x80, 0x01])); + let result = transform.strict_project("name", &predicate)?; + assert_eq!(result.map(|p| p.to_string()).as_deref(), expected); + } + + for (op, expected) in [ + (PredicateOperator::NotIn, Some("name NOT IN (00FF80)")), + (PredicateOperator::In, None), + ] { + let predicate = fixture.set_predicate(op, vec![ + Datum::binary([0x00, 0xff, 0x80, 0x01]), + Datum::binary([0x00, 0xff, 0x80, 0x02]), + ]); + let result = transform.strict_project("name", &predicate)?; + assert_eq!(result.map(|p| p.to_string()).as_deref(), expected); + } + + Ok(()) + } + + #[test] + fn test_projection_truncate_decimal_outside_precision() -> Result<()> { + let transform = Transform::Truncate(10); + let fixture = TestProjectionFixture::new( + transform, + "name", + NestedField::required( + 1, + "value", + Primitive(Decimal { + precision: 2, + scale: 1, + }), + ), + ); + + for (value, inclusive_op, inclusive_expected, strict_op, strict_expected) in [ + ( + 99, + PredicateOperator::GreaterThan, + "name >= 10.0", + PredicateOperator::LessThanOrEq, + "name < 10.0", + ), + ( + -99, + PredicateOperator::LessThan, + "name <= -10.0", + PredicateOperator::GreaterThanOrEq, + "name > -10.0", + ), + ] { + let datum = Datum::new( + Decimal { + precision: 2, + scale: 1, + }, + PrimitiveLiteral::Int128(value), + ); + fixture.assert_projection( + &fixture.binary_predicate(inclusive_op, datum.clone()), + Some(inclusive_expected), + )?; + let result = transform + .strict_project("name", &fixture.binary_predicate(strict_op, datum))? + .unwrap(); + assert_eq!(result.to_string(), strict_expected); + } + + Ok(()) + } + #[test] fn test_projection_truncate_string_rewrite_op() -> Result<()> { let value = "abcde"; @@ -359,7 +484,7 @@ mod test { fixture.assert_projection( &fixture.binary_predicate(PredicateOperator::LessThan, Datum::decimal_from_str(curr)?), - Some("name <= 9990"), + Some("name <= 99.90"), )?; fixture.assert_projection( @@ -367,7 +492,7 @@ mod test { PredicateOperator::LessThanOrEq, Datum::decimal_from_str(curr)?, ), - Some("name <= 9990"), + Some("name <= 99.90"), )?; fixture.assert_projection( @@ -375,12 +500,12 @@ mod test { PredicateOperator::GreaterThanOrEq, Datum::decimal_from_str(curr)?, ), - Some("name >= 9990"), + Some("name >= 99.90"), )?; fixture.assert_projection( &fixture.binary_predicate(PredicateOperator::Eq, Datum::decimal_from_str(curr)?), - Some("name = 9990"), + Some("name = 99.90"), )?; fixture.assert_projection( @@ -394,7 +519,7 @@ mod test { Datum::decimal_from_str(curr)?, Datum::decimal_from_str(next)?, ]), - Some("name IN (9890, 9990, 10090)"), + Some("name IN (99.90, 100.90, 98.90)"), )?; fixture.assert_projection( @@ -429,7 +554,7 @@ mod test { fixture.assert_projection( &fixture.binary_predicate(PredicateOperator::LessThan, Datum::decimal_from_str(curr)?), - Some("name <= 9990"), + Some("name <= 99.90"), )?; fixture.assert_projection( @@ -437,7 +562,7 @@ mod test { PredicateOperator::LessThanOrEq, Datum::decimal_from_str(curr)?, ), - Some("name <= 10000"), + Some("name <= 100.00"), )?; fixture.assert_projection( @@ -445,12 +570,12 @@ mod test { PredicateOperator::GreaterThanOrEq, Datum::decimal_from_str(curr)?, ), - Some("name >= 10000"), + Some("name >= 100.00"), )?; fixture.assert_projection( &fixture.binary_predicate(PredicateOperator::Eq, Datum::decimal_from_str(curr)?), - Some("name = 10000"), + Some("name = 100.00"), )?; fixture.assert_projection( @@ -464,7 +589,7 @@ mod test { Datum::decimal_from_str(curr)?, Datum::decimal_from_str(next)?, ]), - Some("name IN (10000, 10100, 9900)"), + Some("name IN (99.00, 100.00, 101.00)"), )?; fixture.assert_projection( @@ -795,6 +920,34 @@ mod test { assert_eq!(super::Truncate::truncate_str(test4, 1), test4_1_expected); } + #[test] + fn test_literal_spec_examples() -> Result<()> { + // https://iceberg.apache.org/spec/#truncate-transform-details + let cases = [ + (10, Datum::int(1), Datum::int(0)), + (10, Datum::int(-1), Datum::int(-10)), + (10, Datum::long(1), Datum::long(0)), + (10, Datum::long(-1), Datum::long(-10)), + ( + 50, + Datum::decimal_from_str("10.65")?, + Datum::decimal_from_str("10.50")?, + ), + (3, Datum::string("iceberg"), Datum::string("ice")), + ( + 3, + Datum::binary(vec![0x01, 0x02, 0x03, 0x04, 0x05]), + Datum::binary(vec![0x01, 0x02, 0x03]), + ), + ]; + + for (width, input, expected) in cases { + let result = super::Truncate::new(width).transform_literal(&input)?; + assert_eq!(result, Some(expected), "width: {width}, input: {input:?}"); + } + Ok(()) + } + #[test] fn test_literal_int() { let input = Datum::int(1); @@ -831,12 +984,52 @@ mod test { #[test] fn test_decimal_literal() { - let input = Datum::decimal(decimal_new(1065, 0)).unwrap(); - let res = super::Truncate::new(50) - .transform_literal(&input) - .unwrap() - .unwrap(); - assert_eq!(res, Datum::decimal(decimal_new(1050, 0)).unwrap(),); + for (precision, scale, value, expected) in [ + (9, 0, 1065, 1050), + (9, 2, 1065, 1050), + (9, 2, -1065, -1100), + (9, 2, 1050, 1050), + (9, 2, 0, 0), + (9, 5, 1065, 1050), + (38, 2, 1065, 1050), + ] { + let data_type = Decimal { precision, scale }; + let input = Datum::new(data_type.clone(), PrimitiveLiteral::Int128(value)); + let res = super::Truncate::new(50) + .transform_literal(&input) + .unwrap() + .unwrap(); + assert_eq!( + res, + Datum::new(data_type, PrimitiveLiteral::Int128(expected)), + "input: {input:?}" + ); + } + } + + #[test] + fn test_binary_literal() { + for (value, width, expected) in [ + (vec![0x00, 0xff, 0x80, 0x01], 2, vec![0x00, 0xff]), + (vec![0x00, 0xff], 2, vec![0x00, 0xff]), + (vec![0x00, 0xff], 4, vec![0x00, 0xff]), + (vec![], 2, vec![]), + ] { + let input = Datum::binary(value); + let res = super::Truncate::new(width) + .transform_literal(&input) + .unwrap() + .unwrap(); + assert_eq!(res, Datum::binary(expected)); + } + } + + #[test] + fn test_fixed_literal_is_unsupported() { + let error = super::Truncate::new(2) + .transform_literal(&Datum::fixed(vec![0x00, 0xff, 0x80])) + .unwrap_err(); + assert_eq!(error.kind(), crate::ErrorKind::FeatureUnsupported); } #[test]
