shoemoney commented on code in PR #3256:
URL: https://github.com/apache/iceberg-rust/pull/3256#discussion_r4153498688
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -347,19 +344,39 @@ impl Serialize for PrimitiveType {
}
}
+fn validate_decimal_precision(precision: u32) -> Result<()> {
+ ensure_data_valid!(precision > 0, "Decimal precision must be greater than
zero",);
+ ensure_data_valid!(
+ precision <= MAX_DECIMAL_PRECISION,
+ "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not
supported: {precision}",
+ );
+ Ok(())
+}
+
fn deserialize_decimal<'de, D>(deserializer: D) ->
std::result::Result<PrimitiveType, D::Error>
where D: Deserializer<'de> {
let s = String::deserialize(deserializer)?;
+ let malformed = || D::Error::custom(format!("Invalid decimal type: {s}"));
+
let (precision, scale) = s
- .trim_start_matches(r"decimal(")
- .trim_end_matches(')')
+ .strip_prefix("decimal(")
+ .and_then(|inner| inner.strip_suffix(')'))
+ .ok_or_else(malformed)?
.split_once(',')
- .ok_or_else(|| D::Error::custom("Decimal requires precision and scale:
{s}"))?;
+ .ok_or_else(|| D::Error::custom(format!("Decimal requires precision
and scale: {s}")))?;
- Ok(PrimitiveType::Decimal {
- precision: precision.trim().parse().map_err(D::Error::custom)?,
- scale: scale.trim().parse().map_err(D::Error::custom)?,
- })
+ let (precision, scale) = (precision.trim(), scale.trim());
+ if ![precision, scale]
+ .iter()
+ .all(|token| token.bytes().all(|byte| byte.is_ascii_digit()))
Review Comment:
Added explicit nonempty-token checks in
0c6d0d5fd13057ae8b86b10d1f479f1c1e8093ff. The rejection tests now cover
decimal(, 2), decimal(5,), whitespace-only tokens and fixed[], through both
Type and PrimitiveType. All 1,763 library tests, formatting and strict
all-target/all-feature Clippy passed on the pinned nightly.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -347,19 +344,39 @@ impl Serialize for PrimitiveType {
}
}
+fn validate_decimal_precision(precision: u32) -> Result<()> {
+ ensure_data_valid!(precision > 0, "Decimal precision must be greater than
zero",);
+ ensure_data_valid!(
+ precision <= MAX_DECIMAL_PRECISION,
+ "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are not
supported: {precision}",
+ );
+ Ok(())
+}
+
fn deserialize_decimal<'de, D>(deserializer: D) ->
std::result::Result<PrimitiveType, D::Error>
where D: Deserializer<'de> {
let s = String::deserialize(deserializer)?;
+ let malformed = || D::Error::custom(format!("Invalid decimal type: {s}"));
+
let (precision, scale) = s
- .trim_start_matches(r"decimal(")
- .trim_end_matches(')')
+ .strip_prefix("decimal(")
+ .and_then(|inner| inner.strip_suffix(')'))
+ .ok_or_else(malformed)?
.split_once(',')
- .ok_or_else(|| D::Error::custom("Decimal requires precision and scale:
{s}"))?;
+ .ok_or_else(|| D::Error::custom(format!("Decimal requires precision
and scale: {s}")))?;
- Ok(PrimitiveType::Decimal {
- precision: precision.trim().parse().map_err(D::Error::custom)?,
- scale: scale.trim().parse().map_err(D::Error::custom)?,
- })
+ let (precision, scale) = (precision.trim(), scale.trim());
+ if ![precision, scale]
+ .iter()
+ .all(|token| token.bytes().all(|byte| byte.is_ascii_digit()))
+ {
+ return Err(malformed());
+ }
+ let precision: u32 = precision.parse().map_err(|_| malformed())?;
Review Comment:
Restored map_err(D::Error::custom) for decimal precision, decimal scale and
fixed length in 0c6d0d5fd13057ae8b86b10d1f479f1c1e8093ff, so overflow retains
the ParseIntError diagnostic. The new regression failed on the previous head
and now passes for overflowing u32 precision/scale and u64 fixed length.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1326,6 +1343,135 @@ mod tests {
assert_eq!(16, Type::decimal_required_bytes(38).unwrap());
}
+ #[test]
+ fn test_reject_malformed_decimal_and_fixed_type_strings() {
+ for invalid in [
+ r#""decimal(50, 2)""#,
+ r#""decimal(0, 0)""#,
+ r#""decimal(decimal(5, 2)))""#,
+ r#""decimal(decimal(5, 2)""#,
+ r#""decimal(5, 2""#,
+ r#""decimal(5, 2)))))""#,
+ r#""decimal(5, 2, 3)""#,
+ r#""decimal(-5, 2)""#,
+ r#""decimal(5, -2)""#,
+ r#""decimal(-5, -2)""#,
+ r#""decimal(-2, -5)""#,
+ r#""decimal((5, 2))""#,
+ r#""decimal[5, 2]""#,
+ r#""decimal()""#,
+ r#""fixed[fixed[16]]]""#,
+ r#""fixed[16""#,
+ r#""fixed[16]]]""#,
+ r#""fixed[[16]]""#,
+ r#""fixed[[16]""#,
+ r#""fixed(16)""#,
+ r#""fixed[]""#,
+ ] {
+ assert!(
+ serde_json::from_str::<Type>(invalid).is_err(),
+ "expected {invalid} to be rejected"
+ );
+ }
+ }
+
+ #[test]
+ fn test_reject_leading_plus_in_type_strings() {
+ for invalid in [
+ r#""decimal(+5, 2)""#,
+ r#""decimal(5, +2)""#,
+ r#""decimal(+5, +2)""#,
+ r#""fixed[+16]""#,
+ ] {
+ assert!(serde_json::from_str::<Type>(invalid).is_err(),
"{invalid}");
+ assert!(
+ serde_json::from_str::<PrimitiveType>(invalid).is_err(),
+ "{invalid}"
+ );
+ }
+ }
+
+ #[test]
+ fn test_decimal_constructor_json_roundtrip() {
+ for (precision, scale) in [(1, 0), (5, 5), (5, 8), (38, 38)] {
Review Comment:
Added the comment beside validate_decimal_precision in
0c6d0d5fd13057ae8b86b10d1f479f1c1e8093ff. Scale remains deliberately
unconstrained for Java parity; the existing (5, 8) constructor/JSON roundtrip
still passes.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -1326,6 +1343,135 @@ mod tests {
assert_eq!(16, Type::decimal_required_bytes(38).unwrap());
}
+ #[test]
+ fn test_reject_malformed_decimal_and_fixed_type_strings() {
+ for invalid in [
+ r#""decimal(50, 2)""#,
+ r#""decimal(0, 0)""#,
+ r#""decimal(decimal(5, 2)))""#,
+ r#""decimal(decimal(5, 2)""#,
+ r#""decimal(5, 2""#,
+ r#""decimal(5, 2)))))""#,
+ r#""decimal(5, 2, 3)""#,
+ r#""decimal(-5, 2)""#,
+ r#""decimal(5, -2)""#,
+ r#""decimal(-5, -2)""#,
+ r#""decimal(-2, -5)""#,
+ r#""decimal((5, 2))""#,
+ r#""decimal[5, 2]""#,
+ r#""decimal()""#,
+ r#""fixed[fixed[16]]]""#,
+ r#""fixed[16""#,
+ r#""fixed[16]]]""#,
+ r#""fixed[[16]]""#,
+ r#""fixed[[16]""#,
+ r#""fixed(16)""#,
+ r#""fixed[]""#,
+ ] {
+ assert!(
+ serde_json::from_str::<Type>(invalid).is_err(),
+ "expected {invalid} to be rejected"
+ );
+ }
+ }
+
+ #[test]
+ fn test_reject_leading_plus_in_type_strings() {
+ for invalid in [
+ r#""decimal(+5, 2)""#,
+ r#""decimal(5, +2)""#,
+ r#""decimal(+5, +2)""#,
+ r#""fixed[+16]""#,
+ ] {
+ assert!(serde_json::from_str::<Type>(invalid).is_err(),
"{invalid}");
+ assert!(
+ serde_json::from_str::<PrimitiveType>(invalid).is_err(),
+ "{invalid}"
+ );
+ }
+ }
+
+ #[test]
+ fn test_decimal_constructor_json_roundtrip() {
+ for (precision, scale) in [(1, 0), (5, 5), (5, 8), (38, 38)] {
+ let decimal = Type::decimal(precision, scale).unwrap();
+ let serialized = serde_json::to_string(&decimal).unwrap();
+ let reparsed: Type = serde_json::from_str(&serialized).unwrap();
+ assert_eq!(reparsed, decimal);
+ let primitive: PrimitiveType =
serde_json::from_str(&serialized).unwrap();
+ assert_eq!(Type::Primitive(primitive), decimal);
+ }
+ for precision in [0, 39] {
+ assert!(Type::decimal(precision, 0).is_err());
+ let json = format!(r#""decimal({precision}, 0)""#);
+ assert!(serde_json::from_str::<Type>(&json).is_err());
+ assert!(serde_json::from_str::<PrimitiveType>(&json).is_err());
+ }
+ }
+
+ #[test]
+ fn test_decimal_zero_precision_error() {
+ let error = serde_json::from_str::<PrimitiveType>(r#""decimal(0,
0)""#).unwrap_err();
+ assert!(
+ error
+ .to_string()
+ .contains("Decimal precision must be greater than zero"),
+ "unexpected error: {error}"
+ );
+ }
+
+ #[test]
+ fn test_accept_valid_decimal_and_fixed_type_strings() {
+ for (json, expected) in [
+ (
+ r#""decimal(9, 2)""#,
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 9,
+ scale: 2,
+ }),
+ ),
+ (
+ r#""decimal(38, 10)""#,
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 38,
+ scale: 10,
+ }),
+ ),
+ (
+ r#""decimal(5,2)""#,
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 5,
+ scale: 2,
+ }),
+ ),
+ (
+ r#""decimal(5, 0)""#,
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 5,
+ scale: 0,
+ }),
+ ),
+ (
+ r#""decimal(5, 5)""#,
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 5,
+ scale: 5,
+ }),
+ ),
+ (r#""fixed[16]""#, Type::Primitive(PrimitiveType::Fixed(16))),
+ ] {
+ check_type_serde_roundtrip_value(json, expected);
+ }
+ }
+
+ fn check_type_serde_roundtrip_value(json: &str, expected_type: Type) {
Review Comment:
Strengthened the test in 0c6d0d5fd13057ae8b86b10d1f479f1c1e8093ff to compare
serialization of the parsed value with explicit canonical JSON, through both
Type and PrimitiveType. The compact decimal(5,2) input must serialize as
decimal(5, 2), and the canonical output is reparsed as well.
--
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]
