laskoviymishka commented on code in PR #3256:
URL: https://github.com/apache/iceberg-rust/pull/3256#discussion_r4153248498
##########
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:
`[precision, scale].iter().all(|t| t.bytes().all(is_ascii_digit))` is
vacuously true on an empty token, so `decimal(, 2)` and `decimal(5,)` slip past
this guard and only get rejected downstream by `.parse()` — same for `fixed[]`
via the `.filter(...)` in `deserialize_fixed`. The observable behavior is
already correct (and matches the `decimal()` / `fixed[]` reject cases in
apache/iceberg-verification#9), so this is clarity rather than a bug — but the
guard isn't what's doing the work. I'd fold in the emptiness check and flip the
negation to `any` while we're here:
```rust
if [precision, scale]
.iter()
.any(|token| token.is_empty() || !token.bytes().all(|b|
b.is_ascii_digit()))
{
return Err(malformed());
}
```
Worth adding `decimal(, 2)` / `decimal(5,)` to the rejection test so the
guard path is what's exercised, not the parse fallback.
##########
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:
Once a token clears the digit filter, the only thing `.parse::<u32>()` can
still reject is overflow (e.g. `decimal(9999999999, 2)`), and `map_err(|_|
malformed())` collapses that into a generic `Invalid decimal type` — the old
code forwarded the real `ParseIntError`. Minor, but I'd keep
`map_err(D::Error::custom)` on the parse so overflow still says what it is.
Same story with `deserialize_fixed`'s `.parse().ok()`.
##########
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:
This checks `parse(json) == expected` and `parse(serialize(expected)) ==
expected`, but never that `serialize(parse(json))` matches the input — so
`decimal(5,2)` (no space) doesn't actually round-trip through the string form.
That canonical write direction is exactly what the conformance suite makes
normative: apache/iceberg-verification#9 pins `decimal(9,2)` re-serializing to
the spaced `decimal(9, 2)` byte-for-byte. So asserting `to_string(parse(json))`
equals the canonical form here isn't just naming hygiene — it's the check that
proves the write direction. I'd either reuse the existing `check_type_serde`
for the canonical cases or add that assertion (and drop `roundtrip` from the
name if you keep it weak).
##########
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:
The `(5, 8)` case pins `scale > precision` as supported, and the
deserializer has no `scale <= precision` guard. Worth knowing this is a
genuinely unpinned corner: the cross-client conformance suite explicitly leaves
`scale > precision` "out on purpose" (apache/iceberg-verification#9,
`table-spec/types/README.md`) because the spec neither permits nor forbids it.
So accepting it isn't non-conformant — but a Rust- or Java-written `decimal(5,
8)` can still fail to load in iceberg-go (`validateDecimalPrecisionScale`) and
PyIceberg (`check_scale`), and no conformance run will catch that.
Since this test makes the choice documented behavior, I'd add a one-line
comment on `validate_decimal_precision` noting scale is deliberately
unconstrained for Java parity (and that the spec leaves it open). I would not
add a `scale <= precision` check — that'd diverge from both Java and the
suite's deliberate non-decision. Just want the acceptance to be on purpose.
--
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]
