Copilot commented on code in PR #173:
URL: https://github.com/apache/datasketches-rust/pull/173#discussion_r3672152814
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -1327,6 +1336,23 @@ fn check_nonzero(value: u64, tag: &'static str) ->
Result<NonZeroU64, Error> {
.ok_or_else(|| Error::deserial(format!("malformed data: {tag} cannot
be zero")))
}
+fn check_compat_weight(value: f64, tag: &'static str) -> Result<NonZeroU64,
Error> {
+ check_non_nan(value, tag)?;
+ check_finite(value, tag)?;
+ if !(1.0..u64::MAX as f64).contains(&value) {
+ return Err(Error::deserial(format!(
+ "malformed data: {tag} must be representable as a positive u64"
+ )));
+ }
+ check_nonzero(value as u64, tag)
+}
Review Comment:
`check_compat_weight` currently accepts fractional weights (e.g., `1.5`),
then silently truncates via `value as u64` and treats it as valid. That
contradicts the intent of “representable as a positive u64” and can turn
malformed serialized inputs into a different (but accepted) state. Consider
additionally validating that the value is an integer-valued float (e.g.,
`value.trunc() == value` / `value.fract() == 0.0`) before casting.
##########
datasketches/tests/serde_tests/tdigest.rs:
##########
@@ -186,3 +186,24 @@ fn test_many_values() {
assert_eq!(td.rank(500.0), deserialized_td.rank(500.0));
assert_eq!(td.quantile(0.5), deserialized_td.quantile(0.5));
}
+
+#[test]
+fn test_large_weights_produce_finite_extreme_quantile() {
+ let lower = f64::from_bits(f64::MAX.to_bits() - 1);
+ let mut tdigest = TDigestMut::default();
+ tdigest.update(lower);
+ tdigest.update(f64::MAX);
+ let mut bytes = tdigest.serialize();
+
+ // These valid weights retain a positive lower contribution even though
its normalized ratio
+ // rounds away when interpolating between the two extreme values.
+ bytes[40..48].copy_from_slice(&((1_u64 << 52) - 1).to_le_bytes());
+ bytes[56..64].copy_from_slice(&(1_u64 << 52).to_le_bytes());
+
Review Comment:
This test directly patches serialized bytes using hardcoded offsets
(`40..48`, `56..64`), which is brittle (layout changes will panic with an
out-of-bounds slice and give a less actionable failure). Consider centralizing
these offsets (named constants with a short comment tying them to the
serialization layout), or asserting the expected minimum length/format before
indexing so failures are clearer.
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -1327,6 +1336,23 @@ fn check_nonzero(value: u64, tag: &'static str) ->
Result<NonZeroU64, Error> {
.ok_or_else(|| Error::deserial(format!("malformed data: {tag} cannot
be zero")))
}
+fn check_compat_weight(value: f64, tag: &'static str) -> Result<NonZeroU64,
Error> {
+ check_non_nan(value, tag)?;
+ check_finite(value, tag)?;
+ if !(1.0..u64::MAX as f64).contains(&value) {
+ return Err(Error::deserial(format!(
+ "malformed data: {tag} must be representable as a positive u64"
+ )));
+ }
+ check_nonzero(value as u64, tag)
+}
+
+fn checked_weight_sum(total_weight: u64, weight: u64) -> Result<u64, Error> {
+ total_weight
+ .checked_add(weight)
+ .ok_or_else(|| Error::deserial("malformed data: total weight
overflow"))
+}
Review Comment:
The new overflow error message is generic (“total weight overflow”) and
loses context about which accumulation overflowed (native centroid sum,
buffered values, compat centroid sum, etc.). Consider including a tag/source in
the message (e.g., pass a `tag` into `checked_weight_sum` or wrap errors at
call sites) to make malformed-input debugging more actionable.
--
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]
