Copilot commented on code in PR #209:
URL: https://github.com/apache/datasketches-rust/pull/209#discussion_r3841179197
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -762,54 +795,53 @@ impl TDigestMut {
/// # Contract
///
/// * `buffer` must have at least one centroid.
- /// * `buffer` is generated from `self.buffer`, and thus:
- /// * No `NAN` values are present in `buffer`.
- /// * We should clear `self.buffer` after merging.
+ /// * `buffer` contains all existing centroids and values from
`self.buffer`.
+ /// * No `NAN` values are present in `buffer`.
+ /// * We should clear `self.buffer` after merging.
Review Comment:
`do_merge`’s contract comment says the `buffer` contains "all existing
centroids and values from `self.buffer`", but `merge()` also passes in the
other digest’s buffered values and centroids. This makes the contract
misleading for future callers/maintainers; consider wording it as “all
centroids/values to be merged (including self’s existing centroids and any
buffered values from both digests)” or similar.
##########
datasketches/src/tdigest/sketch.rs:
##########
@@ -762,54 +795,53 @@ impl TDigestMut {
/// # Contract
///
/// * `buffer` must have at least one centroid.
- /// * `buffer` is generated from `self.buffer`, and thus:
- /// * No `NAN` values are present in `buffer`.
- /// * We should clear `self.buffer` after merging.
+ /// * `buffer` contains all existing centroids and values from
`self.buffer`.
+ /// * No `NAN` values are present in `buffer`.
+ /// * We should clear `self.buffer` after merging.
fn do_merge(&mut self, mut buffer: Vec<Centroid>, weight: u64) {
- buffer.extend(std::mem::take(&mut self.centroids));
buffer.sort_by(centroid_cmp);
if self.reverse_merge {
buffer.reverse();
}
self.centroids_weight += weight;
- let mut num_centroids = 0;
+ let mut num_centroids = 1;
let len = buffer.len();
- self.centroids.push(buffer[0]);
- num_centroids += 1;
+ let centroids_weight = self.centroids_weight as f64;
+ let normalizer = scale_function::normalizer((2 * self.k) as f64,
centroids_weight);
let mut current = 1;
Review Comment:
`scale_function::normalizer((2 * self.k) as f64, …)` multiplies two `u16`
values before casting, so `k > 32767` will overflow in debug builds (panic) and
wrap in release builds, producing an incorrect compression value. To avoid
overflow, cast before multiplying (e.g., `2.0 * self.k as f64` or `(2u32 *
self.k as u32) as f64`).
--
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]
