shoemoney commented on code in PR #3256:
URL: https://github.com/apache/iceberg-rust/pull/3256#discussion_r4127334633
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -350,16 +350,35 @@ impl Serialize for PrimitiveType {
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}")))?;
+
+ let precision: u32 = precision.trim().parse().map_err(|_| malformed())?;
+ let scale: u32 = scale.trim().parse().map_err(|_| malformed())?;
Review Comment:
Fixed in 93a06c0aab3d0fd2bb04491c08381c6d7b08566b. The parser now checks
that each trimmed numeric token contains ASCII digits before parsing. Added
rejection cases for `decimal(+5, 2)`, `decimal(5, +2)`, `decimal(+5, +2)`, and
`fixed[+16]` through both `Type` and `PrimitiveType`.
The new test failed on the previous head and passes with the fix. All 1,761
library tests, the pinned formatting check, and strict all-target/all-feature
Clippy passed.
##########
crates/iceberg/src/spec/datatypes.rs:
##########
@@ -350,16 +350,35 @@ impl Serialize for PrimitiveType {
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}")))?;
+
+ let precision: u32 = precision.trim().parse().map_err(|_| malformed())?;
+ let scale: u32 = scale.trim().parse().map_err(|_| malformed())?;
+
+ if precision == 0 {
+ return Err(D::Error::custom(
+ "Decimal precision must be greater than zero",
+ ));
+ }
+ if precision > MAX_DECIMAL_PRECISION {
+ return Err(D::Error::custom(format!(
+ "Decimals with precision larger than {MAX_DECIMAL_PRECISION} are
not supported: {precision}"
+ )));
+ }
+ if scale > precision {
+ return Err(D::Error::custom(format!(
+ "Decimal scale {scale} must not be larger than precision
{precision}"
+ )));
+ }
Review Comment:
You're right: the `scale <= precision` statement in the description was too
broad. `arrow/schema.rs` applies Arrow's stricter validation, but
`Type::decimal` and the metadata parser did not share that restriction.
In 93a06c0aab3d0fd2bb04491c08381c6d7b08566b, the constructor and
deserializer share a private precision validator. I removed this PR's added
scale restriction to preserve existing constructor behavior, including `(5,
8)`, and corrected the description. Constructor-built roundtrip tests cover
`(1, 0)`, `(5, 5)`, `(5, 8)`, and `(38, 38)`, plus rejection of precision 0 and
39. The `(5, 8)` roundtrip failed on the previous head and now passes.
All 1,761 library tests, the pinned formatting check, and strict
all-target/all-feature Clippy passed. Tightening the public constructor's scale
policy can be considered separately.
--
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]
