Copilot commented on code in PR #284:
URL: https://github.com/apache/datasketches-rust/pull/284#discussion_r4171920041
##########
datasketches/src/kll/sorted_view.rs:
##########
@@ -100,6 +102,11 @@ impl<T: Clone + Ord> SortedView<T> {
)));
}
+ // Large stream weights can round down when converted to f64.
+ if rank == 1.0 {
+ return Ok(self.entries[self.entries.len() - 1].item.clone());
Review Comment:
Returning the last retained entry does not preserve KLL's upper endpoint
when compaction has discarded the stream maximum. `KllSketch` tracks the exact
`max_item`, and Apache KLL rank-`1.0` queries are defined to return that exact
maximum; this test misses the case because it inserts `1` only after all
compactions. Carry the tracked extrema into `SortedView`, use the maximum here,
and cover a maximum observed before compaction.
##########
CHANGELOG.md:
##########
@@ -29,13 +29,15 @@ All significant changes to this project will be documented
in this file.
### Bug fixes
+* KLL quantile queries at rank `1.0` now return the largest retained item even
when the stream weight exceeds `2^53`.
Review Comment:
The same large-weight rank-`1.0` correction is added and tested for
`ReqSketch`, but this changelog entry names only KLL. The repository changelog
guidance requires each user-visible behavior to name the affected public APIs,
so include both sketches here.
##########
datasketches/src/req/sorted_view.rs:
##########
@@ -148,43 +121,26 @@ where
)));
}
- // Handle edge cases
- if rank == 0.0 {
- match criteria {
- SearchCriteria::Inclusive => return Ok(self.items[0].clone()),
- SearchCriteria::Exclusive => return Ok(self.items[0].clone()),
- }
- }
+ // Large stream weights can round down when converted to f64.
if rank == 1.0 {
return Ok(self.items[self.items.len() - 1].clone());
Review Comment:
This returns the largest retained item, but low-rank REQ compaction can
compact the high tail and discard the stream maximum even though `ReqSketch`
tracks it separately in `max_item`. Rank `1.0` is an exact maximum endpoint in
Apache REQ semantics; the new test masks this because it appends `1` after
every compaction. Pass the tracked extrema into the sorted view and add a
regression where the maximum is observed before compaction.
--
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]
