laskoviymishka commented on code in PR #3243:
URL: https://github.com/apache/iceberg-rust/pull/3243#discussion_r4242733423
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -159,15 +158,18 @@ 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,
- ))?
+ Datum::new(
+ input.data_type().clone(),
+ PrimitiveLiteral::Int128(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())
})),
+ PrimitiveLiteral::Binary(v) if input.data_type() ==
&PrimitiveType::Binary => Ok(Some(
Review Comment:
This arm is correct only because the `== &PrimitiveType::Binary` guard keeps
Fixed out — `Datum::fixed` and `Datum::binary` both store
`PrimitiveLiteral::Binary`, so without the guard a Fixed value would truncate
and come back typed as binary. I'd add a one-line comment saying exactly that,
since it's an easy guard to drop in a later refactor.
(`matches!(input.data_type(), PrimitiveType::Binary)` also reads a touch
cleaner than `== &..` if you're on the line anyway.)
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -159,15 +158,18 @@ 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,
- ))?
+ Datum::new(
Review Comment:
We're now building `Datum::new(type.clone(), Int128(..))` in three places —
here plus `try_increment_number` / `try_decrement_number` — all through the
unchecked `pub(crate)` constructor. Passing the unscaled value straight through
is the right call for the fix, so my only suggestion is pulling it into a small
`Datum`-level helper so the idiom lives in one spot instead of being copied.
Fine as a follow-up if you'd rather keep this PR tight.
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -159,15 +158,18 @@ 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,
- ))?
+ Datum::new(
+ input.data_type().clone(),
+ PrimitiveLiteral::Int128(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())
})),
+ PrimitiveLiteral::Binary(v) if input.data_type() ==
&PrimitiveType::Binary => Ok(Some(
+ Datum::binary(Self::truncate_binary(v, self.width as
usize).to_vec()),
Review Comment:
`Datum::binary` already collects from an `IntoIterator<u8>`, so the
`.to_vec()` allocates once and then the constructor collects again. Dropping to
an iterator skips the intermediate `Vec` — small, non-hot path:
```suggestion
Datum::binary(Self::truncate_binary(v, self.width as
usize).iter().copied()),
```
##########
crates/iceberg/src/transform/truncate.rs:
##########
@@ -831,12 +860,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() {
Review Comment:
These exercise `transform_literal` directly, but the binary arm is now
reachable through strict/inclusive projection too and nothing covers that path.
A `TestProjectionFixture` binary case (`=`, `<=`, `IN`) would lock in the
projection behavior the same way the decimal fixtures do. Follow-up is fine.
--
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]
