mbutrovich commented on code in PR #3256:
URL: https://github.com/apache/iceberg-rust/pull/3256#discussion_r4126342776
##########
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:
`Type::decimal`
([`datatypes.rs#L193-L199`](https://github.com/apache/iceberg-rust/blob/86d6618804a902eac10f09c09a565aca5dad0d46/crates/iceberg/src/spec/datatypes.rs#L193-L199))
checks precision but not scale. That means `Type::decimal(5, 8)` succeeds and
serializes as `"decimal(5, 8)"`, which this function now rejects. On the head
commit, `serde_json::from_str::<Type>(&serde_json::to_string(&Type::decimal(5,
8).unwrap()).unwrap())` returns `data did not match any variant of untagged
enum SerdeType`. So a schema built through the public API can end up in table
metadata that iceberg-rust can't read back.
What do you think about moving these checks into one private function that
both `Type::decimal` and `deserialize_decimal` call? The two construction paths
couldn't disagree, and the precision message wouldn't be written twice. A test
that round-trips a `Type::decimal` result through JSON would cover it.
The description says the crate already requires `scale <= precision`
elsewhere. Could you point me to where? I couldn't find it outside this PR. The
implementations don't agree on the rule either. The spec only requires
precision to be 38 or less
([`spec.md#L273`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/format/spec.md?plain=1#L273)),
and Java's `DecimalType` checks only that
([`Types.java#L525-L533`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/types/Types.java#L525-L533)).
PyIceberg
([`types.py#L331-L347`](https://github.com/apache/iceberg-python/blob/ebbc0ba3e4c633179b4069a70a8f9a4453e75d88/pyiceberg/types.py#L331-L347)),
iceberg-go
([`types.go#L680-L694`](https://github.com/apache/iceberg-go/blob/8832cf696b5cc2a54767b5f2fa4c06b138c04495/types.go#L680-L694)),
and arrow-rs
([`types.rs#L1412-L1442`](https://github.com/apache/arrow-rs/blob/f90e061326bd821a7af09281d9e92de6
f3b603d9/arrow-array/src/types.rs#L1412-L1442)) all reject precision 0 and a
scale larger than the precision. With the stricter rule, iceberg-rust would
fail to load table metadata that Java accepts, the same way PyIceberg does
today. Whichever rule we pick, it would help to note the choice in the
description.
##########
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:
Should we also reject a leading `+` here? `u32::from_str` accepts it, so on
the head commit `"decimal(+5, +2)"` parses to `Decimal { precision: 5, scale: 2
}` and `"fixed[+16]"` (line 401) parses to `Fixed(16)`. The type-string
patterns in Java
([`Types.java#L66-L73`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/api/src/main/java/org/apache/iceberg/types/Types.java#L66-L73)),
iceberg-go
([`types.go#L36-L37`](https://github.com/apache/iceberg-go/blob/8832cf696b5cc2a54767b5f2fa4c06b138c04495/types.go#L36-L37)),
and PyIceberg
([`types.py#L60`](https://github.com/apache/iceberg-python/blob/ebbc0ba3e4c633179b4069a70a8f9a4453e75d88/pyiceberg/types.py#L60))
only allow digits there. If we reject it, could both strings go into
`test_reject_malformed_decimal_and_fixed_type_strings`?
--
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]
