Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Implement Decimal from/to bytes represents #665

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ itertools = "0.13"
log = "0.4"
mockito = "1"
murmur3 = "0.5.2"
num-bigint = "0.4.6"
once_cell = "1"
opendal = "0.50"
ordered-float = "4"
Expand Down
1 change: 1 addition & 0 deletions crates/iceberg/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ futures = { workspace = true }
itertools = { workspace = true }
moka = { version = "0.12.8", features = ["future"] }
murmur3 = { workspace = true }
num-bigint = { workspace = true }
once_cell = { workspace = true }
opendal = { workspace = true }
ordered-float = { workspace = true }
Expand Down
65 changes: 60 additions & 5 deletions crates/iceberg/src/spec/values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@ use std::str::FromStr;
pub use _serde::RawLiteral;
use bitvec::vec::BitVec;
use chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Utc};
use num_bigint::BigInt;
use ordered_float::OrderedFloat;
use rust_decimal::prelude::ToPrimitive;
use rust_decimal::Decimal;
use serde::de::{
MapAccess, {self},
Expand Down Expand Up @@ -422,10 +424,15 @@ impl Datum {
}
PrimitiveType::Fixed(_) => PrimitiveLiteral::Binary(Vec::from(bytes)),
PrimitiveType::Binary => PrimitiveLiteral::Binary(Vec::from(bytes)),
PrimitiveType::Decimal {
precision: _,
scale: _,
} => todo!(),
PrimitiveType::Decimal { .. } => {
let unscaled_value = BigInt::from_signed_bytes_be(bytes);
PrimitiveLiteral::Int128(unscaled_value.to_i128().ok_or_else(|| {
Error::new(
ErrorKind::DataInvalid,
format!("Can't convert bytes to i128: {:?}", bytes),
)
})?)
}
};
Ok(Datum::new(data_type, literal))
}
Expand All @@ -449,7 +456,30 @@ impl Datum {
PrimitiveLiteral::String(val) => ByteBuf::from(val.as_bytes()),
PrimitiveLiteral::UInt128(val) => ByteBuf::from(val.to_be_bytes()),
PrimitiveLiteral::Binary(val) => ByteBuf::from(val.as_slice()),
PrimitiveLiteral::Int128(_) => todo!(),
PrimitiveLiteral::Int128(val) => {
let PrimitiveType::Decimal { precision, .. } = self.r#type else {
unreachable!(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we return error here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right @liurenjie1024 . I don't see a way that a malformed Datum could be created anywhere right now but this may not always be true as the project continues to evolve.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we will change Datum::to_bytes's return value into Result<ByteBuf>?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we will change Datum::to_bytes's return value into Result?

Looks good to me.

"PrimitiveLiteral Int128 must be PrimitiveType Decimal but got {}",
&self.r#type
)
};

// It's required by iceberg spec that we must keep the minimum
// number of bytes for the value
let required_bytes = Type::decimal_required_bytes(precision)
.expect("PrimitiveType must has valid precision")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

as usize;

// The primitive literal is unscaled value.
let unscaled_value = BigInt::from(*val);
// Convert into two's-complement byte representation of the BigInt
// in big-endian byte order.
let mut bytes = unscaled_value.to_signed_bytes_be();
// Truncate with required bytes to make sure.
bytes.truncate(required_bytes);

ByteBuf::from(bytes)
}
}
}

Expand Down Expand Up @@ -3031,6 +3061,31 @@ mod tests {
check_avro_bytes_serde(bytes, Datum::string("iceberg"), &PrimitiveType::String);
}

#[test]
fn avro_bytes_decimal() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we check add more tests for different precision?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

let bytes = vec![4u8, 210u8];

check_avro_bytes_serde(
bytes,
Datum::decimal(Decimal::new(1234, 2)).unwrap(),
&PrimitiveType::Decimal {
precision: 38,
scale: 2,
},
);

let bytes = vec![251u8, 46u8];

check_avro_bytes_serde(
bytes,
Datum::decimal(Decimal::new(-1234, 2)).unwrap(),
&PrimitiveType::Decimal {
precision: 38,
scale: 2,
},
);
}

#[test]
fn avro_convert_test_int() {
check_convert_with_avro(
Expand Down
Loading