From 5bda67578537976ff77897d2394d895325baa133 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 15:48:09 +0800 Subject: [PATCH 1/6] fix(kll): distinguish empty queries from invalid arguments Return None for valid queries on an empty sketch or sorted view, and validate ranks and split points before checking emptiness. Document the query contract and cover invalid inputs, empty arguments, and endpoints. Co-authored-by: Jiawei Zhao --- CHANGELOG.md | 2 +- datasketches/src/error.rs | 24 +++ datasketches/src/kll/mod.rs | 14 +- datasketches/src/kll/sketch.rs | 68 +++++---- datasketches/src/kll/sorted_view.rs | 93 ++++++++---- tests-integration/tests/kll_test/generic.rs | 12 +- tests-integration/tests/kll_test/merge.rs | 1 + tests-integration/tests/kll_test/query.rs | 157 +++++++++++++++----- 8 files changed, 268 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cfc3dfd..86f55dd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ All significant changes to this project will be documented in this file. ### New features * Add `HllSketch::composite_estimate` and `HllUnion::composite_estimate` to query cardinality without relying on HIP update history, matching Java's `getCompositeEstimate()`. -* `KllSketch` is now available behind the `kll` feature, with rank, quantile, PMF, and CDF queries, merging, serialization, custom ordered item types, and a `KllFloat` adapter for non-NaN floating-point values. +* `KllSketch` is now available behind the `kll` feature, with rank, quantile, PMF, and CDF queries, merging, serialization, custom ordered item types, and a `KllFloat` adapter for non-NaN floating-point values. Queries distinguish an empty sketch (`None`) from invalid arguments (`InvalidArgument`). * Add `TDigestMut::quantiles` and `TDigest::quantiles` for querying several ranks in one centroid scan while preserving input order. * Add `FromIterator` as an alternative to incremental `TDigestMut::merge` calls for combining owned partial sketches in a single compression pass. diff --git a/datasketches/src/error.rs b/datasketches/src/error.rs index 110b8c31..801f902d 100644 --- a/datasketches/src/error.rs +++ b/datasketches/src/error.rs @@ -16,6 +16,30 @@ // under the License. //! Error types for DataSketches operations. +//! +//! # Query results +//! +//! An empty sketch is valid. Queries with no meaningful result use `Option` to represent +//! absence; invalid caller arguments use `Result::Err`. A query that can encounter both returns +//! `Result, Error>`: `Ok(None)` means no result, not an invalid request. +//! +//! The following contract applies to KLL sketches and their sorted views: +//! +//! | Query | Valid input, non-empty | Valid input, empty | Invalid input, any state | +//! | --- | --- | --- | --- | +//! | `min_item`, `max_item` | `Some(item)` | `None` | No arguments | +//! | `rank` | `Some(rank)` | `None` | Items implement `Ord` | +//! | `quantile`, `quantiles`, `cdf`, `pmf` | `Ok(Some(result))` | `Ok(None)` | `Err(InvalidArgument)` | +//! +//! - **Validation comes first:** ranks must be in `[0.0, 1.0]` and not NaN; split points must be +//! strictly increasing. +//! - **Empty argument slices are valid:** on a non-empty sketch, `quantiles(&[])` returns +//! `Ok(Some(vec![]))`, and `cdf(&[])` / `pmf(&[])` return `Ok(Some(vec![1.0]))`. On an empty +//! sketch, all return `Ok(None)`. +//! - **Caller migration:** handle `rank` as an `Option`; for fallible queries, use `?` to propagate +//! invalid arguments, then handle the `Option` for an empty sketch. +//! - **Errors remain distinct:** malformed serialized data returns [`ErrorKind::InvalidData`]; +//! broken internal invariants remain programming errors. use std::fmt; diff --git a/datasketches/src/kll/mod.rs b/datasketches/src/kll/mod.rs index 469bc7f1..8cf77c9c 100644 --- a/datasketches/src/kll/mod.rs +++ b/datasketches/src/kll/mod.rs @@ -28,16 +28,26 @@ //! provides their ordinary numerical order. Custom ordering should be expressed with a newtype //! that implements [`Ord`], keeping the ordering semantics part of the item type. //! +//! # Query results +//! +//! Valid queries on an empty sketch return no result. Invalid ranks or split points return +//! [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument), even on an empty +//! sketch. See the [query contract](crate::error#query-results) for return +//! types and migration guidance. +//! //! # Usage //! //! ```rust //! # use datasketches::common::SearchCriteria; //! # use datasketches::kll::KllSketch; -//! let mut sketch = KllSketch::::new(200).unwrap(); +//! let mut sketch = KllSketch::::new(200)?; //! sketch.update(1); //! sketch.update(2); -//! let q = sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(); +//! let Some(q) = sketch.quantile(0.5, SearchCriteria::Inclusive)? else { +//! return Ok(()); // No samples to query. +//! }; //! assert!((1..=2).contains(&q)); +//! # Ok::<(), datasketches::error::Error>(()) //! ``` mod capacity; diff --git a/datasketches/src/kll/sketch.rs b/datasketches/src/kll/sketch.rs index 9fb3a62a..13972db5 100644 --- a/datasketches/src/kll/sketch.rs +++ b/datasketches/src/kll/sketch.rs @@ -122,12 +122,12 @@ impl KllSketch { self.levels.len() > 1 } - /// Returns the minimum item seen by the sketch. + /// Returns the minimum item seen by the sketch, or `None` if it is empty. pub fn min_item(&self) -> Option<&T> { self.min_item.as_ref() } - /// Returns the maximum item seen by the sketch. + /// Returns the maximum item seen by the sketch, or `None` if it is empty. pub fn max_item(&self) -> Option<&T> { self.max_item.as_ref() } @@ -202,12 +202,10 @@ impl KllSketch { /// Returns the normalized rank of the given item. /// - /// # Errors - /// - /// Returns an error if the sketch is empty. - pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Result { + /// Returns `None` if the sketch is empty. + pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Option { if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty sketch")); + return None; } let inclusive = criteria == SearchCriteria::Inclusive; let mut weight = 0u64; @@ -222,30 +220,31 @@ impl KllSketch { .count() as u64; weight += count << level; } - Ok(weight as f64 / self.n as f64) + Some(weight as f64 / self.n as f64) } /// Returns the quantile for the given normalized rank. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the sketch is empty. /// /// # Errors /// - /// Returns an error if the sketch is empty or `rank` is outside `[0.0, 1.0]`. - pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result { - if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty sketch")); - } + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( "rank must be in [0.0, 1.0], got {rank}" ))); } + if self.is_empty() { + return Ok(None); + } if rank == 0.0 { - return Ok(self.min_item.as_ref().unwrap().clone()); + return Ok(Some(self.min_item.as_ref().unwrap().clone())); } if rank == 1.0 { - return Ok(self.max_item.as_ref().unwrap().clone()); + return Ok(Some(self.max_item.as_ref().unwrap().clone())); } self.sorted_view().quantile(rank, criteria) } @@ -253,39 +252,50 @@ impl KllSketch { /// Returns approximate quantiles for the given normalized ranks. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the sketch is empty. /// /// The sorted view is built once for the whole batch. /// /// # Errors /// - /// Returns an error if the sketch is empty or any rank is outside `[0.0, 1.0]`. - pub fn quantiles(&self, ranks: &[f64], criteria: SearchCriteria) -> Result, Error> { + /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + pub fn quantiles( + &self, + ranks: &[f64], + criteria: SearchCriteria, + ) -> Result>, Error> { self.sorted_view().quantiles(ranks, criteria) } /// Returns the approximate CDF for the given split points. /// + /// Returns `Ok(None)` if the sketch is empty. + /// /// # Errors /// - /// Returns an error if the sketch is empty or the split points are not unique and strictly - /// increasing. - pub fn cdf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty sketch")); - } + /// Returns an error if the split points are not strictly increasing, even if the sketch is + /// empty. + pub fn cdf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { self.sorted_view().cdf(split_points, criteria) } /// Returns the approximate PMF for the given split points. /// + /// Returns `Ok(None)` if the sketch is empty. + /// /// # Errors /// - /// Returns an error if the sketch is empty or the split points are not unique and strictly - /// increasing. - pub fn pmf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty sketch")); - } + /// Returns an error if the split points are not strictly increasing, even if the sketch is + /// empty. + pub fn pmf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { self.sorted_view().pmf(split_points, criteria) } diff --git a/datasketches/src/kll/sorted_view.rs b/datasketches/src/kll/sorted_view.rs index d11f45be..482325bc 100644 --- a/datasketches/src/kll/sorted_view.rs +++ b/datasketches/src/kll/sorted_view.rs @@ -70,12 +70,10 @@ impl SortedView { /// Returns the approximate normalized rank of `item`. /// - /// # Errors - /// - /// Returns an error if the view is empty. - pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Result { + /// Returns `None` if the view is empty. + pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Option { if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty view")); + return None; } let index = if criteria == SearchCriteria::Inclusive { upper_bound(&self.entries, item) @@ -84,34 +82,35 @@ impl SortedView { }; if index == 0 { - return Ok(0.0); + return Some(0.0); } - Ok(self.entries[index - 1].cumulative_weight as f64 / self.total_weight as f64) + Some(self.entries[index - 1].cumulative_weight as f64 / self.total_weight as f64) } /// Returns the approximate quantile for `rank`. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the view is empty. /// /// # Errors /// - /// Returns an error if the view is empty or `rank` is outside `[0.0, 1.0]`. - pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result { - if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty view")); - } + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is empty. + pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( "rank must be in [0.0, 1.0], got {rank}" ))); } + if self.is_empty() { + return Ok(None); + } // Compaction may discard extrema, and large weights can lose precision in f64. if rank == 0.0 { - return Ok(self.min_item.as_ref().unwrap().clone()); + return Ok(Some(self.min_item.as_ref().unwrap().clone())); } if rank == 1.0 { - return Ok(self.max_item.as_ref().unwrap().clone()); + return Ok(Some(self.max_item.as_ref().unwrap().clone())); } let weight = if criteria == SearchCriteria::Inclusive { @@ -125,55 +124,87 @@ impl SortedView { upper_bound_by_weight(&self.entries, weight) }; - Ok(self.entries[index.min(self.entries.len() - 1)].item.clone()) + Ok(Some( + self.entries[index.min(self.entries.len() - 1)].item.clone(), + )) } /// Returns approximate quantiles for all `ranks`. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the view is empty. /// /// # Errors /// - /// Returns an error if the view is empty or any rank is outside `[0.0, 1.0]`. - pub fn quantiles(&self, ranks: &[f64], criteria: SearchCriteria) -> Result, Error> { + /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the view is empty. + pub fn quantiles( + &self, + ranks: &[f64], + criteria: SearchCriteria, + ) -> Result>, Error> { + for &rank in ranks { + if !(0.0..=1.0).contains(&rank) { + return Err(Error::invalid_argument(format!( + "rank must be in [0.0, 1.0], got {rank}" + ))); + } + } if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty view")); + return Ok(None); } - ranks + let quantiles = ranks .iter() - .map(|&rank| self.quantile(rank, criteria)) - .collect() + .map(|&rank| { + self.quantile(rank, criteria) + .map(|quantile| quantile.expect("checked non-empty view")) + }) + .collect::>()?; + Ok(Some(quantiles)) } /// Returns the approximate cumulative distribution over `split_points`. /// + /// Returns `Ok(None)` if the view is empty. + /// /// # Errors /// - /// Returns an error if the view is empty or the split points are invalid. - pub fn cdf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { + /// Returns an error if the split points are not strictly increasing, even if the view is empty. + pub fn cdf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { + check_split_points(split_points)?; if self.is_empty() { - return Err(Error::invalid_argument("cannot query an empty view")); + return Ok(None); } - check_split_points(split_points)?; let mut ranks = Vec::with_capacity(split_points.len() + 1); for item in split_points { - ranks.push(self.rank(item, criteria)?); + ranks.push(self.rank(item, criteria).expect("checked non-empty view")); } ranks.push(1.0); - Ok(ranks) + Ok(Some(ranks)) } /// Returns the approximate probability mass over `split_points`. /// + /// Returns `Ok(None)` if the view is empty. + /// /// # Errors /// - /// Returns an error if the view is empty or the split points are invalid. - pub fn pmf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - let mut buckets = self.cdf(split_points, criteria)?; + /// Returns an error if the split points are not strictly increasing, even if the view is empty. + pub fn pmf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { + let Some(mut buckets) = self.cdf(split_points, criteria)? else { + return Ok(None); + }; for index in (1..buckets.len()).rev() { buckets[index] -= buckets[index - 1]; } - Ok(buckets) + Ok(Some(buckets)) } } diff --git a/tests-integration/tests/kll_test/generic.rs b/tests-integration/tests/kll_test/generic.rs index 14d06677..544f6798 100644 --- a/tests-integration/tests/kll_test/generic.rs +++ b/tests-integration/tests/kll_test/generic.rs @@ -68,7 +68,11 @@ fn custom_item_order_controls_queries_and_survives_roundtrip() { assert_eq!(sketch.min_item().map(|item| item.0.as_str()), Some("1")); assert_eq!(sketch.max_item().map(|item| item.0.as_str()), Some("10")); assert_eq!( - sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap().0, + sketch + .quantile(0.5, SearchCriteria::Inclusive) + .unwrap() + .unwrap() + .0, "2" ); @@ -78,7 +82,11 @@ fn custom_item_order_controls_queries_and_survives_roundtrip() { assert_eq!(decoded.min_item().map(|item| item.0.as_str()), Some("1")); assert_eq!(decoded.max_item().map(|item| item.0.as_str()), Some("10")); assert_eq!( - decoded.quantile(0.5, SearchCriteria::Inclusive).unwrap().0, + decoded + .quantile(0.5, SearchCriteria::Inclusive) + .unwrap() + .unwrap() + .0, "2" ); } diff --git a/tests-integration/tests/kll_test/merge.rs b/tests-integration/tests/kll_test/merge.rs index 417fa1b1..513f0dbc 100644 --- a/tests-integration/tests/kll_test/merge.rs +++ b/tests-integration/tests/kll_test/merge.rs @@ -35,6 +35,7 @@ fn merge_preserves_weight_extrema_and_query_invariants() { assert_eq!(left.sorted_view().total_weight(), left.n()); let quantiles = left .quantiles(&[0.0, 0.25, 0.5, 0.75, 1.0], SearchCriteria::Inclusive) + .unwrap() .unwrap(); assert!(quantiles.windows(2).all(|pair| pair[0] <= pair[1])); } diff --git a/tests-integration/tests/kll_test/query.rs b/tests-integration/tests/kll_test/query.rs index 3730224a..aa303e36 100644 --- a/tests-integration/tests/kll_test/query.rs +++ b/tests-integration/tests/kll_test/query.rs @@ -35,18 +35,24 @@ fn quantile_endpoints_preserve_extrema_after_compaction() { let view = sketch.sorted_view(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert_eq!(sketch.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(sketch.quantile(1.0, criteria).unwrap(), 7); - assert_eq!(sketch.quantiles(&[0.0, 1.0], criteria).unwrap(), [0, 7]); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), Some(7)); + assert_eq!( + sketch.quantiles(&[0.0, 1.0], criteria).unwrap(), + Some(vec![0, 7]) + ); } sketch.reset(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert!(sketch.quantile(0.0, criteria).is_err()); - assert!(sketch.quantile(1.0, criteria).is_err()); - assert_eq!(view.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(view.quantile(1.0, criteria).unwrap(), 7); - assert_eq!(view.quantiles(&[0.0, 1.0], criteria).unwrap(), [0, 7]); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), None); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), None); + assert_eq!(view.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(view.quantile(1.0, criteria).unwrap(), Some(7)); + assert_eq!( + view.quantiles(&[0.0, 1.0], criteria).unwrap(), + Some(vec![0, 7]) + ); } } @@ -62,30 +68,85 @@ fn quantile_endpoints_with_large_stream_weight() { let view = sketch.sorted_view(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert_eq!(sketch.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(sketch.quantile(1.0, criteria).unwrap(), 1); - assert_eq!(view.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(view.quantile(1.0, criteria).unwrap(), 1); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), Some(1)); + assert_eq!(view.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(view.quantile(1.0, criteria).unwrap(), Some(1)); } } #[test] -fn empty_and_invalid_queries_return_errors() { - let mut sketch = KllSketch::::new(DEFAULT_K).unwrap(); - assert!(sketch.rank(&0, SearchCriteria::Inclusive).is_err()); - assert!(sketch.quantile(0.5, SearchCriteria::Inclusive).is_err()); - assert!(sketch.pmf(&[0], SearchCriteria::Inclusive).is_err()); - assert!(sketch.cdf(&[0], SearchCriteria::Inclusive).is_err()); +fn valid_queries_on_empty_sketch_return_no_result() { + let sketch = KllSketch::::new(DEFAULT_K).unwrap(); + let view = sketch.sorted_view(); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + assert_eq!(sketch.rank(&0, criteria), None); + assert_eq!(view.rank(&0, criteria), None); + for rank in [0.0, 0.5, 1.0] { + assert_eq!(sketch.quantile(rank, criteria).unwrap(), None); + assert_eq!(view.quantile(rank, criteria).unwrap(), None); + } + assert_eq!(sketch.quantiles(&[0.0, 0.5, 1.0], criteria).unwrap(), None); + assert_eq!(view.quantiles(&[0.0, 0.5, 1.0], criteria).unwrap(), None); + assert_eq!(sketch.cdf(&[0], criteria).unwrap(), None); + assert_eq!(sketch.pmf(&[0], criteria).unwrap(), None); + assert_eq!(view.cdf(&[0], criteria).unwrap(), None); + assert_eq!(view.pmf(&[0], criteria).unwrap(), None); + } +} - sketch.update(0); - for rank in [-1.0, f64::NAN, 1.1] { - let error = sketch - .quantile(rank, SearchCriteria::Inclusive) - .unwrap_err(); - assert_eq!(error.kind(), ErrorKind::InvalidArgument); - } - let error = sketch.cdf(&[1, 0], SearchCriteria::Inclusive).unwrap_err(); - assert_eq!(error.kind(), ErrorKind::InvalidArgument); +#[test] +fn empty_argument_slices_preserve_the_sketch_empty_state() { + for n in [0, 1] { + let mut sketch = KllSketch::::new(DEFAULT_K).unwrap(); + for item in 0..n { + sketch.update(item); + } + let view = sketch.sorted_view(); + let quantiles = (n != 0).then(Vec::new); + let distribution = (n != 0).then(|| vec![1.0]); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + assert_eq!(sketch.quantiles(&[], criteria).unwrap(), quantiles); + assert_eq!(view.quantiles(&[], criteria).unwrap(), quantiles); + assert_eq!(sketch.cdf(&[], criteria).unwrap(), distribution); + assert_eq!(sketch.pmf(&[], criteria).unwrap(), distribution); + assert_eq!(view.cdf(&[], criteria).unwrap(), distribution); + assert_eq!(view.pmf(&[], criteria).unwrap(), distribution); + } + } +} + +#[test] +fn invalid_arguments_are_rejected_regardless_of_sketch_state() { + for n in [0, 1, 1_000] { + let mut sketch = KllSketch::::new(DEFAULT_K).unwrap(); + for item in 0..n { + sketch.update(item); + } + let view = sketch.sorted_view(); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + for rank in [f64::NAN, f64::NEG_INFINITY, -0.1, 1.1, f64::INFINITY] { + for error in [ + sketch.quantile(rank, criteria).unwrap_err(), + view.quantile(rank, criteria).unwrap_err(), + sketch.quantiles(&[0.5, rank], criteria).unwrap_err(), + view.quantiles(&[0.5, rank], criteria).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } + } + for splits in [[1, 1], [2, 1]] { + for error in [ + sketch.cdf(&splits, criteria).unwrap_err(), + sketch.pmf(&splits, criteria).unwrap_err(), + view.cdf(&splits, criteria).unwrap_err(), + view.pmf(&splits, criteria).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } + } + } + } } #[test] @@ -99,8 +160,14 @@ fn inclusive_and_exclusive_semantics_cover_duplicates() { assert_eq!(sketch.rank(&1, SearchCriteria::Inclusive).unwrap(), 0.5); assert_eq!(sketch.rank(&2, SearchCriteria::Exclusive).unwrap(), 0.5); assert_eq!(sketch.rank(&2, SearchCriteria::Inclusive).unwrap(), 1.0); - assert_eq!(sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(), 1); - assert_eq!(sketch.quantile(0.5, SearchCriteria::Exclusive).unwrap(), 2); + assert_eq!( + sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(), + Some(1) + ); + assert_eq!( + sketch.quantile(0.5, SearchCriteria::Exclusive).unwrap(), + Some(2) + ); } #[test] @@ -110,11 +177,17 @@ fn exact_mode_queries_match_the_stream() { sketch.update(item); } - assert_eq!(sketch.quantile(0.0, SearchCriteria::Inclusive).unwrap(), 1); - assert_eq!(sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(), 50); + assert_eq!( + sketch.quantile(0.0, SearchCriteria::Inclusive).unwrap(), + Some(1) + ); + assert_eq!( + sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(), + Some(50) + ); assert_eq!( sketch.quantile(1.0, SearchCriteria::Inclusive).unwrap(), - 100 + Some(100) ); for item in 1..=100 { assert_eq!( @@ -152,8 +225,8 @@ fn rank_cdf_and_pmf_are_consistent() { let split_points: Vec<_> = (100..10_000).step_by(100).collect(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - let cdf = sketch.cdf(&split_points, criteria).unwrap(); - let pmf = sketch.pmf(&split_points, criteria).unwrap(); + let cdf = sketch.cdf(&split_points, criteria).unwrap().unwrap(); + let pmf = sketch.pmf(&split_points, criteria).unwrap().unwrap(); let mut subtotal = 0.0; for (index, split_point) in split_points.iter().enumerate() { subtotal += pmf[index]; @@ -172,18 +245,23 @@ fn sorted_view_supports_repeated_and_batch_queries() { } let view = sketch.sorted_view(); let ranks = [0.0, 0.25, 0.5, 0.75, 1.0]; - let quantiles = sketch.quantiles(&ranks, SearchCriteria::Inclusive).unwrap(); + let quantiles = sketch + .quantiles(&ranks, SearchCriteria::Inclusive) + .unwrap() + .unwrap(); assert_eq!(view.len(), sketch.num_retained()); assert_eq!(view.total_weight(), sketch.n()); assert_eq!( - view.quantiles(&ranks, SearchCriteria::Inclusive).unwrap(), + view.quantiles(&ranks, SearchCriteria::Inclusive) + .unwrap() + .unwrap(), quantiles ); for (&rank, quantile) in ranks.iter().zip(&quantiles) { assert_eq!( view.quantile(rank, SearchCriteria::Inclusive).unwrap(), - *quantile + Some(*quantile) ); assert_eq!( view.rank(quantile, SearchCriteria::Inclusive).unwrap(), @@ -193,5 +271,8 @@ fn sorted_view_supports_repeated_and_batch_queries() { sketch.update(2_000); assert_eq!(view.total_weight(), 1_000); - assert_eq!(view.quantile(1.0, SearchCriteria::Inclusive).unwrap(), 999); + assert_eq!( + view.quantile(1.0, SearchCriteria::Inclusive).unwrap(), + Some(999) + ); } From fd871f9fb515fa0007302577ba4949ec37cf0b3c Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 15:49:09 +0800 Subject: [PATCH 2/6] fix(req): distinguish empty queries from invalid arguments Apply the query contract to REQ sketches and sorted views, preserving exact extrema and validating ranks and split points before emptiness. Document the migration and distinguish sample queries from error models. Co-authored-by: Jiawei Zhao --- CHANGELOG.md | 1 + datasketches/src/error.rs | 6 +- datasketches/src/req/mod.rs | 11 +- datasketches/src/req/sketch.rs | 85 +++++++----- datasketches/src/req/sorted_view.rs | 123 +++++++++++------- tests-integration/tests/req_test/accuracy.rs | 8 +- tests-integration/tests/req_test/bounds.rs | 4 +- tests-integration/tests/req_test/core.rs | 44 ++++--- tests-integration/tests/req_test/generic.rs | 5 +- tests-integration/tests/req_test/merge.rs | 21 ++- tests-integration/tests/req_test/property.rs | 6 +- tests-integration/tests/req_test/query.rs | 84 +++++++----- .../tests/req_test/sorted_view_api.rs | 96 +++++++++----- tests-integration/tests/serde_tests/req.rs | 5 +- 14 files changed, 323 insertions(+), 176 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 86f55dd4..0e1ef939 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ All significant changes to this project will be documented in this file. * `BloomFilter::invert` is removed; use `BloomFilter::difference` for approximate A-not-B. The result excludes items in the right filter, but hash collisions can also remove items unique to the left filter. * `FrequentItemsSketch::is_empty` now returns `false` when the stream weight is nonzero, even if no items are retained. Use `num_active_items() == 0` to test for zero retained items. * `ReqSketch` queries now use `datasketches::common::SearchCriteria` instead of `datasketches::req::SearchCriteria`. `SearchCriteria` no longer implements `Default`; explicitly choose `Inclusive` or `Exclusive`. +* `ReqSketch` and `req::SortedView` queries now distinguish empty sketches from invalid arguments. `rank` returns `Option`; `quantile`, `quantiles`, `cdf`, and `pmf`, where available, return `Result, Error>`. Handle `None` for valid queries on an empty sketch, and propagate or handle `InvalidArgument` for invalid ranks or split points, which are checked before emptiness. ### New features diff --git a/datasketches/src/error.rs b/datasketches/src/error.rs index 801f902d..18a26100 100644 --- a/datasketches/src/error.rs +++ b/datasketches/src/error.rs @@ -23,7 +23,7 @@ //! absence; invalid caller arguments use `Result::Err`. A query that can encounter both returns //! `Result, Error>`: `Ok(None)` means no result, not an invalid request. //! -//! The following contract applies to KLL sketches and their sorted views: +//! The following contract applies to KLL and REQ sketches and their sorted views, where exposed: //! //! | Query | Valid input, non-empty | Valid input, empty | Invalid input, any state | //! | --- | --- | --- | --- | @@ -40,6 +40,10 @@ //! invalid arguments, then handle the `Option` for an empty sketch. //! - **Errors remain distinct:** malformed serialized data returns [`ErrorKind::InvalidData`]; //! broken internal invariants remain programming errors. +//! - **Error models still have results:** REQ `rank_lower_bound` / `rank_upper_bound` calculate +//! bounds around a supplied normalized rank. In exact mode, including an empty sketch, both +//! return that rank. KLL `normalized_rank_error` / `normalized_pmf_error` depend on +//! configuration. use std::fmt; diff --git a/datasketches/src/req/mod.rs b/datasketches/src/req/mod.rs index ce5a65fe..f479d2d6 100644 --- a/datasketches/src/req/mod.rs +++ b/datasketches/src/req/mod.rs @@ -32,6 +32,13 @@ //! Custom item types need only [`Clone`] and [`Ord`] for in-memory use. Serialization additionally //! requires [`ReqValue`]. //! +//! # Query results +//! +//! Rank, quantile, CDF, and PMF queries on an empty sketch return no result. Invalid ranks or +//! split points return [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument), +//! even on an empty sketch. See the [query contract](crate::error#query-results) for return types +//! and migration guidance. +//! //! # Example //! //! ``` @@ -44,7 +51,9 @@ //! sketch.update(ReqFloat::::new(value)?); //! } //! -//! let median = sketch.quantile(0.5, SearchCriteria::Inclusive)?; +//! let Some(median) = sketch.quantile(0.5, SearchCriteria::Inclusive)? else { +//! return Ok(()); // No samples to query. +//! }; //! assert_eq!(median.into_inner(), 2.0); //! # Ok::<(), datasketches::error::Error>(()) //! ``` diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index 77a3284a..ef17e1e7 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -180,12 +180,10 @@ where /// without building a sorted view. The result is identical to /// [`SortedView::rank`] on [`Self::sorted_view`]. /// - /// # Errors - /// - /// Returns an error if the sketch is empty. - pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Result { + /// Returns `None` if the sketch is empty. + pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Option { if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); + return None; } let inclusive = matches!(criteria, SearchCriteria::Inclusive); let weight: u64 = self @@ -193,33 +191,34 @@ where .iter() .map(|c| c.count_below(item, inclusive) as u64 * c.weight()) .sum(); - Ok(weight as f64 / self.n as f64) + Some(weight as f64 / self.n as f64) } /// Returns the approximate quantile at the given normalized rank. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the sketch is empty. /// /// For ranks strictly between `0.0` and `1.0`, builds a transient [`SortedView`]. /// For repeated queries at those ranks, take one snapshot with [`Self::sorted_view`]. /// /// # Errors /// - /// Returns an error if the sketch is empty or `rank` is outside `[0.0, 1.0]`. - pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result { - if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); - } + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( "rank {rank} must be in [0, 1]" ))); } + if self.is_empty() { + return Ok(None); + } if rank == 0.0 { - return Ok(self.min_item.as_ref().unwrap().clone()); + return Ok(Some(self.min_item.as_ref().unwrap().clone())); } if rank == 1.0 { - return Ok(self.max_item.as_ref().unwrap().clone()); + return Ok(Some(self.max_item.as_ref().unwrap().clone())); } self.sorted_view().quantile(rank, criteria) } @@ -227,16 +226,18 @@ where /// Returns approximate quantiles for the given normalized ranks. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the sketch is empty. /// /// The sorted view is built once and shared across all ranks. /// /// # Errors /// - /// Returns an error if the sketch is empty or any rank is outside `[0.0, 1.0]`. - pub fn quantiles(&self, ranks: &[f64], criteria: SearchCriteria) -> Result, Error> { - if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); - } + /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + pub fn quantiles( + &self, + ranks: &[f64], + criteria: SearchCriteria, + ) -> Result>, Error> { // Reject invalid ranks before paying for the view build. for &r in ranks { if !(0.0..=1.0).contains(&r) { @@ -245,37 +246,55 @@ where ))); } } + if self.is_empty() { + return Ok(None); + } let view = self.sorted_view(); - ranks.iter().map(|&r| view.quantile(r, criteria)).collect() + let quantiles = ranks + .iter() + .map(|&r| { + view.quantile(r, criteria) + .map(|quantile| quantile.expect("checked non-empty view")) + }) + .collect::>()?; + Ok(Some(quantiles)) } /// Returns the Probability Mass Function over the given split points. /// + /// Returns `Ok(None)` if the sketch is empty. + /// /// # Errors /// - /// Returns an error if the sketch is empty or the split points are not strictly increasing. - pub fn pmf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); - } + /// Returns an error if the split points are not strictly increasing, even if the sketch is + /// empty. + pub fn pmf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { self.sorted_view().pmf(split_points, criteria) } /// Returns the Cumulative Distribution Function over the given split points. /// + /// Returns `Ok(None)` if the sketch is empty. + /// /// # Errors /// - /// Returns an error if the sketch is empty or the split points are not strictly increasing. - pub fn cdf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); - } + /// Returns an error if the split points are not strictly increasing, even if the sketch is + /// empty. + pub fn cdf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { self.sorted_view().cdf(split_points, criteria) } /// Returns an owned, sorted snapshot of the sketch's current state. /// - /// An empty sketch yields an empty view; queries on it return an error. The + /// An empty sketch yields an empty view; valid queries on it return no result. The /// view is independent of the sketch — it can be queried (and sent to other /// threads) while the sketch keeps receiving updates, and it keeps answering /// from the state it was taken at. @@ -371,6 +390,9 @@ where } /// Returns the lower bound for the normalized `rank` at the requested confidence level. + /// + /// This evaluates the error model around the supplied rank without querying a sample. + /// In exact mode, including an empty sketch, the bound equals `rank`. pub fn rank_lower_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_lower_bound( self.k, @@ -383,6 +405,9 @@ where } /// Returns the upper bound for the normalized `rank` at the requested confidence level. + /// + /// This evaluates the error model around the supplied rank without querying a sample. + /// In exact mode, including an empty sketch, the bound equals `rank`. pub fn rank_upper_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_upper_bound( self.k, diff --git a/datasketches/src/req/sorted_view.rs b/datasketches/src/req/sorted_view.rs index ec62f873..1558c6ba 100644 --- a/datasketches/src/req/sorted_view.rs +++ b/datasketches/src/req/sorted_view.rs @@ -93,48 +93,46 @@ where /// Returns the approximate normalized rank of `item` in `[0.0, 1.0]`. /// - /// # Errors - /// - /// Returns an error if the view is empty. - pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Result { + /// Returns `None` if the view is empty. + pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Option { if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); + return None; } let pos = match criteria { SearchCriteria::Inclusive => self.items.partition_point(|x| x <= item), SearchCriteria::Exclusive => self.items.partition_point(|x| x < item), }; if pos == 0 { - Ok(0.0) + Some(0.0) } else { - Ok(self.cumulative_weights[pos - 1] as f64 / self.total_weight as f64) + Some(self.cumulative_weights[pos - 1] as f64 / self.total_weight as f64) } } /// Returns the approximate quantile at the given normalized rank. /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. + /// Returns `Ok(None)` if the view is empty. /// /// # Errors /// - /// Returns an error if the view is empty or `rank` is outside `[0.0, 1.0]`. - pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result { - if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); - } - + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is empty. + pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( "rank {rank} must be in [0, 1]" ))); } + if self.is_empty() { + return Ok(None); + } // Compaction may discard extrema, and large weights can lose precision in f64. if rank == 0.0 { - return Ok(self.min_item.as_ref().unwrap().clone()); + return Ok(Some(self.min_item.as_ref().unwrap().clone())); } if rank == 1.0 { - return Ok(self.max_item.as_ref().unwrap().clone()); + return Ok(Some(self.max_item.as_ref().unwrap().clone())); } let target_weight = match criteria { @@ -151,44 +149,58 @@ where .partition_point(|&w| w <= target_weight), }; - Ok(self.items[index.min(self.items.len() - 1)].clone()) + Ok(Some(self.items[index.min(self.items.len() - 1)].clone())) } /// Returns the probability mass function (PMF) over the given split points. /// /// The result contains one more value than `split_points`. + /// Returns `Ok(None)` if the view is empty. /// /// # Errors /// - /// Returns an error if the view is empty or the split points are not strictly increasing. - pub fn pmf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { - let mut result = self.cdf(split_points, criteria)?; + /// Returns an error if the split points are not strictly increasing, even if the view is empty. + pub fn pmf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { + let Some(mut result) = self.cdf(split_points, criteria)? else { + return Ok(None); + }; for index in (1..result.len()).rev() { result[index] -= result[index - 1]; } - Ok(result) + Ok(Some(result)) } /// Returns the cumulative distribution function (CDF) over the given split points. /// /// The result contains one more value than `split_points` and ends at `1.0`. + /// Returns `Ok(None)` if the view is empty. /// /// # Errors /// - /// Returns an error if the view is empty or the split points are not strictly increasing. - pub fn cdf(&self, split_points: &[T], criteria: SearchCriteria) -> Result, Error> { + /// Returns an error if the split points are not strictly increasing, even if the view is empty. + pub fn cdf( + &self, + split_points: &[T], + criteria: SearchCriteria, + ) -> Result>, Error> { + self.validate_split_points(split_points)?; if self.is_empty() { - return Err(Error::invalid_argument("sketch is empty")); + return Ok(None); } - self.validate_split_points(split_points)?; - let mut result = Vec::with_capacity(split_points.len() + 1); for split_point in split_points { - result.push(self.rank(split_point, criteria)?); + result.push( + self.rank(split_point, criteria) + .expect("checked non-empty view"), + ); } result.push(1.0); - Ok(result) + Ok(Some(result)) } fn validate_split_points(&self, split_points: &[T]) -> Result<(), Error> { @@ -205,8 +217,6 @@ where mod tests { use googletest::assert_that; use googletest::prelude::all; - use googletest::prelude::anything; - use googletest::prelude::err; use googletest::prelude::ge; use googletest::prelude::le; use googletest::prelude::near; @@ -227,21 +237,38 @@ mod tests { } #[test] - fn test_rank_queries() -> Result<(), Error> { + fn test_rank_queries() { let view = create_test_view(); // Test exact matches - assert_that!(view.rank(&1, SearchCriteria::Inclusive)?, near(0.2, 1e-10)); - assert_that!(view.rank(&1, SearchCriteria::Exclusive)?, near(0.0, 1e-10)); + assert_that!( + view.rank(&1, SearchCriteria::Inclusive).unwrap(), + near(0.2, 1e-10) + ); + assert_that!( + view.rank(&1, SearchCriteria::Exclusive).unwrap(), + near(0.0, 1e-10) + ); // Test values between items - assert_that!(view.rank(&2, SearchCriteria::Inclusive)?, near(0.2, 1e-10)); - assert_that!(view.rank(&6, SearchCriteria::Inclusive)?, near(0.6, 1e-10)); + assert_that!( + view.rank(&2, SearchCriteria::Inclusive).unwrap(), + near(0.2, 1e-10) + ); + assert_that!( + view.rank(&6, SearchCriteria::Inclusive).unwrap(), + near(0.6, 1e-10) + ); // Test edge cases - assert_that!(view.rank(&0, SearchCriteria::Inclusive)?, near(0.0, 1e-10)); - assert_that!(view.rank(&10, SearchCriteria::Inclusive)?, near(1.0, 1e-10)); - Ok(()) + assert_that!( + view.rank(&0, SearchCriteria::Inclusive).unwrap(), + near(0.0, 1e-10) + ); + assert_that!( + view.rank(&10, SearchCriteria::Inclusive).unwrap(), + near(1.0, 1e-10) + ); } #[test] @@ -249,16 +276,16 @@ mod tests { let view = create_test_view(); // Test edge cases - assert_eq!(view.quantile(0.0, SearchCriteria::Inclusive)?, 1); - assert_eq!(view.quantile(1.0, SearchCriteria::Inclusive)?, 9); + assert_eq!(view.quantile(0.0, SearchCriteria::Inclusive)?, Some(1)); + assert_eq!(view.quantile(1.0, SearchCriteria::Inclusive)?, Some(9)); // Test middle values - let median = view.quantile(0.5, SearchCriteria::Inclusive)?; + let median = view.quantile(0.5, SearchCriteria::Inclusive)?.unwrap(); assert_that!(median, all!(ge(3), le(7))); // Should be around the middle (values are 1,3,5,7,9) // Test various ranks - let q25 = view.quantile(0.25, SearchCriteria::Inclusive)?; - let q75 = view.quantile(0.75, SearchCriteria::Inclusive)?; + let q25 = view.quantile(0.25, SearchCriteria::Inclusive)?.unwrap(); + let q75 = view.quantile(0.75, SearchCriteria::Inclusive)?.unwrap(); assert_that!(q25, le(median)); assert_that!(median, le(q75)); Ok(()) @@ -269,7 +296,7 @@ mod tests { let view = create_test_view(); let split_points = vec![3, 7]; - let pmf = view.pmf(&split_points, SearchCriteria::Inclusive)?; + let pmf = view.pmf(&split_points, SearchCriteria::Inclusive)?.unwrap(); assert_eq!(pmf.len(), 3); // 2 split points create 3 intervals // Sum should be approximately 1.0 @@ -283,7 +310,7 @@ mod tests { let view = create_test_view(); let split_points = vec![3, 7]; - let cdf = view.cdf(&split_points, SearchCriteria::Inclusive)?; + let cdf = view.cdf(&split_points, SearchCriteria::Inclusive)?.unwrap(); assert_eq!(cdf.len(), 3); // CDF should be monotonically increasing @@ -303,11 +330,7 @@ mod tests { assert_eq!(view.len(), 0); assert_eq!(view.total_weight(), 0); - // Operations on empty view should return errors - assert_that!(view.rank(&5, SearchCriteria::Inclusive), err(anything())); - assert_that!( - view.quantile(0.5, SearchCriteria::Inclusive), - err(anything()) - ); + assert_eq!(view.rank(&5, SearchCriteria::Inclusive), None); + assert_eq!(view.quantile(0.5, SearchCriteria::Inclusive).unwrap(), None); } } diff --git a/tests-integration/tests/req_test/accuracy.rs b/tests-integration/tests/req_test/accuracy.rs index 76c318cc..0fc361c7 100644 --- a/tests-integration/tests/req_test/accuracy.rs +++ b/tests-integration/tests/req_test/accuracy.rs @@ -38,8 +38,12 @@ fn rank_space_error_is_bounded() -> Result<(), Error> { assert_eq!(sketch.n(), n as u64); for rank in [0.1, 0.25, 0.5, 0.75, 0.9, 0.95, 0.99] { - let quantile = sketch.quantile(rank, SearchCriteria::Inclusive)?; - let estimated_rank = sketch.rank(&quantile, SearchCriteria::Inclusive)?; + let quantile = sketch + .quantile(rank, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"); + let estimated_rank = sketch + .rank(&quantile, SearchCriteria::Inclusive) + .expect("the sketch is non-empty"); let abs_rank_error = (estimated_rank - rank).abs(); let max_abs_rank_error = if rank >= 0.9 { 0.01 } else { 0.02 }; diff --git a/tests-integration/tests/req_test/bounds.rs b/tests-integration/tests/req_test/bounds.rs index d6ee3a77..a0c61992 100644 --- a/tests-integration/tests/req_test/bounds.rs +++ b/tests-integration/tests/req_test/bounds.rs @@ -77,7 +77,9 @@ fn theoretical_error_bounds_cover_uniform_quantiles() -> Result<(), Error> { 0.95, 0.97, 0.98, 0.99, 0.995, 0.999, ] { let true_quantile = req_f64(rank * (n - 1) as f64); - let estimated_rank = sketch.rank(&true_quantile, SearchCriteria::Inclusive)?; + let estimated_rank = sketch + .rank(&true_quantile, SearchCriteria::Inclusive) + .expect("the sketch is non-empty"); let lower = sketch.rank_lower_bound(rank, NumStdDev::Three); let upper = sketch.rank_upper_bound(rank, NumStdDev::Three); assert_that!(estimated_rank, all!(ge(lower), le(upper)), "rank: {rank}"); diff --git a/tests-integration/tests/req_test/core.rs b/tests-integration/tests/req_test/core.rs index 626632db..1a3622e7 100644 --- a/tests-integration/tests/req_test/core.rs +++ b/tests-integration/tests/req_test/core.rs @@ -42,7 +42,7 @@ use super::req_f32; use super::req_f64; #[test] -fn empty_sketch_has_default_state_and_rejects_queries() { +fn empty_sketch_has_default_state_and_absent_queries() { let sketch: ReqSketch = ReqSketch::default(); assert_eq!(sketch.k(), 12); @@ -53,22 +53,23 @@ fn empty_sketch_has_default_state_and_rejects_queries() { assert_that!(sketch.min_item(), none()); assert_that!(sketch.max_item(), none()); - assert_that!( - sketch.rank(&req_f64(0.0), SearchCriteria::Inclusive), - err(anything()) - ); - assert_that!( + assert_eq!(sketch.rank(&req_f64(0.0), SearchCriteria::Inclusive), None); + assert!(matches!( sketch.quantile(0.5, SearchCriteria::Inclusive), - err(anything()) - ); - assert_that!( + Ok(None) + )); + assert!(matches!( + sketch.quantiles(&[0.25, 0.75], SearchCriteria::Inclusive), + Ok(None) + )); + assert!(matches!( sketch.pmf(&[req_f64(0.0)], SearchCriteria::Inclusive), - err(anything()) - ); - assert_that!( + Ok(None) + )); + assert!(matches!( sketch.cdf(&[req_f64(0.0)], SearchCriteria::Inclusive), - err(anything()) - ); + Ok(None) + )); } #[test] @@ -112,7 +113,8 @@ fn single_value_hra_answers_exactly() { assert_eq!( sketch .quantile(rank, SearchCriteria::Exclusive) - .expect("quantile should succeed"), + .expect("quantile should succeed") + .expect("the sketch is non-empty"), req_f32(1.0) ); } @@ -221,14 +223,18 @@ fn small_edge_cases_answer_reasonably() -> Result<(), Error> { let mut single: ReqSketch = ReqSketch::default(); single.update(req_f64(42.0)); assert_eq!( - single.quantile(0.5, SearchCriteria::Inclusive)?, + single + .quantile(0.5, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"), req_f64(42.0) ); let mut two_values = ReqSketch::default(); two_values.update(req_f64(1.0)); two_values.update(req_f64(100.0)); - let median = two_values.quantile(0.5, SearchCriteria::Inclusive)?; + let median = two_values + .quantile(0.5, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"); assert_that!(*median, all!(ge(1.0), le(100.0))); let mut duplicates = ReqSketch::default(); @@ -236,7 +242,9 @@ fn small_edge_cases_answer_reasonably() -> Result<(), Error> { duplicates.update(req_f64(42.0)); } assert_eq!( - duplicates.quantile(0.5, SearchCriteria::Inclusive)?, + duplicates + .quantile(0.5, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"), req_f64(42.0) ); diff --git a/tests-integration/tests/req_test/generic.rs b/tests-integration/tests/req_test/generic.rs index a405e815..ab4335b5 100644 --- a/tests-integration/tests/req_test/generic.rs +++ b/tests-integration/tests/req_test/generic.rs @@ -31,7 +31,10 @@ fn custom_items_do_not_need_serialization() { assert_eq!(sketch.min_item(), Some(&Reading(10))); assert_eq!(sketch.max_item(), Some(&Reading(30))); assert_eq!( - sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(), + sketch + .quantile(0.5, SearchCriteria::Inclusive) + .unwrap() + .unwrap(), Reading(20) ); } diff --git a/tests-integration/tests/req_test/merge.rs b/tests-integration/tests/req_test/merge.rs index d4c80e29..d24d6e7c 100644 --- a/tests-integration/tests/req_test/merge.rs +++ b/tests-integration/tests/req_test/merge.rs @@ -43,13 +43,16 @@ fn merge_into_empty_preserves_source_distribution() { let q25 = target .quantile(0.25, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let q50 = target .quantile(0.5, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let q75 = target .quantile(0.75, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let r50 = target .rank(&req_f64(500.0), SearchCriteria::Inclusive) .expect("rank should succeed"); @@ -78,13 +81,16 @@ fn merge_two_ranges_preserves_distribution() { let q25 = left .quantile(0.25, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let q50 = left .quantile(0.5, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let q75 = left .quantile(0.75, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let r50 = left .rank(&req_f64(1000.0), SearchCriteria::Inclusive) .expect("rank should succeed"); @@ -122,6 +128,7 @@ fn many_small_merges_preserve_count_bounds_and_median() { let median = sketch .quantile(0.5, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); assert_that!(*median, near(4999.5, 500.0)); } diff --git a/tests-integration/tests/req_test/property.rs b/tests-integration/tests/req_test/property.rs index cf7a8bd5..24d91df7 100644 --- a/tests-integration/tests/req_test/property.rs +++ b/tests-integration/tests/req_test/property.rs @@ -48,7 +48,8 @@ fn prop_quantile_rank_consistency() { for rank in [0.1, 0.25, 0.5, 0.75, 0.9] { let quantile = sketch .quantile(rank, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); let recovered = sketch .rank(&quantile, SearchCriteria::Inclusive) .expect("rank should succeed"); @@ -96,7 +97,8 @@ fn prop_sketch_bounds() { for rank in [0.0, 0.25, 0.5, 0.75, 1.0] { let quantile = sketch .quantile(rank, SearchCriteria::Inclusive) - .expect("quantile should succeed"); + .expect("quantile should succeed") + .expect("the sketch is non-empty"); assert!( quantile >= true_min && quantile <= true_max, "quantile {} out of bounds [{}, {}]", diff --git a/tests-integration/tests/req_test/query.rs b/tests-integration/tests/req_test/query.rs index bc037434..20d2fda6 100644 --- a/tests-integration/tests/req_test/query.rs +++ b/tests-integration/tests/req_test/query.rs @@ -64,7 +64,8 @@ fn exact_mode_rank_quantile_pmf_and_cdf_match_reference() { assert_eq!( *sketch .quantile(rank, SearchCriteria::Exclusive) - .expect("quantile should succeed"), + .expect("quantile should succeed") + .expect("the sketch is non-empty"), expected ); } @@ -73,7 +74,8 @@ fn exact_mode_rank_quantile_pmf_and_cdf_match_reference() { assert_eq!( *sketch .quantile(rank, SearchCriteria::Inclusive) - .expect("quantile should succeed"), + .expect("quantile should succeed") + .expect("the sketch is non-empty"), expected ); } @@ -81,7 +83,8 @@ fn exact_mode_rank_quantile_pmf_and_cdf_match_reference() { let splits = [2.0, 6.0, 9.0].map(req_f64); let cdf = sketch .cdf(&splits, SearchCriteria::Exclusive) - .expect("cdf should succeed"); + .expect("cdf should succeed") + .expect("the sketch is non-empty"); assert_that!(cdf[0], near(0.1, 1e-6)); assert_that!(cdf[1], near(0.5, 1e-6)); assert_that!(cdf[2], near(0.8, 1e-6)); @@ -89,7 +92,8 @@ fn exact_mode_rank_quantile_pmf_and_cdf_match_reference() { let pmf = sketch .pmf(&splits, SearchCriteria::Exclusive) - .expect("pmf should succeed"); + .expect("pmf should succeed") + .expect("the sketch is non-empty"); assert_that!(pmf[0], near(0.1, 1e-6)); assert_that!(pmf[1], near(0.4, 1e-6)); assert_that!(pmf[2], near(0.3, 1e-6)); @@ -106,10 +110,12 @@ fn pmf_and_cdf_are_consistent() { let split_points = [100.0, 300.0, 500.0, 700.0, 900.0].map(req_f64); let pmf = sketch .pmf(&split_points, SearchCriteria::Inclusive) - .expect("pmf should succeed"); + .expect("pmf should succeed") + .expect("the sketch is non-empty"); let cdf = sketch .cdf(&split_points, SearchCriteria::Inclusive) - .expect("cdf should succeed"); + .expect("cdf should succeed") + .expect("the sketch is non-empty"); assert_that!(pmf.iter().sum::(), near(1.0, 1e-10)); @@ -139,9 +145,9 @@ fn cdf_preserves_ranks_and_exact_endpoint() { .chain(std::iter::once(1.0)) .collect(); - assert_eq!(sketch.cdf(&splits, criteria).unwrap(), expected); - assert_eq!(view.cdf(&splits, criteria).unwrap(), expected); - assert_eq!(view.cdf(&[], criteria).unwrap(), [1.0]); + assert_eq!(sketch.cdf(&splits, criteria).unwrap().unwrap(), expected); + assert_eq!(view.cdf(&splits, criteria).unwrap().unwrap(), expected); + assert_eq!(view.cdf(&[], criteria).unwrap(), Some(vec![1.0])); } } @@ -157,14 +163,14 @@ fn quantile_endpoints_preserve_extrema_after_compaction() { let view = sketch.sorted_view(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert_eq!(sketch.quantile(0.0, criteria).unwrap(), -item); - assert_eq!(sketch.quantile(1.0, criteria).unwrap(), item); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), Some(-item)); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), Some(item)); assert_eq!( sketch.quantiles(&[0.0, 1.0], criteria).unwrap(), - [-item, item] + Some(vec![-item, item]) ); - assert_eq!(view.quantile(0.0, criteria).unwrap(), -item); - assert_eq!(view.quantile(1.0, criteria).unwrap(), item); + assert_eq!(view.quantile(0.0, criteria).unwrap(), Some(-item)); + assert_eq!(view.quantile(1.0, criteria).unwrap(), Some(item)); } } assert!(sketch.is_estimation_mode()); @@ -172,10 +178,10 @@ fn quantile_endpoints_preserve_extrema_after_compaction() { let view = sketch.sorted_view(); sketch.reset(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert!(sketch.quantile(0.0, criteria).is_err()); - assert!(sketch.quantile(1.0, criteria).is_err()); - assert_eq!(view.quantile(0.0, criteria).unwrap(), -64); - assert_eq!(view.quantile(1.0, criteria).unwrap(), 64); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), None); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), None); + assert_eq!(view.quantile(0.0, criteria).unwrap(), Some(-64)); + assert_eq!(view.quantile(1.0, criteria).unwrap(), Some(64)); } } } @@ -192,10 +198,10 @@ fn quantile_endpoints_with_large_stream_weight() { let view = sketch.sorted_view(); for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { - assert_eq!(sketch.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(sketch.quantile(1.0, criteria).unwrap(), 1); - assert_eq!(view.quantile(0.0, criteria).unwrap(), 0); - assert_eq!(view.quantile(1.0, criteria).unwrap(), 1); + assert_eq!(sketch.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(sketch.quantile(1.0, criteria).unwrap(), Some(1)); + assert_eq!(view.quantile(0.0, criteria).unwrap(), Some(0)); + assert_eq!(view.quantile(1.0, criteria).unwrap(), Some(1)); } } @@ -230,7 +236,9 @@ fn quantiles_are_monotonic() -> Result<(), Error> { let mut previous = 0.0; for rank in ranks { - let quantile = sketch.quantile(rank, SearchCriteria::Inclusive)?; + let quantile = sketch + .quantile(rank, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"); assert_that!(*quantile, ge(previous)); previous = *quantile; } @@ -246,8 +254,12 @@ fn rank_quantile_round_trip_is_consistent() -> Result<(), Error> { } for target_rank in [0.1, 0.25, 0.5, 0.75, 0.9] { - let quantile = sketch.quantile(target_rank, SearchCriteria::Inclusive)?; - let recovered_rank = sketch.rank(&quantile, SearchCriteria::Inclusive)?; + let quantile = sketch + .quantile(target_rank, SearchCriteria::Inclusive)? + .expect("the sketch is non-empty"); + let recovered_rank = sketch + .rank(&quantile, SearchCriteria::Inclusive) + .expect("the sketch is non-empty"); let error = (recovered_rank - target_rank).abs() / target_rank; assert_that!(error, lt(0.2)); } @@ -263,8 +275,12 @@ fn search_criteria_rank_consistency() -> Result<(), Error> { } for value in [100.0, 250.0, 500.0, 750.0].map(req_f64) { - let inclusive_rank = sketch.rank(&value, SearchCriteria::Inclusive)?; - let exclusive_rank = sketch.rank(&value, SearchCriteria::Exclusive)?; + let inclusive_rank = sketch + .rank(&value, SearchCriteria::Inclusive) + .expect("the sketch is non-empty"); + let exclusive_rank = sketch + .rank(&value, SearchCriteria::Exclusive) + .expect("the sketch is non-empty"); assert_that!(exclusive_rank, le(inclusive_rank)); assert_that!(inclusive_rank, all!(ge(0.0), le(1.0))); @@ -283,8 +299,18 @@ fn signed_zeros_share_rank_and_cannot_be_distinct_splits() -> Result<(), Error> sketch.update(positive_zero); for value in [negative_zero, positive_zero] { - assert_eq!(sketch.rank(&value, SearchCriteria::Exclusive)?, 0.0); - assert_eq!(sketch.rank(&value, SearchCriteria::Inclusive)?, 1.0); + assert_eq!( + sketch + .rank(&value, SearchCriteria::Exclusive) + .expect("the sketch is non-empty"), + 0.0 + ); + assert_eq!( + sketch + .rank(&value, SearchCriteria::Inclusive) + .expect("the sketch is non-empty"), + 1.0 + ); } assert!( diff --git a/tests-integration/tests/req_test/sorted_view_api.rs b/tests-integration/tests/req_test/sorted_view_api.rs index 59c4e6dc..4c4d3d71 100644 --- a/tests-integration/tests/req_test/sorted_view_api.rs +++ b/tests-integration/tests/req_test/sorted_view_api.rs @@ -25,9 +25,6 @@ use datasketches::req::ReqSketch; use datasketches::req::SortedView; use googletest::assert_that; use googletest::prelude::all; -use googletest::prelude::anything; -use googletest::prelude::contains_substring; -use googletest::prelude::err; use googletest::prelude::ge; use googletest::prelude::lt; use googletest::prelude::near; @@ -47,19 +44,23 @@ fn populated_sketch(n: i64) -> ReqSketch { fn query_through_shared_ref(sketch: &ReqSketch) { sketch .quantile(0.5, SearchCriteria::Inclusive) - .expect("quantile"); + .expect("quantile") + .expect("the sketch is non-empty"); sketch .quantiles(&[0.25, 0.5, 0.75], SearchCriteria::Inclusive) - .expect("quantiles"); + .expect("quantiles") + .expect("the sketch is non-empty"); sketch .rank(&req_f64(50.0), SearchCriteria::Inclusive) .expect("rank"); sketch .pmf(&[req_f64(10.0), req_f64(50.0)], SearchCriteria::Inclusive) - .expect("pmf"); + .expect("pmf") + .expect("the sketch is non-empty"); sketch .cdf(&[req_f64(10.0), req_f64(50.0)], SearchCriteria::Inclusive) - .expect("cdf"); + .expect("cdf") + .expect("the sketch is non-empty"); assert!(!sketch.sorted_view().is_empty()); } @@ -95,24 +96,36 @@ fn sorted_view_on_empty_sketch_is_an_empty_view() { assert!(view.is_empty()); assert_eq!(view.len(), 0); assert_eq!(view.total_weight(), 0); - // Queries on the empty view still report an error. - assert_that!( - view.quantile(0.5, SearchCriteria::Inclusive), - err(anything()) - ); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + assert_eq!(sketch.rank(&req_f64(0.0), criteria), None); + assert_eq!(view.rank(&req_f64(0.0), criteria), None); + for rank in [0.0, 0.5, 1.0] { + assert_eq!(sketch.quantile(rank, criteria).unwrap(), None); + assert_eq!(view.quantile(rank, criteria).unwrap(), None); + } + assert_eq!(sketch.quantiles(&[0.0, 0.5, 1.0], criteria).unwrap(), None); + assert_eq!(sketch.pmf(&[req_f64(0.0)], criteria).unwrap(), None); + assert_eq!(sketch.cdf(&[req_f64(0.0)], criteria).unwrap(), None); + assert_eq!(view.pmf(&[req_f64(0.0)], criteria).unwrap(), None); + assert_eq!(view.cdf(&[req_f64(0.0)], criteria).unwrap(), None); + } } #[test] -fn empty_sketch_pmf_cdf_report_error() { - let sketch: ReqSketch = ReqSketch::default(); - assert_that!( - sketch.pmf(&[req_f64(1.0)], SearchCriteria::Inclusive), - err(anything()) - ); - assert_that!( - sketch.cdf(&[req_f64(1.0)], SearchCriteria::Inclusive), - err(anything()) - ); +fn empty_argument_slices_preserve_the_sketch_empty_state() { + for n in [0, 1] { + let sketch = populated_sketch(n); + let view = sketch.sorted_view(); + let quantiles = (n != 0).then(Vec::new); + let distribution = (n != 0).then(|| vec![1.0]); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + assert_eq!(sketch.quantiles(&[], criteria).unwrap(), quantiles); + assert_eq!(sketch.cdf(&[], criteria).unwrap(), distribution); + assert_eq!(sketch.pmf(&[], criteria).unwrap(), distribution); + assert_eq!(view.cdf(&[], criteria).unwrap(), distribution); + assert_eq!(view.pmf(&[], criteria).unwrap(), distribution); + } + } } #[test] @@ -126,17 +139,33 @@ fn view_rank_is_primary_query_name() { } #[test] -fn error_precedence_empty_before_invalid_rank() { - // On an empty sketch the emptiness is reported before the out-of-range rank. - let empty: ReqSketch = ReqSketch::default(); - let empty_err = empty.quantile(2.0, SearchCriteria::Inclusive).unwrap_err(); - assert_that!(empty_err.message(), contains_substring("empty")); - - // On a populated sketch the out-of-range rank is reported. - let sketch = populated_sketch(10); - let range_err = sketch.quantile(2.0, SearchCriteria::Inclusive).unwrap_err(); - assert_eq!(range_err.kind(), ErrorKind::InvalidArgument); - assert_that!(range_err.message(), contains_substring("must be in")); +fn invalid_arguments_are_rejected_regardless_of_sketch_state() { + for n in [0, 1, 1_000] { + let sketch = populated_sketch(n); + let view = sketch.sorted_view(); + for criteria in [SearchCriteria::Inclusive, SearchCriteria::Exclusive] { + for rank in [f64::NAN, f64::NEG_INFINITY, -0.1, 1.1, f64::INFINITY] { + for error in [ + sketch.quantile(rank, criteria).unwrap_err(), + view.quantile(rank, criteria).unwrap_err(), + sketch.quantiles(&[0.5, rank], criteria).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } + } + for splits in [[1.0, 1.0], [2.0, 1.0]] { + let splits = splits.map(req_f64); + for error in [ + sketch.cdf(&splits, criteria).unwrap_err(), + sketch.pmf(&splits, criteria).unwrap_err(), + view.cdf(&splits, criteria).unwrap_err(), + view.pmf(&splits, criteria).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } + } + } + } } #[test] @@ -157,6 +186,7 @@ fn concurrent_readers_share_the_sketch() { sketch .quantile(rank, SearchCriteria::Inclusive) .expect("quantile from shared sketch") + .expect("the sketch is non-empty") }) }) .collect(); diff --git a/tests-integration/tests/serde_tests/req.rs b/tests-integration/tests/serde_tests/req.rs index d0a03e1e..a3a920f6 100644 --- a/tests-integration/tests/serde_tests/req.rs +++ b/tests-integration/tests/serde_tests/req.rs @@ -510,7 +510,10 @@ fn validate_cross_language_fixture(path: PathBuf, expected_n: u64) { sketch.max_item().copied().map(ReqFloat::into_inner), Some(expected_n as f32) ); - let _ = sketch.quantile(0.5, SearchCriteria::Inclusive).unwrap(); + sketch + .quantile(0.5, SearchCriteria::Inclusive) + .unwrap() + .expect("non-empty snapshot should have a median"); } let serialized = sketch.serialize(); From 6947ee0776652e1b4d583122060e1eed379fe339 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 15:55:05 +0800 Subject: [PATCH 3/6] fix(tdigest): distinguish empty queries from invalid arguments Return Result, Error> from rank, quantile, quantiles, CDF, and PMF queries on both mutable and frozen digests. Validate arguments before emptiness and compression, preserving None for empty data. Migrate callers, test invalid arguments and valid boundary queries, and document the empty-result inventory across all sketch families. Co-authored-by: Jiawei Zhao --- CHANGELOG.md | 1 + benchmarks/tdigest/aggregation.rs | 1 + benchmarks/tdigest/compress.rs | 6 +- benchmarks/tdigest/merge.rs | 22 +- benchmarks/tdigest/query.rs | 12 +- datasketches/src/error.rs | 76 ++- datasketches/src/lib.rs | 2 + datasketches/src/tdigest/mod.rs | 16 +- datasketches/src/tdigest/sketch.rs | 214 +++++---- .../tests/serde_tests/tdigest.rs | 59 ++- .../tests/tdigest_test/property.rs | 18 +- .../tests/tdigest_test/sketch.rs | 439 ++++++++++++------ 12 files changed, 557 insertions(+), 309 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e1ef939..2e4e3ca3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ All significant changes to this project will be documented in this file. * `FrequentItemsSketch::is_empty` now returns `false` when the stream weight is nonzero, even if no items are retained. Use `num_active_items() == 0` to test for zero retained items. * `ReqSketch` queries now use `datasketches::common::SearchCriteria` instead of `datasketches::req::SearchCriteria`. `SearchCriteria` no longer implements `Default`; explicitly choose `Inclusive` or `Exclusive`. * `ReqSketch` and `req::SortedView` queries now distinguish empty sketches from invalid arguments. `rank` returns `Option`; `quantile`, `quantiles`, `cdf`, and `pmf`, where available, return `Result, Error>`. Handle `None` for valid queries on an empty sketch, and propagate or handle `InvalidArgument` for invalid ranks or split points, which are checked before emptiness. +* `TDigest` and `TDigestMut` rank, quantile, CDF, and PMF queries now return `Result, Error>`. Invalid arguments return `InvalidArgument` instead of panicking, including on empty digests. Propagate or handle the `Result` before handling `None` for an empty digest; `min_value` and `max_value` still return `Option`. ### New features diff --git a/benchmarks/tdigest/aggregation.rs b/benchmarks/tdigest/aggregation.rs index da17069d..dd418235 100644 --- a/benchmarks/tdigest/aggregation.rs +++ b/benchmarks/tdigest/aggregation.rs @@ -127,4 +127,5 @@ fn final_quantile(mut merged: TDigestMut) -> Option { TDigestMut::deserialize(black_box(&bytes)) .unwrap() .quantile(black_box(0.9)) + .unwrap() } diff --git a/benchmarks/tdigest/compress.rs b/benchmarks/tdigest/compress.rs index 3fd4cb63..ead19006 100644 --- a/benchmarks/tdigest/compress.rs +++ b/benchmarks/tdigest/compress.rs @@ -31,14 +31,14 @@ fn initial_buffer(bencher: Bencher) { bencher .counter(ItemsCount::new(values.len())) .with_inputs(|| digest.clone()) - .bench_local_values(|mut digest| black_box(digest.rank(0.5))); + .bench_local_values(|mut digest| black_box(digest.rank(0.5).unwrap())); } #[divan::bench] fn unmerged_tail(bencher: Bencher) { let values = values(3_280); let mut digest = build_mut_digest(&values[..1_640]); - black_box(digest.rank(0.5)); + black_box(digest.rank(0.5).unwrap()); for &value in &values[1_640..] { digest.update(value); } @@ -46,7 +46,7 @@ fn unmerged_tail(bencher: Bencher) { bencher .counter(ItemsCount::new(1_640_usize)) .with_inputs(|| digest.clone()) - .bench_local_values(|mut digest| black_box(digest.rank(0.5))); + .bench_local_values(|mut digest| black_box(digest.rank(0.5).unwrap())); } #[divan::bench] diff --git a/benchmarks/tdigest/merge.rs b/benchmarks/tdigest/merge.rs index 12f6f8b0..d8ae2739 100644 --- a/benchmarks/tdigest/merge.rs +++ b/benchmarks/tdigest/merge.rs @@ -34,8 +34,8 @@ fn merge(bencher: Bencher) { let values = values(200_000); let mut left = build_mut_digest(&values[..100_000]); let mut right = build_mut_digest(&values[100_000..]); - black_box(left.rank(0.5)); - black_box(right.rank(0.5)); + black_box(left.rank(0.5).unwrap()); + black_box(right.rank(0.5).unwrap()); bencher .counter(ItemsCount::new(values.len())) @@ -65,7 +65,7 @@ fn small_partials(bencher: Bencher) { let partials = partial_digests(64, SMALL_ROWS_PER_PARTIAL) .into_iter() .map(|mut digest| { - black_box(digest.quantile(0.5)); + black_box(digest.quantile(0.5).unwrap()); digest }) .collect::>(); @@ -86,7 +86,7 @@ fn partials(bencher: Bencher) { let partials = partial_digests_with(DEFAULT_DIGEST_K, 64, ROWS_PER_PARTIAL) .into_iter() .map(|mut digest| { - black_box(digest.quantile(0.5)); + black_box(digest.quantile(0.5).unwrap()); digest }) .collect::>(); @@ -107,7 +107,7 @@ fn partials_from_iter(bencher: Bencher) { let partials = partial_digests_with(DEFAULT_DIGEST_K, 64, ROWS_PER_PARTIAL) .into_iter() .map(|mut digest| { - black_box(digest.quantile(0.5)); + black_box(digest.quantile(0.5).unwrap()); digest }) .collect::>(); @@ -125,7 +125,7 @@ fn partials_from_iter_cloned(bencher: Bencher) { let partials = partial_digests_with(DEFAULT_DIGEST_K, 64, ROWS_PER_PARTIAL) .into_iter() .map(|mut digest| { - black_box(digest.quantile(0.5)); + black_box(digest.quantile(0.5).unwrap()); digest }) .collect::>(); @@ -141,7 +141,7 @@ fn partials_from_iter_finalize(bencher: Bencher, additional_rows: usize) { let partials = partial_digests_with(DEFAULT_DIGEST_K, 64, ROWS_PER_PARTIAL) .into_iter() .map(|mut digest| { - black_box(digest.quantile(0.5)); + black_box(digest.quantile(0.5).unwrap()); digest }) .collect::>(); @@ -188,7 +188,7 @@ fn serialized_partials(bencher: Bencher, rows_per_partial: usize) { let partial = TDigestMut::deserialize(partial).unwrap(); merged.merge(&partial); } - black_box(merged.quantile(0.5)) + black_box(merged.quantile(0.5).unwrap()) }); } @@ -208,7 +208,7 @@ fn serialized_overlapping_partials(bencher: Bencher) { let partial = TDigestMut::deserialize(partial).unwrap(); merged.merge(&partial); } - black_box(merged.quantile(0.5)) + black_box(merged.quantile(0.5).unwrap()) }); } @@ -224,7 +224,7 @@ fn serialized_partials_from_iter(bencher: Bencher, overlapping: bool) { .map(|partial| TDigestMut::deserialize(partial)) .collect::>() .unwrap(); - black_box(merged.quantile(0.5)) + black_box(merged.quantile(0.5).unwrap()) }); } @@ -244,7 +244,7 @@ fn serialized_partials_in_batches(bencher: Bencher, overlapping: bool) { .unwrap(); merged.merge(&batch); } - black_box(merged.quantile(0.5)) + black_box(merged.quantile(0.5).unwrap()) }); } diff --git a/benchmarks/tdigest/query.rs b/benchmarks/tdigest/query.rs index 338179e8..76b05ac8 100644 --- a/benchmarks/tdigest/query.rs +++ b/benchmarks/tdigest/query.rs @@ -29,7 +29,7 @@ use super::support::prepared_digest; fn rank(bencher: Bencher) { let digest = prepared_digest(); - bencher.bench_local(|| black_box(&digest).rank(black_box(0.531_25))); + bencher.bench_local(|| black_box(&digest).rank(black_box(0.531_25)).unwrap()); } #[divan::bench(args = [0.0, 3.0, 3.5, 7.0])] @@ -37,14 +37,14 @@ fn rank_repeated_means(bencher: Bencher, value: f64) { let values = (0..100_000).map(|i| (i % 8) as f64).collect::>(); let digest = build_digest(&values); - bencher.bench_local(|| black_box(&digest).rank(black_box(value))); + bencher.bench_local(|| black_box(&digest).rank(black_box(value)).unwrap()); } #[divan::bench] fn quantile(bencher: Bencher) { let digest = prepared_digest(); - bencher.bench_local(|| black_box(&digest).quantile(black_box(0.531_25))); + bencher.bench_local(|| black_box(&digest).quantile(black_box(0.531_25)).unwrap()); } #[divan::bench] @@ -54,7 +54,7 @@ fn cdf_100(bencher: Bencher) { bencher .counter(ItemsCount::new(split_points.len())) - .bench_local(|| black_box(&digest).cdf(black_box(&split_points))); + .bench_local(|| black_box(&digest).cdf(black_box(&split_points)).unwrap()); } #[divan::bench(consts = [true, false], args = [2, 6, 100])] @@ -64,7 +64,7 @@ fn quantiles_sequential(bencher: Bencher, num_ranks: usize) bencher.counter(ItemsCount::new(num_ranks)).bench_local(|| { for &rank in black_box(&ranks) { - black_box(black_box(&digest).quantile(black_box(rank))); + black_box(black_box(&digest).quantile(black_box(rank)).unwrap()); } }); } @@ -76,7 +76,7 @@ fn quantiles_batch(bencher: Bencher, num_ranks: usize) { bencher .counter(ItemsCount::new(num_ranks)) - .bench_local(|| black_box(&digest).quantiles(black_box(&ranks))); + .bench_local(|| black_box(&digest).quantiles(black_box(&ranks)).unwrap()); } fn query_ranks(num_ranks: usize, sorted: bool) -> Vec { diff --git a/datasketches/src/error.rs b/datasketches/src/error.rs index 18a26100..24b57eb3 100644 --- a/datasketches/src/error.rs +++ b/datasketches/src/error.rs @@ -20,30 +20,72 @@ //! # Query results //! //! An empty sketch is valid. Queries with no meaningful result use `Option` to represent -//! absence; invalid caller arguments use `Result::Err`. A query that can encounter both returns +//! absence; invalid query arguments use `Result::Err`. A query that can encounter both returns //! `Result, Error>`: `Ok(None)` means no result, not an invalid request. //! -//! The following contract applies to KLL and REQ sketches and their sorted views, where exposed: +//! ## Queries without an empty-data result //! -//! | Query | Valid input, non-empty | Valid input, empty | Invalid input, any state | +//! The following contracts include each sketch's public sorted view, where the query is exposed: +//! +//! | Sketch | Query | Return type | Valid query on empty data | //! | --- | --- | --- | --- | -//! | `min_item`, `max_item` | `Some(item)` | `None` | No arguments | -//! | `rank` | `Some(rank)` | `None` | Items implement `Ord` | -//! | `quantile`, `quantiles`, `cdf`, `pmf` | `Ok(Some(result))` | `Ok(None)` | `Err(InvalidArgument)` | +//! | KLL, REQ | `min_item`, `max_item` | `Option<&T>` | `None` | +//! | KLL, REQ | `rank` | `Option` | `None` | +//! | KLL, REQ | `quantile`, `quantiles`, `cdf`, `pmf` | `Result, Error>` | `Ok(None)` | +//! | `TDigest`, `TDigestMut` | `min_value`, `max_value` | `Option` | `None` | +//! | `TDigest`, `TDigestMut` | `rank`, `quantile`, `quantiles`, `cdf`, `pmf` | `Result, Error>` | `Ok(None)` | //! -//! - **Validation comes first:** ranks must be in `[0.0, 1.0]` and not NaN; split points must be -//! strictly increasing. +//! - **Validation comes first:** quantile ranks must be in `[0.0, 1.0]` and not NaN; split points +//! must be strictly increasing. T-Digest also rejects NaN values passed to `rank` or used as +//! split points. Invalid arguments return [`ErrorKind::InvalidArgument`], even on empty data. +//! - **Ordering determines fallibility:** KLL and REQ rank queries accept `Ord` items, so they +//! return `Option` directly. T-Digest accepts `f64` and needs `Result` to reject NaN. Infinite +//! values are valid T-Digest rank arguments and split points, but not quantile ranks. //! - **Empty argument slices are valid:** on a non-empty sketch, `quantiles(&[])` returns //! `Ok(Some(vec![]))`, and `cdf(&[])` / `pmf(&[])` return `Ok(Some(vec![1.0]))`. On an empty -//! sketch, all return `Ok(None)`. -//! - **Caller migration:** handle `rank` as an `Option`; for fallible queries, use `?` to propagate -//! invalid arguments, then handle the `Option` for an empty sketch. -//! - **Errors remain distinct:** malformed serialized data returns [`ErrorKind::InvalidData`]; -//! broken internal invariants remain programming errors. -//! - **Error models still have results:** REQ `rank_lower_bound` / `rank_upper_bound` calculate -//! bounds around a supplied normalized rank. In exact mode, including an empty sketch, both -//! return that rank. KLL `normalized_rank_error` / `normalized_pmf_error` depend on -//! configuration. +//! sketch, all return `Ok(None)` because there is no distribution to query. +//! - **Caller migration:** handle infallible queries as an `Option`; for fallible queries, use `?` +//! to propagate invalid arguments, then handle the `Option` for an empty sketch. +//! +//! ## Queries with meaningful empty-data results +//! +//! These existing APIs keep their return types. A defined zero, false, or empty collection is +//! a result and does not need an additional `Option`. +//! +//! | Sketch or operator | Query | Empty-data result | +//! | --- | --- | --- | +//! | CPC and `CpcWrapper` | `estimate`, `lower_bound`, `upper_bound` | `0.0` | +//! | HLL and `HllUnion` | `estimate`, `composite_estimate`, `lower_bound`, `upper_bound` | `0.0` | +//! | Theta, Tuple, including compact sketches | `estimate`, `lower_bound`, `upper_bound` | `0.0` | +//! | Count-Min, Frequent Items | `estimate`, `lower_bound`, `upper_bound` | `0` | +//! | Frequent Items | `frequent_items`, `frequent_items_with_threshold` | Empty vector | +//! | Bloom | `contains`, `contains_and_insert` | `false` (before insertion for the latter) | +//! | Bloom | `bits_used`, `load_factor`, `estimated_fpp` | Zero | +//! | CPC, HLL, Theta, Tuple unions | `to_sketch` | Empty sketch | +//! | Theta, Tuple | `compact`, `iter` | Empty sketch or iterator | +//! | Theta, Tuple A-not-B | `compute` | Empty left input: empty result; empty right input: left input | +//! | KLL, REQ | `sorted_view`; REQ `iter` | Empty view or iterator | +//! | T-Digest | `freeze`, `unfreeze` | Empty digest | +//! | Theta, Tuple Jaccard | `compute`, `exactly_equal` | Both empty: similarity `1.0`, equal; exactly one empty: similarity `0.0`, unequal | +//! +//! Configuration, size, count, and state accessors remain defined on empty sketches. KLL +//! `normalized_rank_error` / `normalized_pmf_error`, Count-Min `relative_error`, and Frequent +//! Items `epsilon` describe configuration. Frequent Items `maximum_error` is zero when empty. +//! REQ `rank_lower_bound` / `rank_upper_bound` evaluate an error model around a supplied +//! normalized rank; in exact mode, including an empty sketch, both return that rank. +//! +//! Emptiness means logical emptiness, not merely zero retained samples. Theta and Tuple can +//! sample out every input, and Frequent Items can purge all retained items from a non-empty stream. +//! +//! ## Intersection initialization +//! +//! `ThetaIntersection::to_sketch` and `TupleIntersection::to_sketch` already return `Option`: +//! before the first successful update they return `None`, because no result has been established. +//! After an update they return `Some(sketch)`, including `Some(empty_sketch)` for an empty +//! intersection. Updating with an empty input also establishes a result. +//! +//! Malformed serialized data continues to return [`ErrorKind::InvalidData`]; broken internal +//! invariants remain programming errors. Absence of a query result needs no additional error kind. use std::fmt; diff --git a/datasketches/src/lib.rs b/datasketches/src/lib.rs index b4ff0164..9cc54bee 100644 --- a/datasketches/src/lib.rs +++ b/datasketches/src/lib.rs @@ -45,6 +45,8 @@ //! * Use `tuple` when retained Theta keys need application-defined summaries. //! //! See each module's documentation for accuracy, memory, serialization, and update examples. +//! See the [query contract](error#query-results) for empty results, invalid query arguments, and +//! the interfaces that retain meaningful values on empty sketches. //! //! ## Cross-language hashing //! diff --git a/datasketches/src/tdigest/mod.rs b/datasketches/src/tdigest/mod.rs index 0d466187..9cfea65d 100644 --- a/datasketches/src/tdigest/mod.rs +++ b/datasketches/src/tdigest/mod.rs @@ -48,6 +48,14 @@ //! //! [paper]: https://arxiv.org/abs/1902.04023 //! +//! # Query results +//! +//! Rank, quantile, CDF, and PMF queries return `Ok(None)` for an empty t-digest and +//! [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument) for invalid arguments, +//! even when empty. Minimum and maximum queries return `Option` directly. The same contract +//! applies to [`TDigestMut`] and [`TDigest`]. See the [query contract](crate::error#query-results) +//! for argument validation and migration guidance. +//! //! # Usage //! //! ``` @@ -56,9 +64,13 @@ //! let mut sketch = TDigestMut::new(100).unwrap(); //! sketch.update(1.0); //! sketch.update(2.0); -//! let median = sketch.quantile(0.5).unwrap(); +//! let Some(median) = sketch.quantile(0.5)? else { +//! return Ok(()); // No samples to query. +//! }; +//! assert!((1.0..=2.0).contains(&median)); //! let frozen = sketch.freeze(); -//! assert!(frozen.rank(2.0).is_some()); +//! assert!(frozen.rank(2.0)?.is_some()); +//! # Ok::<(), datasketches::error::Error>(()) //! ``` mod serialization; diff --git a/datasketches/src/tdigest/sketch.rs b/datasketches/src/tdigest/sketch.rs index 810c86be..5046dd28 100644 --- a/datasketches/src/tdigest/sketch.rs +++ b/datasketches/src/tdigest/sketch.rs @@ -403,10 +403,12 @@ impl TDigestMut { /// Returns the cumulative distribution approximation described by [`TDigest::cdf`]. /// - /// # Panics + /// Returns `Ok(None)` if this t-digest is empty. + /// + /// # Errors /// - /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` - /// values. + /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if + /// this t-digest is empty. /// /// # Examples /// @@ -417,25 +419,27 @@ impl TDigestMut { /// for value in [1.0, 2.0, 3.0] { /// sketch.update(value); /// } - /// let cdf = sketch.cdf(&[1.5]).unwrap(); + /// let cdf = sketch.cdf(&[1.5]).unwrap().unwrap(); /// assert_eq!(cdf.len(), 2); /// ``` - pub fn cdf(&mut self, split_points: &[f64]) -> Option> { - check_split_points(split_points); + pub fn cdf(&mut self, split_points: &[f64]) -> Result>, Error> { + check_split_points(split_points)?; if self.is_empty() { - return None; + return Ok(None); } - self.view().cdf(split_points) + Ok(self.view().cdf(split_points)) } /// Returns the probability mass approximation described by [`TDigest::pmf`]. /// - /// # Panics + /// Returns `Ok(None)` if this t-digest is empty. /// - /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` - /// values. + /// # Errors + /// + /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if + /// this t-digest is empty. /// /// # Examples /// @@ -446,24 +450,26 @@ impl TDigestMut { /// for value in [1.0, 2.0, 3.0] { /// sketch.update(value); /// } - /// let pmf = sketch.pmf(&[1.5]).unwrap(); + /// let pmf = sketch.pmf(&[1.5]).unwrap().unwrap(); /// assert_eq!(pmf.len(), 2); /// ``` - pub fn pmf(&mut self, split_points: &[f64]) -> Option> { - check_split_points(split_points); + pub fn pmf(&mut self, split_points: &[f64]) -> Result>, Error> { + check_split_points(split_points)?; if self.is_empty() { - return None; + return Ok(None); } - self.view().pmf(split_points) + Ok(self.view().pmf(split_points)) } /// Returns the normalized rank described by [`TDigest::rank`]. /// - /// # Panics + /// Returns `Ok(None)` if this t-digest is empty. + /// + /// # Errors /// - /// Panics if `value` is `NaN`. + /// Returns an error if `value` is NaN, even if this t-digest is empty. /// /// # Examples /// @@ -474,33 +480,37 @@ impl TDigestMut { /// for value in [1.0, 2.0, 3.0] { /// sketch.update(value); /// } - /// let rank = sketch.rank(2.0).unwrap(); + /// let rank = sketch.rank(2.0).unwrap().unwrap(); /// assert!((0.0..=1.0).contains(&rank)); /// ``` - pub fn rank(&mut self, value: f64) -> Option { - assert!(!value.is_nan(), "value must not be NaN"); + pub fn rank(&mut self, value: f64) -> Result, Error> { + if value.is_nan() { + return Err(Error::invalid_argument("value must not be NaN")); + } if self.is_empty() { - return None; + return Ok(None); } if value < self.min { - return Some(0.0); + return Ok(Some(0.0)); } if value > self.max { - return Some(1.0); + return Ok(Some(1.0)); } if self.min == self.max { - return Some(0.5); + return Ok(Some(0.5)); } - self.view().rank(value) + Ok(self.view().rank(value)) } /// Returns the quantile described by [`TDigest::quantile`]. /// - /// # Panics + /// Returns `Ok(None)` if this t-digest is empty. /// - /// Panics if `rank` is outside `[0.0, 1.0]`. + /// # Errors + /// + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. /// /// # Examples /// @@ -511,35 +521,35 @@ impl TDigestMut { /// for value in [1.0, 2.0, 3.0] { /// sketch.update(value); /// } - /// let median = sketch.quantile(0.5).unwrap(); + /// let median = sketch.quantile(0.5).unwrap().unwrap(); /// assert!((1.0..=3.0).contains(&median)); /// ``` - pub fn quantile(&mut self, rank: f64) -> Option { - assert!( - (0.0..=1.0).contains(&rank), - "rank must be in [0.0, 1.0]; got {rank}" - ); + pub fn quantile(&mut self, rank: f64) -> Result, Error> { + check_rank(rank)?; if self.is_empty() { - return None; + return Ok(None); } - self.view().quantile(rank) + Ok(self.view().quantile(rank)) } /// Returns the quantiles described by [`TDigest::quantiles`]. /// - /// # Panics + /// Returns `Ok(None)` if this t-digest is empty. + /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. /// - /// Panics if any rank is outside `[0.0, 1.0]`. - pub fn quantiles(&mut self, ranks: &[f64]) -> Option> { - check_ranks(ranks); + /// # Errors + /// + /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. + pub fn quantiles(&mut self, ranks: &[f64]) -> Result>, Error> { + check_ranks(ranks)?; if self.is_empty() { - return None; + return Ok(None); } - self.view().quantiles(ranks) + Ok(self.view().quantiles(ranks)) } /// Serializes this mutable t-digest to bytes. @@ -1427,14 +1437,15 @@ impl TDigest { /// stream given the split points. The value at array position j of the returned CDF array /// is the sum of the returned values in positions 0 through j of the returned PMF array. /// This can be viewed as array of ranks of the given split points plus one more value that - /// is always 1. An empty `split_points` slice returns the single value `[1.0]`. + /// is always 1. For a non-empty t-digest, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// - /// Returns `None` if this t-digest is empty. + /// Returns `Ok(None)` if this t-digest is empty. /// - /// # Panics + /// # Errors /// - /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` - /// values. + /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if + /// this t-digest is empty. /// /// # Examples /// @@ -1446,11 +1457,12 @@ impl TDigest { /// sketch.update(value); /// } /// let digest = sketch.freeze(); - /// let cdf = digest.cdf(&[1.5]).unwrap(); + /// let cdf = digest.cdf(&[1.5]).unwrap().unwrap(); /// assert_eq!(cdf.len(), 2); /// ``` - pub fn cdf(&self, split_points: &[f64]) -> Option> { - self.view().cdf(split_points) + pub fn cdf(&self, split_points: &[f64]) -> Result>, Error> { + check_split_points(split_points)?; + Ok(self.view().cdf(split_points)) } /// Returns an approximation to the Probability Mass Function (PMF) of the input stream @@ -1465,14 +1477,14 @@ impl TDigest { /// /// An array of m+1 doubles each of which is an approximation to the fraction of the input /// stream values (the mass) that fall into one of those intervals. - /// An empty `split_points` slice returns the single value `[1.0]`. + /// For a non-empty t-digest, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// - /// Returns `None` if this t-digest is empty. + /// Returns `Ok(None)` if this t-digest is empty. /// - /// # Panics + /// # Errors /// - /// Panics if `split_points` is not unique, not monotonically increasing, or contains `NaN` - /// values. + /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if + /// this t-digest is empty. /// /// # Examples /// @@ -1484,20 +1496,21 @@ impl TDigest { /// sketch.update(value); /// } /// let digest = sketch.freeze(); - /// let pmf = digest.pmf(&[1.5]).unwrap(); + /// let pmf = digest.pmf(&[1.5]).unwrap().unwrap(); /// assert_eq!(pmf.len(), 2); /// ``` - pub fn pmf(&self, split_points: &[f64]) -> Option> { - self.view().pmf(split_points) + pub fn pmf(&self, split_points: &[f64]) -> Result>, Error> { + check_split_points(split_points)?; + Ok(self.view().pmf(split_points)) } - /// Computes the approximate normalized rank in `[0.0, 1.0]` of the given value. + /// Returns the approximate normalized rank in `[0.0, 1.0]` of the given value. /// - /// Returns `None` if this t-digest is empty. + /// Returns `Ok(None)` if this t-digest is empty. /// - /// # Panics + /// # Errors /// - /// Panics if the value is `NaN`. + /// Returns an error if `value` is NaN, even if this t-digest is empty. /// /// # Examples /// @@ -1509,23 +1522,25 @@ impl TDigest { /// sketch.update(value); /// } /// let digest = sketch.freeze(); - /// let rank = digest.rank(2.0).unwrap(); + /// let rank = digest.rank(2.0).unwrap().unwrap(); /// assert!((0.0..=1.0).contains(&rank)); /// ``` - pub fn rank(&self, value: f64) -> Option { - assert!(!value.is_nan(), "value must not be NaN"); - self.view().rank(value) + pub fn rank(&self, value: f64) -> Result, Error> { + if value.is_nan() { + return Err(Error::invalid_argument("value must not be NaN")); + } + Ok(self.view().rank(value)) } - /// Computes the approximate quantile for the given normalized rank. + /// Returns the approximate quantile for the given normalized rank. /// /// Ranks `0.0` and `1.0` return the stored minimum and maximum, respectively. /// - /// Returns `None` if this t-digest is empty. + /// Returns `Ok(None)` if this t-digest is empty. /// - /// # Panics + /// # Errors /// - /// Panics if `rank` is outside `[0.0, 1.0]`. + /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. /// /// # Examples /// @@ -1537,24 +1552,25 @@ impl TDigest { /// sketch.update(value); /// } /// let digest = sketch.freeze(); - /// let q = digest.quantile(0.5).unwrap(); + /// let q = digest.quantile(0.5).unwrap().unwrap(); /// assert!((1.0..=3.0).contains(&q)); /// ``` - pub fn quantile(&self, rank: f64) -> Option { - assert!((0.0..=1.0).contains(&rank), "rank must be in [0.0, 1.0]"); - self.view().quantile(rank) + pub fn quantile(&self, rank: f64) -> Result, Error> { + check_rank(rank)?; + Ok(self.view().quantile(rank)) } - /// Computes approximate quantiles for the given normalized ranks. + /// Returns approximate quantiles for the given normalized ranks. /// /// Ranks in nondecreasing order are answered with one centroid scan. Ranks in any other order /// are accepted and results are returned in the same order as the input. /// - /// Returns `None` if this t-digest is empty. + /// Returns `Ok(None)` if this t-digest is empty. + /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. /// - /// # Panics + /// # Errors /// - /// Panics if any rank is outside `[0.0, 1.0]`. + /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. /// /// # Examples /// @@ -1566,12 +1582,12 @@ impl TDigest { /// sketch.update(value); /// } /// let digest = sketch.freeze(); - /// let quantiles = digest.quantiles(&[0.25, 0.5, 0.75]).unwrap(); + /// let quantiles = digest.quantiles(&[0.25, 0.5, 0.75]).unwrap().unwrap(); /// assert_eq!(quantiles.len(), 3); /// ``` - pub fn quantiles(&self, ranks: &[f64]) -> Option> { - check_ranks(ranks); - self.view().quantiles(ranks) + pub fn quantiles(&self, ranks: &[f64]) -> Result>, Error> { + check_ranks(ranks)?; + Ok(self.view().quantiles(ranks)) } /// Converts this immutable t-digest into a mutable one. @@ -1622,8 +1638,6 @@ impl TDigestView<'_> { } fn cdf(&self, split_points: &[f64]) -> Option> { - check_split_points(split_points); - if self.centroids.is_empty() { return None; } @@ -1896,22 +1910,34 @@ impl TDigestView<'_> { /// Checks the sequential validity of the given array of double values. /// They must be unique, monotonically increasing and not NaN. -#[track_caller] -fn check_split_points(split_points: &[f64]) { +fn check_split_points(split_points: &[f64]) -> Result<(), Error> { if split_points.iter().any(|split_point| split_point.is_nan()) { - panic!("split_points must not contain NaN values: {split_points:?}"); + return Err(Error::invalid_argument( + "split_points must not contain NaN values", + )); } if !split_points.windows(2).all(|pair| pair[0] < pair[1]) { - panic!("split_points must be unique and monotonically increasing: {split_points:?}"); + return Err(Error::invalid_argument( + "split_points must be unique and monotonically increasing", + )); } + Ok(()) } -#[track_caller] -fn check_ranks(ranks: &[f64]) { - assert!( - ranks.iter().all(|rank| (0.0..=1.0).contains(rank)), - "ranks must be in [0.0, 1.0]; got {ranks:?}" - ); +fn check_rank(rank: f64) -> Result<(), Error> { + if !(0.0..=1.0).contains(&rank) { + return Err(Error::invalid_argument(format!( + "rank must be in [0.0, 1.0], got {rank}" + ))); + } + Ok(()) +} + +fn check_ranks(ranks: &[f64]) -> Result<(), Error> { + for &rank in ranks { + check_rank(rank)?; + } + Ok(()) } fn centroid_cmp(a: &Centroid, b: &Centroid) -> Ordering { diff --git a/tests-integration/tests/serde_tests/tdigest.rs b/tests-integration/tests/serde_tests/tdigest.rs index b90b1ca6..8bcd9890 100644 --- a/tests-integration/tests/serde_tests/tdigest.rs +++ b/tests-integration/tests/serde_tests/tdigest.rs @@ -64,13 +64,17 @@ fn test_sketch_file(path: PathBuf, n: u64, with_buffer: bool, is_f32: bool) { assert_eq!(td.total_weight(), n, "filepath: {path}"); assert_eq!(td.min_value(), Some(1.0), "filepath: {path}"); assert_eq!(td.max_value(), Some(n as f64), "filepath: {path}"); - assert_eq!(td.rank(0.0), Some(0.0), "filepath: {path}"); - assert_eq!(td.rank((n + 1) as f64), Some(1.0), "filepath: {path}"); + assert_eq!(td.rank(0.0).unwrap(), Some(0.0), "filepath: {path}"); + assert_eq!( + td.rank((n + 1) as f64).unwrap(), + Some(1.0), + "filepath: {path}" + ); if n == 1 { - assert_eq!(td.rank(n as f64), Some(0.5), "filepath: {path}"); + assert_eq!(td.rank(n as f64).unwrap(), Some(0.5), "filepath: {path}"); } else { assert_that!( - td.rank(n as f64 / 2.).unwrap(), + td.rank(n as f64 / 2.).unwrap().unwrap(), near(0.5, 0.05), "filepath: {path}", ); @@ -126,23 +130,31 @@ fn test_deserialize_from_reference_implementation() { assert_eq!(td.total_weight(), n, "filepath: {path}"); assert_eq!(td.min_value(), Some(0.0), "filepath: {path}"); assert_eq!(td.max_value(), Some((n - 1) as f64), "filepath: {path}"); - assert_that!(td.rank(0.0).unwrap(), near(0.0, 0.0001), "filepath: {path}"); assert_that!( - td.rank(n as f64 / 4.).unwrap(), + td.rank(0.0).unwrap().unwrap(), + near(0.0, 0.0001), + "filepath: {path}" + ); + assert_that!( + td.rank(n as f64 / 4.).unwrap().unwrap(), near(0.25, 0.0001), "filepath: {path}" ); assert_that!( - td.rank(n as f64 / 2.).unwrap(), + td.rank(n as f64 / 2.).unwrap().unwrap(), near(0.5, 0.0001), "filepath: {path}" ); assert_that!( - td.rank((n * 3) as f64 / 4.).unwrap(), + td.rank((n * 3) as f64 / 4.).unwrap().unwrap(), near(0.75, 0.0001), "filepath: {path}" ); - assert_that!(td.rank(n as f64).unwrap(), eq(1.0), "filepath: {path}"); + assert_that!( + td.rank(n as f64).unwrap().unwrap(), + eq(1.0), + "filepath: {path}" + ); } } @@ -223,8 +235,14 @@ fn test_many_values() { assert_eq!(td.is_empty(), deserialized_td.is_empty()); assert_eq!(td.min_value(), deserialized_td.min_value()); assert_eq!(td.max_value(), deserialized_td.max_value()); - assert_eq!(td.rank(500.0), deserialized_td.rank(500.0)); - assert_eq!(td.quantile(0.5), deserialized_td.quantile(0.5)); + assert_eq!( + td.rank(500.0).unwrap(), + deserialized_td.rank(500.0).unwrap() + ); + assert_eq!( + td.quantile(0.5).unwrap(), + deserialized_td.quantile(0.5).unwrap() + ); } #[test] @@ -239,7 +257,10 @@ fn test_frozen_roundtrip() { assert_eq!(actual.total_weight(), expected.total_weight()); assert_eq!(actual.min_value(), expected.min_value()); assert_eq!(actual.max_value(), expected.max_value()); - assert_eq!(actual.quantile(0.5), expected.quantile(0.5)); + assert_eq!( + actual.quantile(0.5).unwrap(), + expected.quantile(0.5).unwrap() + ); } #[test] @@ -251,7 +272,7 @@ fn test_serialized_bytes_stable_for_full_and_merged_digests() { let mut left = patterned_digest(10, 201, 2); let mut right = patterned_digest(10, 199, 3); - right.rank(0.0); + right.rank(0.0).unwrap(); left.merge(&right); let bytes = left.serialize(); assert_eq!(bytes.len(), 272); @@ -394,7 +415,10 @@ fn test_deserialize_single_sample_requires_equal_extrema() { if min == max { let mut digest = result.unwrap(); assert_eq!(digest.total_weight(), 1); - assert_eq!(digest.quantiles(&[0., 0.5, 1.]), Some(vec![50.; 3])); + assert_eq!( + digest.quantiles(&[0., 0.5, 1.]).unwrap(), + Some(vec![50.; 3]) + ); } else { assert_eq!( result.unwrap_err().kind(), @@ -428,7 +452,10 @@ fn test_deserialize_compat_single_sample_requires_equal_extrema() { if min == max { let mut digest = TDigestMut::deserialize(&bytes).unwrap(); assert_eq!(digest.total_weight(), 1); - assert_eq!(digest.quantiles(&[0., 0.5, 1.]), Some(vec![50.; 3])); + assert_eq!( + digest.quantiles(&[0., 0.5, 1.]).unwrap(), + Some(vec![50.; 3]) + ); } else { assert_invalid_tdigest(&bytes); } @@ -504,6 +531,6 @@ fn test_large_weights_produce_finite_extreme_quantile() { bytes[56..64].copy_from_slice(&(1_u64 << 52).to_le_bytes()); let mut tdigest = TDigestMut::deserialize(&bytes).unwrap(); - let quantile = tdigest.quantile(0.25).unwrap(); + let quantile = tdigest.quantile(0.25).unwrap().unwrap(); assert_that!(quantile, all!(is_finite(), ge(lower), le(f64::MAX))); } diff --git a/tests-integration/tests/tdigest_test/property.rs b/tests-integration/tests/tdigest_test/property.rs index 61577efd..431da0ef 100644 --- a/tests-integration/tests/tdigest_test/property.rs +++ b/tests-integration/tests/tdigest_test/property.rs @@ -39,10 +39,10 @@ fn assert_quantiles_are_monotonic(tdigest: &mut TDigestMut) { let ranks = (0..=RANK_STEPS) .map(|step| step as f64 / RANK_STEPS as f64) .collect::>(); - let quantiles = tdigest.quantiles(&ranks).unwrap(); + let quantiles = tdigest.quantiles(&ranks).unwrap().unwrap(); for (&rank, quantile) in ranks.iter().zip(quantiles) { - assert_eq!(tdigest.quantile(rank), Some(quantile)); + assert_eq!(tdigest.quantile(rank).unwrap(), Some(quantile)); assert!( (previous..=max).contains(&quantile), "quantile {quantile} at rank {rank} is outside [{previous}, {max}]" @@ -77,12 +77,12 @@ fn prop_finite_values_survive_partial_aggregation() { // Use the input samples as split points, independently of the computed quantiles. values.sort_unstable_by(f64::total_cmp); values.dedup(); - assert_eq!(digest.quantile(0.), values.first().copied()); - assert_eq!(digest.quantile(1.), values.last().copied()); - let cdf = digest.cdf(&values).unwrap(); + assert_eq!(digest.quantile(0.).unwrap(), values.first().copied()); + assert_eq!(digest.quantile(1.).unwrap(), values.last().copied()); + let cdf = digest.cdf(&values).unwrap().unwrap(); assert!(cdf.is_sorted()); assert!(cdf.iter().all(|rank| (0.0..=1.0).contains(rank))); - let pmf = digest.pmf(&values).unwrap(); + let pmf = digest.pmf(&values).unwrap().unwrap(); assert!(pmf.iter().all(|p| (0.0..=1.0).contains(p))); assert!((pmf.iter().sum::() - 1.).abs() < 1e-12); TestResult::passed() @@ -159,12 +159,12 @@ fn prop_batch_quantiles_preserve_input_order_and_duplicates() { Some( ranks .iter() - .map(|&rank| frozen.quantile(rank).unwrap()) + .map(|&rank| frozen.quantile(rank).unwrap().unwrap()) .collect::>(), ) }; - assert_eq!(tdigest.clone().quantiles(&ranks), expected); - assert_eq!(frozen.quantiles(&ranks), expected); + assert_eq!(tdigest.clone().quantiles(&ranks).unwrap(), expected); + assert_eq!(frozen.quantiles(&ranks).unwrap(), expected); } } diff --git a/tests-integration/tests/tdigest_test/sketch.rs b/tests-integration/tests/tdigest_test/sketch.rs index b5bb67e4..86bc45fb 100644 --- a/tests-integration/tests/tdigest_test/sketch.rs +++ b/tests-integration/tests/tdigest_test/sketch.rs @@ -19,6 +19,7 @@ use std::mem::size_of; use std::panic::AssertUnwindSafe; use std::panic::catch_unwind; +use datasketches::error::ErrorKind; use datasketches::tdigest::TDigestMut; use googletest::assert_that; use googletest::prelude::eq; @@ -33,14 +34,16 @@ fn test_empty() { assert_eq!(tdigest.total_weight(), 0); assert_eq!(tdigest.min_value(), None); assert_eq!(tdigest.max_value(), None); - assert_eq!(tdigest.rank(0.0), None); - assert_eq!(tdigest.quantile(0.5), None); - assert_eq!(tdigest.quantiles(&[0.0, 0.5, 1.0]), None); - assert_eq!(tdigest.quantiles(&[]), None); + assert_eq!(tdigest.rank(0.0).unwrap(), None); + assert_eq!(tdigest.quantile(0.5).unwrap(), None); + assert_eq!(tdigest.quantiles(&[0.0, 0.5, 1.0]).unwrap(), None); + assert_eq!(tdigest.quantiles(&[]).unwrap(), None); let split_points = [0.0]; - assert_eq!(tdigest.pmf(&split_points), None); - assert_eq!(tdigest.cdf(&split_points), None); + assert_eq!(tdigest.pmf(&split_points).unwrap(), None); + assert_eq!(tdigest.cdf(&split_points).unwrap(), None); + assert_eq!(tdigest.pmf(&[]).unwrap(), None); + assert_eq!(tdigest.cdf(&[]).unwrap(), None); let tdigest = TDigestMut::new(10).unwrap().freeze(); assert!(tdigest.is_empty()); @@ -48,14 +51,16 @@ fn test_empty() { assert_eq!(tdigest.total_weight(), 0); assert_eq!(tdigest.min_value(), None); assert_eq!(tdigest.max_value(), None); - assert_eq!(tdigest.rank(0.0), None); - assert_eq!(tdigest.quantile(0.5), None); - assert_eq!(tdigest.quantiles(&[0.0, 0.5, 1.0]), None); - assert_eq!(tdigest.quantiles(&[]), None); + assert_eq!(tdigest.rank(0.0).unwrap(), None); + assert_eq!(tdigest.quantile(0.5).unwrap(), None); + assert_eq!(tdigest.quantiles(&[0.0, 0.5, 1.0]).unwrap(), None); + assert_eq!(tdigest.quantiles(&[]).unwrap(), None); let split_points = [0.0]; - assert_eq!(tdigest.pmf(&split_points), None); - assert_eq!(tdigest.cdf(&split_points), None); + assert_eq!(tdigest.pmf(&split_points).unwrap(), None); + assert_eq!(tdigest.cdf(&split_points).unwrap(), None); + assert_eq!(tdigest.pmf(&[]).unwrap(), None); + assert_eq!(tdigest.cdf(&[]).unwrap(), None); } #[test] @@ -66,31 +71,36 @@ fn test_one_value() { assert_eq!(tdigest.total_weight(), 1); assert_eq!(tdigest.min_value(), Some(1.0)); assert_eq!(tdigest.max_value(), Some(1.0)); - assert_eq!(tdigest.rank(0.99), Some(0.0)); - assert_eq!(tdigest.rank(1.0), Some(0.5)); - assert_eq!(tdigest.rank(1.01), Some(1.0)); - assert_eq!(tdigest.quantile(0.0), Some(1.0)); - assert_eq!(tdigest.quantile(0.5), Some(1.0)); - assert_eq!(tdigest.quantile(1.0), Some(1.0)); + assert_eq!(tdigest.rank(0.99).unwrap(), Some(0.0)); + assert_eq!(tdigest.rank(1.0).unwrap(), Some(0.5)); + assert_eq!(tdigest.rank(1.01).unwrap(), Some(1.0)); + assert_eq!(tdigest.quantile(0.0).unwrap(), Some(1.0)); + assert_eq!(tdigest.quantile(0.5).unwrap(), Some(1.0)); + assert_eq!(tdigest.quantile(1.0).unwrap(), Some(1.0)); let ranks = [1.0, 0.5, 0.0, 0.5]; - assert_eq!(tdigest.quantiles(&ranks), Some(vec![1.0; ranks.len()])); assert_eq!( - tdigest.freeze().quantiles(&ranks), + tdigest.quantiles(&ranks).unwrap(), + Some(vec![1.0; ranks.len()]) + ); + assert_eq!( + tdigest.freeze().quantiles(&ranks).unwrap(), Some(vec![1.0; ranks.len()]) ); } #[test] -fn test_empty_split_points_define_one_bin() { +fn test_empty_query_slices_on_nonempty_digest() { let mut tdigest = TDigestMut::new(100).unwrap(); tdigest.update(1.0); - assert_eq!(tdigest.cdf(&[]), Some(vec![1.0])); - assert_eq!(tdigest.pmf(&[]), Some(vec![1.0])); + assert_eq!(tdigest.cdf(&[]).unwrap(), Some(vec![1.0])); + assert_eq!(tdigest.pmf(&[]).unwrap(), Some(vec![1.0])); + assert_eq!(tdigest.quantiles(&[]).unwrap(), Some(vec![])); let tdigest = tdigest.freeze(); - assert_eq!(tdigest.cdf(&[]), Some(vec![1.0])); - assert_eq!(tdigest.pmf(&[]), Some(vec![1.0])); + assert_eq!(tdigest.cdf(&[]).unwrap(), Some(vec![1.0])); + assert_eq!(tdigest.pmf(&[]).unwrap(), Some(vec![1.0])); + assert_eq!(tdigest.quantiles(&[]).unwrap(), Some(vec![])); } #[test] @@ -100,7 +110,7 @@ fn test_maximum_k() { let tdigest = tdigest.freeze(); assert_eq!(tdigest.k(), u16::MAX); - assert_eq!(tdigest.quantile(0.5), Some(1.0)); + assert_eq!(tdigest.quantile(0.5).unwrap(), Some(1.0)); } #[test] @@ -118,14 +128,14 @@ fn test_estimated_size_reuses_buffer_after_compression() { } let size_before_compression = tdigest.estimated_size(); assert!(size_before_compression > inline_size); - tdigest.rank(0.5); + tdigest.rank(0.5).unwrap(); assert!(tdigest.estimated_size() <= size_before_compression); for value in MAX_UNMERGED..10_000 { tdigest.update(value as f64); } let size_before_compression = tdigest.estimated_size(); - tdigest.rank(0.5); + tdigest.rank(0.5).unwrap(); assert!(tdigest.estimated_size() <= size_before_compression); let mut left = TDigestMut::new(K).unwrap(); @@ -170,35 +180,41 @@ fn test_many_values() { assert_eq!(tdigest.min_value(), Some(0.0)); assert_eq!(tdigest.max_value(), Some((n - 1) as f64)); - assert_that!(tdigest.rank(0.0).unwrap(), near(0.0, 0.0001)); - assert_that!(tdigest.rank((n / 4) as f64).unwrap(), near(0.25, 0.0001)); - assert_that!(tdigest.rank((n / 2) as f64).unwrap(), near(0.5, 0.0001)); + assert_that!(tdigest.rank(0.0).unwrap().unwrap(), near(0.0, 0.0001)); + assert_that!( + tdigest.rank((n / 4) as f64).unwrap().unwrap(), + near(0.25, 0.0001) + ); + assert_that!( + tdigest.rank((n / 2) as f64).unwrap().unwrap(), + near(0.5, 0.0001) + ); assert_that!( - tdigest.rank((n * 3 / 4) as f64).unwrap(), + tdigest.rank((n * 3 / 4) as f64).unwrap().unwrap(), near(0.75, 0.0001) ); - assert_that!(tdigest.rank(n as f64).unwrap(), eq(1.0)); - assert_that!(tdigest.quantile(0.0).unwrap(), eq(0.0)); + assert_that!(tdigest.rank(n as f64).unwrap().unwrap(), eq(1.0)); + assert_that!(tdigest.quantile(0.0).unwrap().unwrap(), eq(0.0)); assert_that!( - tdigest.quantile(0.5).unwrap(), + tdigest.quantile(0.5).unwrap().unwrap(), near((n / 2) as f64, 0.03 * (n / 2) as f64) ); assert_that!( - tdigest.quantile(0.9).unwrap(), + tdigest.quantile(0.9).unwrap().unwrap(), near((n as f64) * 0.9, 0.01 * (n as f64) * 0.9) ); assert_that!( - tdigest.quantile(0.95).unwrap(), + tdigest.quantile(0.95).unwrap().unwrap(), near((n as f64) * 0.95, 0.01 * (n as f64) * 0.95) ); - assert_that!(tdigest.quantile(1.0).unwrap(), eq((n - 1) as f64)); + assert_that!(tdigest.quantile(1.0).unwrap().unwrap(), eq((n - 1) as f64)); let split_points = [n as f64 / 2.0]; - let pmf = tdigest.pmf(&split_points).unwrap(); + let pmf = tdigest.pmf(&split_points).unwrap().unwrap(); assert_eq!(pmf.len(), 2); assert_that!(pmf[0], near(0.5, 0.0001)); assert_that!(pmf[1], near(0.5, 0.0001)); - let cdf = tdigest.cdf(&split_points).unwrap(); + let cdf = tdigest.cdf(&split_points).unwrap().unwrap(); assert_eq!(cdf.len(), 2); assert_that!(cdf[0], near(0.5, 0.0001)); assert_that!(cdf[1], eq(1.0)); @@ -209,13 +225,13 @@ fn test_rank_two_values() { let mut tdigest = TDigestMut::new(100).unwrap(); tdigest.update(1.0); tdigest.update(2.0); - assert_eq!(tdigest.rank(0.99), Some(0.0)); - assert_eq!(tdigest.rank(1.0), Some(0.25)); - assert_eq!(tdigest.rank(1.25), Some(0.375)); - assert_eq!(tdigest.rank(1.5), Some(0.5)); - assert_eq!(tdigest.rank(1.75), Some(0.625)); - assert_eq!(tdigest.rank(2.0), Some(0.75)); - assert_eq!(tdigest.rank(2.01), Some(1.0)); + assert_eq!(tdigest.rank(0.99).unwrap(), Some(0.0)); + assert_eq!(tdigest.rank(1.0).unwrap(), Some(0.25)); + assert_eq!(tdigest.rank(1.25).unwrap(), Some(0.375)); + assert_eq!(tdigest.rank(1.5).unwrap(), Some(0.5)); + assert_eq!(tdigest.rank(1.75).unwrap(), Some(0.625)); + assert_eq!(tdigest.rank(2.0).unwrap(), Some(0.75)); + assert_eq!(tdigest.rank(2.01).unwrap(), Some(1.0)); } #[test] @@ -225,9 +241,9 @@ fn test_rank_repeated_values() { tdigest.update(1.0); tdigest.update(1.0); tdigest.update(1.0); - assert_eq!(tdigest.rank(0.99), Some(0.0)); - assert_eq!(tdigest.rank(1.0), Some(0.5)); - assert_eq!(tdigest.rank(1.01), Some(1.0)); + assert_eq!(tdigest.rank(0.99).unwrap(), Some(0.0)); + assert_eq!(tdigest.rank(1.0).unwrap(), Some(0.5)); + assert_eq!(tdigest.rank(1.01).unwrap(), Some(1.0)); } #[test] @@ -237,11 +253,11 @@ fn test_repeated_blocks() { tdigest.update(2.0); tdigest.update(2.0); tdigest.update(3.0); - assert_eq!(tdigest.rank(0.99), Some(0.0)); - assert_eq!(tdigest.rank(1.0), Some(0.125)); - assert_eq!(tdigest.rank(2.0), Some(0.5)); - assert_eq!(tdigest.rank(3.0), Some(0.875)); - assert_eq!(tdigest.rank(3.01), Some(1.0)); + assert_eq!(tdigest.rank(0.99).unwrap(), Some(0.0)); + assert_eq!(tdigest.rank(1.0).unwrap(), Some(0.125)); + assert_eq!(tdigest.rank(2.0).unwrap(), Some(0.5)); + assert_eq!(tdigest.rank(3.0).unwrap(), Some(0.875)); + assert_eq!(tdigest.rank(3.01).unwrap(), Some(1.0)); } #[test] @@ -266,13 +282,13 @@ fn test_rank_brackets_weighted_equal_mean_runs() { let expected = [0., 3., 7.5, 18., 32.5, 47., 64.].map(|mass| mass / 64.); let frozen = digest.clone().freeze(); for (&point, &rank) in points.iter().zip(&expected) { - assert_eq!(digest.rank(point), Some(rank)); - assert_eq!(frozen.rank(point), Some(rank)); + assert_eq!(digest.rank(point).unwrap(), Some(rank)); + assert_eq!(frozen.rank(point).unwrap(), Some(rank)); } let mut cdf = expected.to_vec(); cdf.push(1.); - assert_eq!(digest.cdf(&points), Some(cdf.clone())); - assert_eq!(frozen.cdf(&points), Some(cdf)); + assert_eq!(digest.cdf(&points).unwrap(), Some(cdf.clone())); + assert_eq!(frozen.cdf(&points).unwrap(), Some(cdf)); let pmf = vec![ 0., 3. / 64., @@ -283,8 +299,8 @@ fn test_rank_brackets_weighted_equal_mean_runs() { 17. / 64., 0., ]; - assert_eq!(digest.pmf(&points), Some(pmf.clone())); - assert_eq!(frozen.pmf(&points), Some(pmf)); + assert_eq!(digest.pmf(&points).unwrap(), Some(pmf.clone())); + assert_eq!(frozen.pmf(&points).unwrap(), Some(pmf)); } #[test] @@ -299,11 +315,11 @@ fn test_merge_small() { assert_eq!(td1.min_value(), Some(1.0)); assert_eq!(td1.max_value(), Some(3.0)); assert_eq!(td1.total_weight(), 4); - assert_eq!(td1.rank(0.99), Some(0.0)); - assert_eq!(td1.rank(1.0), Some(0.125)); - assert_eq!(td1.rank(2.0), Some(0.5)); - assert_eq!(td1.rank(3.0), Some(0.875)); - assert_eq!(td1.rank(3.01), Some(1.0)); + assert_eq!(td1.rank(0.99).unwrap(), Some(0.0)); + assert_eq!(td1.rank(1.0).unwrap(), Some(0.125)); + assert_eq!(td1.rank(2.0).unwrap(), Some(0.5)); + assert_eq!(td1.rank(3.0).unwrap(), Some(0.875)); + assert_eq!(td1.rank(3.01).unwrap(), Some(1.0)); } #[test] @@ -323,11 +339,20 @@ fn test_merge_large() { assert_eq!(td1.min_value(), Some(0.0)); assert_eq!(td1.max_value(), Some((n - 1) as f64)); - assert_that!(td1.rank(0.0).unwrap(), near(0.0, 0.0001)); - assert_that!(td1.rank((n / 4) as f64).unwrap(), near(0.25, 0.0001)); - assert_that!(td1.rank((n / 2) as f64).unwrap(), near(0.5, 0.0001)); - assert_that!(td1.rank((n * 3 / 4) as f64).unwrap(), near(0.75, 0.0001)); - assert_that!(td1.rank(n as f64).unwrap(), eq(1.0)); + assert_that!(td1.rank(0.0).unwrap().unwrap(), near(0.0, 0.0001)); + assert_that!( + td1.rank((n / 4) as f64).unwrap().unwrap(), + near(0.25, 0.0001) + ); + assert_that!( + td1.rank((n / 2) as f64).unwrap().unwrap(), + near(0.5, 0.0001) + ); + assert_that!( + td1.rank((n * 3 / 4) as f64).unwrap().unwrap(), + near(0.75, 0.0001) + ); + assert_that!(td1.rank(n as f64).unwrap().unwrap(), eq(1.0)); } #[test] @@ -346,8 +371,11 @@ fn test_mixed_k_merge_retains_receiver_k() { assert_eq!(left.k(), left_k); assert_eq!(left.total_weight(), 10_000 + right_count); - assert_eq!(left.quantile(0.0), Some(0.0)); - assert_eq!(left.quantile(1.0), Some((9_999 + right_count) as f64)); + assert_eq!(left.quantile(0.0).unwrap(), Some(0.0)); + assert_eq!( + left.quantile(1.0).unwrap(), + Some((9_999 + right_count) as f64) + ); } } @@ -360,8 +388,8 @@ fn test_from_iter_uses_one_result_with_the_smallest_nonempty_k() { first.update(value as f64); second.update((value + 1_000) as f64); } - let _ = first.quantile(0.5); - let _ = second.quantile(0.5); + let _ = first.quantile(0.5).unwrap(); + let _ = second.quantile(0.5).unwrap(); let mut merged = [first, empty, second].into_iter().collect::(); @@ -370,7 +398,7 @@ fn test_from_iter_uses_one_result_with_the_smallest_nonempty_k() { assert_eq!(merged.min_value(), Some(0.0)); assert_eq!(merged.max_value(), Some(1_999.0)); let quantiles = (0..=100) - .map(|rank| merged.quantile(rank as f64 / 100.).unwrap()) + .map(|rank| merged.quantile(rank as f64 / 100.).unwrap().unwrap()) .collect::>(); assert!(quantiles.windows(2).all(|pair| pair[0] <= pair[1])); } @@ -431,8 +459,8 @@ fn test_from_iter_matches_single_compression_for_interleaved_runs() { let mut merged = runs.into_iter().map(make_digest).collect::(); assert_eq!(merged.serialize(), expected.serialize()); - assert_eq!(merged.quantile(0.0), Some(-10.0)); - assert_eq!(merged.quantile(1.0), Some(20.0)); + assert_eq!(merged.quantile(0.0).unwrap(), Some(-10.0)); + assert_eq!(merged.quantile(1.0).unwrap(), Some(20.0)); // A collected digest must still support enough updates to grow and compress again. for value in 0..1_000 { @@ -455,11 +483,11 @@ fn test_from_iter_handles_mixed_compressed_and_buffered_inputs() { digest.update(value); expected.update(value); if run == 2 && row == 29 { - let _ = digest.quantile(0.5); + let _ = digest.quantile(0.5).unwrap(); } } if run == 1 { - let _ = digest.quantile(0.5); + let _ = digest.quantile(0.5).unwrap(); } digest }) @@ -506,12 +534,12 @@ fn test_serialized_batch_merge_tree_preserves_weight_and_quantiles() { let mut merged = TDigestMut::deserialize(&states[0]).unwrap(); assert_eq!(merged.total_weight(), values.len() as u64); - assert_eq!(merged.quantile(0.0), values.first().copied()); - assert_eq!(merged.quantile(1.0), values.last().copied()); + assert_eq!(merged.quantile(0.0).unwrap(), values.first().copied()); + assert_eq!(merged.quantile(1.0).unwrap(), values.last().copied()); let mut previous = values[0]; for rank in [0.01, 0.5, 0.9, 0.99] { - let estimate = merged.quantile(rank).unwrap(); + let estimate = merged.quantile(rank).unwrap().unwrap(); assert!((previous..=values[values.len() - 1]).contains(&estimate)); // Check this fixture's empirical rank, allowing equal values to span a rank interval. let lower = @@ -535,7 +563,7 @@ fn test_from_iter_checks_total_weight_before_compression() { let mut digest = TDigestMut::new(100).unwrap(); digest.update(value); if compressed { - let _ = digest.quantile(0.5); + let _ = digest.quantile(0.5).unwrap(); } digest }); @@ -543,8 +571,8 @@ fn test_from_iter_checks_total_weight_before_compression() { .into_iter() .collect::(); assert_eq!(at_limit.total_weight(), u64::MAX); - assert_eq!(at_limit.quantile(0.0), Some(0.0)); - assert_eq!(at_limit.quantile(1.0), Some(2.0)); + assert_eq!(at_limit.quantile(0.0).unwrap(), Some(0.0)); + assert_eq!(at_limit.quantile(1.0).unwrap(), Some(2.0)); assert!( catch_unwind(|| [heavy, one, two, extra].into_iter().collect::()).is_err() @@ -614,7 +642,7 @@ fn test_extreme_values_produce_finite_quantiles() { assert_eq!(tdigest.min_value(), Some(-f64::MAX)); assert_eq!(tdigest.max_value(), Some(f64::MAX)); for rank in [0.25, 0.5, 0.75] { - let quantile = tdigest.quantile(rank).unwrap(); + let quantile = tdigest.quantile(rank).unwrap().unwrap(); assert_that!(quantile, is_finite(), "quantile at rank {rank}"); } } @@ -630,7 +658,7 @@ fn test_rank_interpolation_across_extreme_finite_values() { let points = [-scale, -scale / 2., 0., scale / 2., scale]; for (point, expected) in points.into_iter().zip([0.25, 0.375, 0.5, 0.625, 0.75]) { assert_that!( - digest.rank(point).unwrap(), + digest.rank(point).unwrap().unwrap(), near(expected, 4. * f64::EPSILON) ); } @@ -647,11 +675,11 @@ fn test_tail_interpolation_across_extreme_finite_values() { for (digest, rank, expected) in [(left, 3. / 11., -0.25), (right, 8. / 11., 0.25)] { let digest = digest.freeze(); assert_that!( - digest.quantile(rank).unwrap() / max, + digest.quantile(rank).unwrap().unwrap() / max, near(expected, 4. * f64::EPSILON) ); assert_that!( - digest.rank(expected * max).unwrap(), + digest.rank(expected * max).unwrap().unwrap(), near(rank, 4. * f64::EPSILON) ); } @@ -663,7 +691,7 @@ fn test_estimate_repeat_values() { for _ in 0..20 { tdigest.update(1.0); } - assert_eq!(tdigest.quantile(0.9), Some(1.0)); + assert_eq!(tdigest.quantile(0.9).unwrap(), Some(1.0)); } /// Builds a weighted image, including tail centroids unavailable through raw updates. @@ -690,8 +718,8 @@ fn deserialize_with_centroids(k: u16, min: f64, max: f64, centroids: &[(f64, u64 fn assert_quantile_queries(tdigest: &TDigestMut, ranks: &[f64], expected: &[f64]) { let frozen = tdigest.clone().freeze(); for actual in [ - tdigest.clone().quantiles(ranks).unwrap(), - frozen.quantiles(ranks).unwrap(), + tdigest.clone().quantiles(ranks).unwrap().unwrap(), + frozen.quantiles(ranks).unwrap().unwrap(), ] { assert_eq!(actual.len(), expected.len()); for (actual, &expected) in actual.into_iter().zip(expected) { @@ -700,8 +728,14 @@ fn assert_quantile_queries(tdigest: &TDigestMut, ranks: &[f64], expected: &[f64] } let mut mutable = tdigest.clone(); for (&rank, &expected) in ranks.iter().zip(expected) { - assert_that!(mutable.quantile(rank).unwrap(), near(expected, 1e-12)); - assert_that!(frozen.quantile(rank).unwrap(), near(expected, 1e-12)); + assert_that!( + mutable.quantile(rank).unwrap().unwrap(), + near(expected, 1e-12) + ); + assert_that!( + frozen.quantile(rank).unwrap().unwrap(), + near(expected, 1e-12) + ); } } @@ -712,11 +746,26 @@ fn test_quantile_moves_toward_the_nearer_bracketing_centroid() { assert_eq!(tdigest.total_weight(), 12); // Ranks 2/12 and 6/12 sit exactly on the two centroids bracketing the first interval. - assert_that!(tdigest.quantile(2.0 / 12.0).unwrap(), near(0.0, 1e-12)); - assert_that!(tdigest.quantile(3.0 / 12.0).unwrap(), near(2.5, 1e-12)); - assert_that!(tdigest.quantile(4.0 / 12.0).unwrap(), near(5.0, 1e-12)); - assert_that!(tdigest.quantile(5.0 / 12.0).unwrap(), near(7.5, 1e-12)); - assert_that!(tdigest.quantile(6.0 / 12.0).unwrap(), near(10.0, 1e-12)); + assert_that!( + tdigest.quantile(2.0 / 12.0).unwrap().unwrap(), + near(0.0, 1e-12) + ); + assert_that!( + tdigest.quantile(3.0 / 12.0).unwrap().unwrap(), + near(2.5, 1e-12) + ); + assert_that!( + tdigest.quantile(4.0 / 12.0).unwrap().unwrap(), + near(5.0, 1e-12) + ); + assert_that!( + tdigest.quantile(5.0 / 12.0).unwrap().unwrap(), + near(7.5, 1e-12) + ); + assert_that!( + tdigest.quantile(6.0 / 12.0).unwrap().unwrap(), + near(10.0, 1e-12) + ); } #[test] @@ -725,11 +774,20 @@ fn test_quantile_right_tail_stays_within_max() { deserialize_with_centroids(100, 0.0, 100.0, &[(10.0, 10), (50.0, 10), (90.0, 10)]); assert_eq!(tdigest.max_value(), Some(100.0)); - assert_that!(tdigest.quantile(0.9).unwrap(), near(95.0, 1e-12)); - assert_that!(tdigest.quantile(29.0 / 30.0).unwrap(), near(100.0, 1e-12)); + assert_that!(tdigest.quantile(0.9).unwrap().unwrap(), near(95.0, 1e-12)); + assert_that!( + tdigest.quantile(29.0 / 30.0).unwrap().unwrap(), + near(100.0, 1e-12) + ); // Mirrors the left tail, which interpolates from min up to the first centroid mean. - assert_that!(tdigest.quantile(1.0 / 30.0).unwrap(), near(0.0, 1e-12)); - assert_that!(tdigest.quantile(5.0 / 30.0).unwrap(), near(10.0, 1e-12)); + assert_that!( + tdigest.quantile(1.0 / 30.0).unwrap().unwrap(), + near(0.0, 1e-12) + ); + assert_that!( + tdigest.quantile(5.0 / 30.0).unwrap().unwrap(), + near(10.0, 1e-12) + ); } #[test] @@ -737,7 +795,7 @@ fn test_quantile_handles_two_sample_last_centroid() { let mut tdigest = deserialize_with_centroids(100, 0.0, 100.0, &[(0.0, 1), (50.0, 1), (90.0, 2)]); - assert_eq!(tdigest.quantile(0.75), Some(100.0)); + assert_eq!(tdigest.quantile(0.75).unwrap(), Some(100.0)); } #[test] @@ -757,10 +815,13 @@ fn test_single_centroid_preserves_stored_tail_information() { // Repeated queries agree before and after creating a mutable view. for _ in 0..2 { for (&point, &rank) in points.iter().zip(&expected) { - assert_eq!(digest.rank(point), Some(rank)); - assert_eq!(frozen.rank(point), Some(rank)); + assert_eq!(digest.rank(point).unwrap(), Some(rank)); + assert_eq!(frozen.rank(point).unwrap(), Some(rank)); } - assert_eq!(&digest.cdf(&points).unwrap()[..points.len()], &expected); + assert_eq!( + &digest.cdf(&points).unwrap().unwrap()[..points.len()], + &expected + ); } } @@ -786,14 +847,14 @@ fn test_rank_tails_stay_monotonic_after_updating_weighted_centroids() { // The new unit-weight tails lie inside the retained extrema. Their half-sample // centers leave no mass for interpolation between min/first or last/max. for (&point, &expected) in points.iter().zip(&expected) { - assert_eq!(digest.rank(point), Some(expected)); - assert_eq!(frozen.rank(point), Some(expected)); + assert_eq!(digest.rank(point).unwrap(), Some(expected)); + assert_eq!(frozen.rank(point).unwrap(), Some(expected)); } - let cdf = digest.cdf(&points).unwrap(); - assert_eq!(cdf, frozen.cdf(&points).unwrap()); + let cdf = digest.cdf(&points).unwrap().unwrap(); + assert_eq!(cdf, frozen.cdf(&points).unwrap().unwrap()); assert!(cdf.is_sorted(), "CDF at {points:?}: {cdf:?}"); - let pmf = digest.pmf(&points).unwrap(); - assert_eq!(pmf, frozen.pmf(&points).unwrap()); + let pmf = digest.pmf(&points).unwrap().unwrap(); + assert_eq!(pmf, frozen.pmf(&points).unwrap().unwrap()); assert!(pmf.iter().all(|&mass| mass >= 0.), "PMF: {pmf:?}"); assert_that!(pmf.iter().sum::(), near(1., 1e-12)); } @@ -992,11 +1053,11 @@ fn test_signed_zero_and_symmetric_centroids_survive_interpolation() { let mut digest = deserialize_with_centroids(100, -1., 1., &[(-1., 1), (-0., 2), (0., 2), (1., 1)]); - assert_eq!(digest.rank(-0.), Some(0.5)); - assert_eq!(digest.rank(0.), Some(0.5)); + assert_eq!(digest.rank(-0.).unwrap(), Some(0.5)); + assert_eq!(digest.rank(0.).unwrap(), Some(0.5)); assert_quantile_queries(&digest, &[1. / 3., 0.5, 2. / 3.], &[0., 0., 0.]); - assert_eq!(digest.cdf(&[-0.]), Some(vec![0.5, 1.])); - assert_eq!(digest.pmf(&[0.]), Some(vec![0.5, 0.5])); + assert_eq!(digest.cdf(&[-0.]).unwrap(), Some(vec![0.5, 1.])); + assert_eq!(digest.pmf(&[0.]).unwrap(), Some(vec![0.5, 0.5])); } #[test] @@ -1015,11 +1076,14 @@ fn test_batch_quantiles_match_scalar_queries_in_input_order() { ] { let expected = ranks .iter() - .map(|&rank| frozen.quantile(rank).unwrap()) + .map(|&rank| frozen.quantile(rank).unwrap().unwrap()) .collect::>(); // Each mutable batch starts with the original buffered values still pending. - assert_eq!(tdigest.clone().quantiles(&ranks), Some(expected.clone())); - assert_eq!(frozen.quantiles(&ranks), Some(expected)); + assert_eq!( + tdigest.clone().quantiles(&ranks).unwrap(), + Some(expected.clone()) + ); + assert_eq!(frozen.quantiles(&ranks).unwrap(), Some(expected)); } } @@ -1109,9 +1173,11 @@ fn test_quantiles_stay_within_extrema_at_large_total_weights() { let frozen = tdigest.clone().freeze(); let ranks = [0.0, 0.5, 1.0_f64.next_down(), 1.0, 1.0]; for quantiles in [ - tdigest.clone().quantiles(&ranks).unwrap(), - frozen.quantiles(&ranks).unwrap(), - ranks.map(|rank| frozen.quantile(rank).unwrap()).to_vec(), + tdigest.clone().quantiles(&ranks).unwrap().unwrap(), + frozen.quantiles(&ranks).unwrap().unwrap(), + ranks + .map(|rank| frozen.quantile(rank).unwrap().unwrap()) + .to_vec(), ] { assert_eq!(quantiles[0], 0.0); assert_eq!(&quantiles[3..], &[100.0, 100.0]); @@ -1144,7 +1210,7 @@ fn test_queries_stay_ordered_at_large_centroid_centers() { 1.0_f64.next_down(), 1., ]; - let quantiles = digest.quantiles(&ranks).unwrap(); + let quantiles = digest.quantiles(&ranks).unwrap().unwrap(); assert_eq!(quantiles[0], 0.); assert_eq!(quantiles[5], 100.); assert!( @@ -1153,7 +1219,10 @@ fn test_queries_stay_ordered_at_large_centroid_centers() { ); assert!(quantiles.iter().all(|value| (0.0..=100.0).contains(value))); - let cdf = digest.cdf(&[0., 5., 10., 50., 90., 95., 100.]).unwrap(); + let cdf = digest + .cdf(&[0., 5., 10., 50., 90., 95., 100.]) + .unwrap() + .unwrap(); assert!(cdf.is_sorted(), "centroids {centroids:?}: {cdf:?}"); assert!(cdf.iter().all(|rank| (0.0..=1.0).contains(rank))); } @@ -1176,7 +1245,7 @@ fn test_quantile_right_tail_uses_the_same_center_as_rank() { ); // These ranks land exactly on the rounded mass N - last_weight/2. Quantile must // return the centroid mean there, before interpolating toward the stored maximum. - assert_eq!(digest.rank(90.), Some(rank)); + assert_eq!(digest.rank(90.).unwrap(), Some(rank)); assert_quantile_queries(&digest, &[rank, rank, 1.], &[90., 90., 100.]); } @@ -1186,7 +1255,7 @@ fn test_quantile_right_tail_uses_the_same_center_as_rank() { let digest = deserialize_with_centroids(100, 0., 100., &[(0., total_weight - 3), (90., 3)]); let rank = 1. - f64::EPSILON; let ranks = [rank.next_down(), rank, rank.next_up(), 1.]; - let quantiles = digest.clone().quantiles(&ranks).unwrap(); + let quantiles = digest.clone().quantiles(&ranks).unwrap().unwrap(); assert!(quantiles.is_sorted()); assert!(quantiles.iter().all(|value| (0.0..=100.0).contains(value))); assert_eq!(quantiles[3], 100.); @@ -1227,10 +1296,17 @@ fn test_rank_max_uses_the_same_rounded_mass_as_last_centroid() { deserialize_with_centroids(100, 0., 100., &[(0., total_weight - 1), (90., 1)]); let frozen = digest.clone().freeze(); let points = [90., 90_f64.next_up(), 95., 100_f64.next_down(), 100.]; - let cdf = digest.cdf(&points).unwrap(); + let cdf = digest.cdf(&points).unwrap().unwrap(); assert_eq!(&cdf[..points.len()], &[expected; 5]); - assert_eq!(cdf, frozen.cdf(&points).unwrap()); - assert!(frozen.pmf(&points).unwrap().iter().all(|&mass| mass >= 0.)); + assert_eq!(cdf, frozen.cdf(&points).unwrap().unwrap()); + assert!( + frozen + .pmf(&points) + .unwrap() + .unwrap() + .iter() + .all(|&mass| mass >= 0.) + ); } } @@ -1238,20 +1314,28 @@ fn test_rank_max_uses_the_same_rounded_mass_as_last_centroid() { fn test_rank_stays_monotonic_at_the_right_tail_boundary() { let digest = deserialize_with_centroids(100, 0., 300., &[(0., 1), (100., 29)]).freeze(); let points = [100., 100_f64.next_up(), 150., 300.]; - let cdf = digest.cdf(&points).unwrap(); + let cdf = digest.cdf(&points).unwrap().unwrap(); // The last center is at weight 1 + 29/2 = 15.5. Entering the tail must not lower its rank. assert_eq!(cdf[0], 15.5 / 30.); assert!(cdf.is_sorted(), "CDF at {points:?}: {cdf:?}"); } #[test] -fn test_batch_quantiles_reject_invalid_ranks() { - for values in [&[][..], &[1.0, 2.0, 3.0][..]] { +fn test_invalid_queries_return_errors_before_checking_emptiness() { + for n in [0, 1, 1_000] { let mut tdigest = TDigestMut::default(); - for &value in values { - tdigest.update(value); + for value in 0..n { + tdigest.update(f64::from(value)); } let frozen = tdigest.clone().freeze(); + assert_eq!( + tdigest.rank(f64::NAN).unwrap_err().kind(), + ErrorKind::InvalidArgument + ); + assert_eq!( + frozen.rank(f64::NAN).unwrap_err().kind(), + ErrorKind::InvalidArgument + ); for invalid in [ -f64::EPSILON, 1.0 + f64::EPSILON, @@ -1260,24 +1344,77 @@ fn test_batch_quantiles_reject_invalid_ranks() { f64::INFINITY, ] { let ranks = [0.5, invalid]; - assert!(catch_unwind(AssertUnwindSafe(|| tdigest.quantiles(&ranks))).is_err()); - assert!(catch_unwind(|| frozen.quantiles(&ranks)).is_err()); + for error in [ + tdigest.quantile(invalid).unwrap_err(), + frozen.quantile(invalid).unwrap_err(), + tdigest.quantiles(&ranks).unwrap_err(), + frozen.quantiles(&ranks).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } + } + for split_points in [ + &[f64::NAN][..], + &[0.0, f64::NAN], + &[f64::NAN, 0.0], + &[1.0, 1.0], + &[2.0, 1.0], + &[-0.0, 0.0], + &[f64::INFINITY, f64::INFINITY], + ] { + for error in [ + tdigest.cdf(split_points).unwrap_err(), + frozen.cdf(split_points).unwrap_err(), + tdigest.pmf(split_points).unwrap_err(), + frozen.pmf(split_points).unwrap_err(), + ] { + assert_eq!(error.kind(), ErrorKind::InvalidArgument); + } } } } +#[test] +fn test_infinite_values_are_valid_rank_and_split_point_arguments() { + for n in [0, 1, 1_000] { + let mut tdigest = TDigestMut::default(); + for value in 0..n { + tdigest.update(f64::from(value)); + } + let frozen = tdigest.clone().freeze(); + for (value, rank) in [(f64::NEG_INFINITY, 0.0), (f64::INFINITY, 1.0)] { + let expected = (n > 0).then_some(rank); + assert_eq!(tdigest.rank(value).unwrap(), expected); + assert_eq!(frozen.rank(value).unwrap(), expected); + } + let split_points = [f64::NEG_INFINITY, f64::INFINITY]; + let cdf = (n > 0).then(|| vec![0.0, 1.0, 1.0]); + let pmf = (n > 0).then(|| vec![0.0, 1.0, 0.0]); + assert_eq!(tdigest.cdf(&split_points).unwrap(), cdf); + assert_eq!(frozen.cdf(&split_points).unwrap(), cdf); + assert_eq!(tdigest.pmf(&split_points).unwrap(), pmf); + assert_eq!(frozen.pmf(&split_points).unwrap(), pmf); + } +} + #[test] fn test_rank_left_tail_is_a_fraction_of_the_total_weight() { let mut tdigest = deserialize_with_centroids(100, 0.0, 100.0, &[(10.0, 10), (50.0, 10), (90.0, 10)]); - assert_that!(tdigest.rank(5.0).unwrap(), near(0.1, 1e-12)); - assert_that!(tdigest.rank(10.0).unwrap(), near(5.0 / 30.0, 1e-12)); + assert_that!(tdigest.rank(5.0).unwrap().unwrap(), near(0.1, 1e-12)); + assert_that!( + tdigest.rank(10.0).unwrap().unwrap(), + near(5.0 / 30.0, 1e-12) + ); // The right tail is the mirror image and pins the scale the left tail must match. - assert_that!(tdigest.rank(95.0).unwrap(), near(0.9, 1e-12)); - assert_that!(tdigest.rank(90.0).unwrap(), near(25.0 / 30.0, 1e-12)); + assert_that!(tdigest.rank(95.0).unwrap().unwrap(), near(0.9, 1e-12)); + assert_that!( + tdigest.rank(90.0).unwrap().unwrap(), + near(25.0 / 30.0, 1e-12) + ); - let pmf = tdigest.pmf(&[5.0, 95.0]).unwrap(); + let pmf = tdigest.pmf(&[5.0, 95.0]).unwrap().unwrap(); assert_that!(pmf[0], near(0.1, 1e-12)); assert_that!(pmf[1], near(0.8, 1e-12)); assert_that!(pmf[2], near(0.1, 1e-12)); @@ -1295,20 +1432,20 @@ fn test_merge_preserves_min_max_from_other() { empty.merge(&other); assert_eq!(empty.min_value(), Some(0.0)); assert_eq!(empty.max_value(), Some(100.0)); - assert_eq!(empty.quantile(0.0), Some(0.0)); - assert_eq!(empty.quantile(1.0), Some(100.0)); - assert_eq!(empty.rank(0.0), Some(0.5 / 30.0)); - assert_eq!(empty.rank(100.0), Some(1.0 - 0.5 / 30.0)); - assert_eq!(empty.rank(-1.0), Some(0.0)); - assert_eq!(empty.rank(101.0), Some(1.0)); + assert_eq!(empty.quantile(0.0).unwrap(), Some(0.0)); + assert_eq!(empty.quantile(1.0).unwrap(), Some(100.0)); + assert_eq!(empty.rank(0.0).unwrap(), Some(0.5 / 30.0)); + assert_eq!(empty.rank(100.0).unwrap(), Some(1.0 - 0.5 / 30.0)); + assert_eq!(empty.rank(-1.0).unwrap(), Some(0.0)); + assert_eq!(empty.rank(101.0).unwrap(), Some(1.0)); let mut left = deserialize_with_centroids(100, 5.0, 40.0, &[(10.0, 4), (20.0, 4), (30.0, 4)]); let right = deserialize_with_centroids(100, -10.0, 80.0, &[(0.0, 4), (40.0, 4), (70.0, 4)]); left.merge(&right); assert_eq!(left.min_value(), Some(-10.0)); assert_eq!(left.max_value(), Some(80.0)); - assert_eq!(left.quantile(0.0), Some(-10.0)); - assert_eq!(left.quantile(1.0), Some(80.0)); + assert_eq!(left.quantile(0.0).unwrap(), Some(-10.0)); + assert_eq!(left.quantile(1.0).unwrap(), Some(80.0)); } #[test] From d938a1b0833fbeeb3944c7dc086adf412fe5c626 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 20:15:13 +0800 Subject: [PATCH 4/6] docs: keep query contracts on their methods Remove the crate-wide query inventory and repeated module summaries. Keep empty-result and argument-error contracts on the query methods, and record the shared design principles briefly in CONTRIBUTING. --- CONTRIBUTING.md | 6 +++ datasketches/src/error.rs | 70 ----------------------------- datasketches/src/kll/mod.rs | 7 --- datasketches/src/kll/sketch.rs | 17 ++++--- datasketches/src/kll/sorted_view.rs | 15 +++++-- datasketches/src/lib.rs | 2 - datasketches/src/req/mod.rs | 7 --- datasketches/src/req/sketch.rs | 25 ++++++----- datasketches/src/req/sorted_view.rs | 11 +++-- datasketches/src/tdigest/mod.rs | 8 ---- datasketches/src/tdigest/sketch.rs | 32 +++++++------ 11 files changed, 67 insertions(+), 133 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 841f8509..28c9cbc1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -80,6 +80,12 @@ cargo bench --package benchmarks --bench benchmarks -- cpc::serde - Describe types with noun phrases and API behavior with third-person present-tense verbs such as `Creates`, `Updates`, and `Returns`. - End summary sentences with punctuation, and format Rust identifiers, literals, and numeric ranges as inline code. - Put contract sections and compatibility notes before examples. When applicable, order sections as `# Errors`, `# Panics`, and `# Examples`. Include only sections that describe an actual contract. +- Document return values and error conditions on the relevant methods rather than repeating them in crate or module overviews. + +## Query semantics + +- Validate query arguments before handling an empty sketch. Invalid arguments return `InvalidArgument` regardless of whether the sketch is empty. +- Return a meaningful result when one exists, such as zero, `false`, or an empty collection. Otherwise, use `Option` for an absent result, combined with `Result` when argument validation can fail. ## Visibility diff --git a/datasketches/src/error.rs b/datasketches/src/error.rs index 24b57eb3..110b8c31 100644 --- a/datasketches/src/error.rs +++ b/datasketches/src/error.rs @@ -16,76 +16,6 @@ // under the License. //! Error types for DataSketches operations. -//! -//! # Query results -//! -//! An empty sketch is valid. Queries with no meaningful result use `Option` to represent -//! absence; invalid query arguments use `Result::Err`. A query that can encounter both returns -//! `Result, Error>`: `Ok(None)` means no result, not an invalid request. -//! -//! ## Queries without an empty-data result -//! -//! The following contracts include each sketch's public sorted view, where the query is exposed: -//! -//! | Sketch | Query | Return type | Valid query on empty data | -//! | --- | --- | --- | --- | -//! | KLL, REQ | `min_item`, `max_item` | `Option<&T>` | `None` | -//! | KLL, REQ | `rank` | `Option` | `None` | -//! | KLL, REQ | `quantile`, `quantiles`, `cdf`, `pmf` | `Result, Error>` | `Ok(None)` | -//! | `TDigest`, `TDigestMut` | `min_value`, `max_value` | `Option` | `None` | -//! | `TDigest`, `TDigestMut` | `rank`, `quantile`, `quantiles`, `cdf`, `pmf` | `Result, Error>` | `Ok(None)` | -//! -//! - **Validation comes first:** quantile ranks must be in `[0.0, 1.0]` and not NaN; split points -//! must be strictly increasing. T-Digest also rejects NaN values passed to `rank` or used as -//! split points. Invalid arguments return [`ErrorKind::InvalidArgument`], even on empty data. -//! - **Ordering determines fallibility:** KLL and REQ rank queries accept `Ord` items, so they -//! return `Option` directly. T-Digest accepts `f64` and needs `Result` to reject NaN. Infinite -//! values are valid T-Digest rank arguments and split points, but not quantile ranks. -//! - **Empty argument slices are valid:** on a non-empty sketch, `quantiles(&[])` returns -//! `Ok(Some(vec![]))`, and `cdf(&[])` / `pmf(&[])` return `Ok(Some(vec![1.0]))`. On an empty -//! sketch, all return `Ok(None)` because there is no distribution to query. -//! - **Caller migration:** handle infallible queries as an `Option`; for fallible queries, use `?` -//! to propagate invalid arguments, then handle the `Option` for an empty sketch. -//! -//! ## Queries with meaningful empty-data results -//! -//! These existing APIs keep their return types. A defined zero, false, or empty collection is -//! a result and does not need an additional `Option`. -//! -//! | Sketch or operator | Query | Empty-data result | -//! | --- | --- | --- | -//! | CPC and `CpcWrapper` | `estimate`, `lower_bound`, `upper_bound` | `0.0` | -//! | HLL and `HllUnion` | `estimate`, `composite_estimate`, `lower_bound`, `upper_bound` | `0.0` | -//! | Theta, Tuple, including compact sketches | `estimate`, `lower_bound`, `upper_bound` | `0.0` | -//! | Count-Min, Frequent Items | `estimate`, `lower_bound`, `upper_bound` | `0` | -//! | Frequent Items | `frequent_items`, `frequent_items_with_threshold` | Empty vector | -//! | Bloom | `contains`, `contains_and_insert` | `false` (before insertion for the latter) | -//! | Bloom | `bits_used`, `load_factor`, `estimated_fpp` | Zero | -//! | CPC, HLL, Theta, Tuple unions | `to_sketch` | Empty sketch | -//! | Theta, Tuple | `compact`, `iter` | Empty sketch or iterator | -//! | Theta, Tuple A-not-B | `compute` | Empty left input: empty result; empty right input: left input | -//! | KLL, REQ | `sorted_view`; REQ `iter` | Empty view or iterator | -//! | T-Digest | `freeze`, `unfreeze` | Empty digest | -//! | Theta, Tuple Jaccard | `compute`, `exactly_equal` | Both empty: similarity `1.0`, equal; exactly one empty: similarity `0.0`, unequal | -//! -//! Configuration, size, count, and state accessors remain defined on empty sketches. KLL -//! `normalized_rank_error` / `normalized_pmf_error`, Count-Min `relative_error`, and Frequent -//! Items `epsilon` describe configuration. Frequent Items `maximum_error` is zero when empty. -//! REQ `rank_lower_bound` / `rank_upper_bound` evaluate an error model around a supplied -//! normalized rank; in exact mode, including an empty sketch, both return that rank. -//! -//! Emptiness means logical emptiness, not merely zero retained samples. Theta and Tuple can -//! sample out every input, and Frequent Items can purge all retained items from a non-empty stream. -//! -//! ## Intersection initialization -//! -//! `ThetaIntersection::to_sketch` and `TupleIntersection::to_sketch` already return `Option`: -//! before the first successful update they return `None`, because no result has been established. -//! After an update they return `Some(sketch)`, including `Some(empty_sketch)` for an empty -//! intersection. Updating with an empty input also establishes a result. -//! -//! Malformed serialized data continues to return [`ErrorKind::InvalidData`]; broken internal -//! invariants remain programming errors. Absence of a query result needs no additional error kind. use std::fmt; diff --git a/datasketches/src/kll/mod.rs b/datasketches/src/kll/mod.rs index 8cf77c9c..5d0f03cf 100644 --- a/datasketches/src/kll/mod.rs +++ b/datasketches/src/kll/mod.rs @@ -28,13 +28,6 @@ //! provides their ordinary numerical order. Custom ordering should be expressed with a newtype //! that implements [`Ord`], keeping the ordering semantics part of the item type. //! -//! # Query results -//! -//! Valid queries on an empty sketch return no result. Invalid ranks or split points return -//! [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument), even on an empty -//! sketch. See the [query contract](crate::error#query-results) for return -//! types and migration guidance. -//! //! # Usage //! //! ```rust diff --git a/datasketches/src/kll/sketch.rs b/datasketches/src/kll/sketch.rs index 13972db5..f0c6dd3a 100644 --- a/datasketches/src/kll/sketch.rs +++ b/datasketches/src/kll/sketch.rs @@ -230,7 +230,8 @@ impl KllSketch { /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is + /// empty. pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( @@ -253,12 +254,14 @@ impl KllSketch { /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. /// /// The sorted view is built once for the whole batch. /// /// # Errors /// - /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + /// Returns `InvalidArgument` if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is + /// empty. pub fn quantiles( &self, ranks: &[f64], @@ -270,11 +273,12 @@ impl KllSketch { /// Returns the approximate CDF for the given split points. /// /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the sketch is - /// empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the + /// sketch is empty. pub fn cdf( &self, split_points: &[T], @@ -286,11 +290,12 @@ impl KllSketch { /// Returns the approximate PMF for the given split points. /// /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the sketch is - /// empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the + /// sketch is empty. pub fn pmf( &self, split_points: &[T], diff --git a/datasketches/src/kll/sorted_view.rs b/datasketches/src/kll/sorted_view.rs index 482325bc..60cff46d 100644 --- a/datasketches/src/kll/sorted_view.rs +++ b/datasketches/src/kll/sorted_view.rs @@ -94,7 +94,8 @@ impl SortedView { /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is + /// empty. pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( @@ -133,10 +134,12 @@ impl SortedView { /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. /// Returns `Ok(None)` if the view is empty. + /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. /// /// # Errors /// - /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the view is empty. + /// Returns `InvalidArgument` if any rank is NaN or outside `[0.0, 1.0]`, even if the view is + /// empty. pub fn quantiles( &self, ranks: &[f64], @@ -165,10 +168,12 @@ impl SortedView { /// Returns the approximate cumulative distribution over `split_points`. /// /// Returns `Ok(None)` if the view is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the view is empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the view + /// is empty. pub fn cdf( &self, split_points: &[T], @@ -189,10 +194,12 @@ impl SortedView { /// Returns the approximate probability mass over `split_points`. /// /// Returns `Ok(None)` if the view is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the view is empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the view + /// is empty. pub fn pmf( &self, split_points: &[T], diff --git a/datasketches/src/lib.rs b/datasketches/src/lib.rs index 9cc54bee..b4ff0164 100644 --- a/datasketches/src/lib.rs +++ b/datasketches/src/lib.rs @@ -45,8 +45,6 @@ //! * Use `tuple` when retained Theta keys need application-defined summaries. //! //! See each module's documentation for accuracy, memory, serialization, and update examples. -//! See the [query contract](error#query-results) for empty results, invalid query arguments, and -//! the interfaces that retain meaningful values on empty sketches. //! //! ## Cross-language hashing //! diff --git a/datasketches/src/req/mod.rs b/datasketches/src/req/mod.rs index f479d2d6..40b787c0 100644 --- a/datasketches/src/req/mod.rs +++ b/datasketches/src/req/mod.rs @@ -32,13 +32,6 @@ //! Custom item types need only [`Clone`] and [`Ord`] for in-memory use. Serialization additionally //! requires [`ReqValue`]. //! -//! # Query results -//! -//! Rank, quantile, CDF, and PMF queries on an empty sketch return no result. Invalid ranks or -//! split points return [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument), -//! even on an empty sketch. See the [query contract](crate::error#query-results) for return types -//! and migration guidance. -//! //! # Example //! //! ``` diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index ef17e1e7..73f5d25f 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -204,7 +204,8 @@ where /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if the sketch is + /// empty. pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( @@ -227,12 +228,14 @@ where /// /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. /// /// The sorted view is built once and shared across all ranks. /// /// # Errors /// - /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is empty. + /// Returns `InvalidArgument` if any rank is NaN or outside `[0.0, 1.0]`, even if the sketch is + /// empty. pub fn quantiles( &self, ranks: &[f64], @@ -263,11 +266,12 @@ where /// Returns the Probability Mass Function over the given split points. /// /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the sketch is - /// empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the + /// sketch is empty. pub fn pmf( &self, split_points: &[T], @@ -279,11 +283,12 @@ where /// Returns the Cumulative Distribution Function over the given split points. /// /// Returns `Ok(None)` if the sketch is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the sketch is - /// empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the + /// sketch is empty. pub fn cdf( &self, split_points: &[T], @@ -294,10 +299,8 @@ where /// Returns an owned, sorted snapshot of the sketch's current state. /// - /// An empty sketch yields an empty view; valid queries on it return no result. The - /// view is independent of the sketch — it can be queried (and sent to other - /// threads) while the sketch keeps receiving updates, and it keeps answering - /// from the state it was taken at. + /// An empty sketch yields an empty view. The view can be queried or sent to other threads + /// while the sketch receives updates, preserving the state captured when it was created. /// /// Building the view costs `O(retained · log retained)`; rank and quantile queries /// then take `O(log retained)`. Prefer reusing one view for repeated quantile @@ -391,7 +394,6 @@ where /// Returns the lower bound for the normalized `rank` at the requested confidence level. /// - /// This evaluates the error model around the supplied rank without querying a sample. /// In exact mode, including an empty sketch, the bound equals `rank`. pub fn rank_lower_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_lower_bound( @@ -406,7 +408,6 @@ where /// Returns the upper bound for the normalized `rank` at the requested confidence level. /// - /// This evaluates the error model around the supplied rank without querying a sample. /// In exact mode, including an empty sketch, the bound equals `rank`. pub fn rank_upper_bound(&self, rank: f64, num_std_dev: NumStdDev) -> f64 { self.compute_rank_upper_bound( diff --git a/datasketches/src/req/sorted_view.rs b/datasketches/src/req/sorted_view.rs index 1558c6ba..a08133c4 100644 --- a/datasketches/src/req/sorted_view.rs +++ b/datasketches/src/req/sorted_view.rs @@ -116,7 +116,8 @@ where /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if the view is + /// empty. pub fn quantile(&self, rank: f64, criteria: SearchCriteria) -> Result, Error> { if !(0.0..=1.0).contains(&rank) { return Err(Error::invalid_argument(format!( @@ -156,10 +157,12 @@ where /// /// The result contains one more value than `split_points`. /// Returns `Ok(None)` if the view is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the view is empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the view + /// is empty. pub fn pmf( &self, split_points: &[T], @@ -178,10 +181,12 @@ where /// /// The result contains one more value than `split_points` and ends at `1.0`. /// Returns `Ok(None)` if the view is empty. + /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// - /// Returns an error if the split points are not strictly increasing, even if the view is empty. + /// Returns `InvalidArgument` if the split points are not strictly increasing, even if the view + /// is empty. pub fn cdf( &self, split_points: &[T], diff --git a/datasketches/src/tdigest/mod.rs b/datasketches/src/tdigest/mod.rs index 9cfea65d..8c5818b4 100644 --- a/datasketches/src/tdigest/mod.rs +++ b/datasketches/src/tdigest/mod.rs @@ -48,14 +48,6 @@ //! //! [paper]: https://arxiv.org/abs/1902.04023 //! -//! # Query results -//! -//! Rank, quantile, CDF, and PMF queries return `Ok(None)` for an empty t-digest and -//! [`ErrorKind::InvalidArgument`](crate::error::ErrorKind::InvalidArgument) for invalid arguments, -//! even when empty. Minimum and maximum queries return `Option` directly. The same contract -//! applies to [`TDigestMut`] and [`TDigest`]. See the [query contract](crate::error#query-results) -//! for argument validation and migration guidance. -//! //! # Usage //! //! ``` diff --git a/datasketches/src/tdigest/sketch.rs b/datasketches/src/tdigest/sketch.rs index 5046dd28..32b9fc4e 100644 --- a/datasketches/src/tdigest/sketch.rs +++ b/datasketches/src/tdigest/sketch.rs @@ -407,8 +407,8 @@ impl TDigestMut { /// /// # Errors /// - /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if - /// this t-digest is empty. + /// Returns `InvalidArgument` if `split_points` contains NaN or is not strictly increasing, even + /// if this t-digest is empty. /// /// # Examples /// @@ -438,8 +438,8 @@ impl TDigestMut { /// /// # Errors /// - /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if - /// this t-digest is empty. + /// Returns `InvalidArgument` if `split_points` contains NaN or is not strictly increasing, even + /// if this t-digest is empty. /// /// # Examples /// @@ -469,7 +469,7 @@ impl TDigestMut { /// /// # Errors /// - /// Returns an error if `value` is NaN, even if this t-digest is empty. + /// Returns `InvalidArgument` if `value` is NaN, even if this t-digest is empty. /// /// # Examples /// @@ -510,7 +510,8 @@ impl TDigestMut { /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is + /// empty. /// /// # Examples /// @@ -541,7 +542,8 @@ impl TDigestMut { /// /// # Errors /// - /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. + /// Returns `InvalidArgument` if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest + /// is empty. pub fn quantiles(&mut self, ranks: &[f64]) -> Result>, Error> { check_ranks(ranks)?; @@ -1444,8 +1446,8 @@ impl TDigest { /// /// # Errors /// - /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if - /// this t-digest is empty. + /// Returns `InvalidArgument` if `split_points` contains NaN or is not strictly increasing, even + /// if this t-digest is empty. /// /// # Examples /// @@ -1483,8 +1485,8 @@ impl TDigest { /// /// # Errors /// - /// Returns an error if `split_points` contains NaN or is not strictly increasing, even if - /// this t-digest is empty. + /// Returns `InvalidArgument` if `split_points` contains NaN or is not strictly increasing, even + /// if this t-digest is empty. /// /// # Examples /// @@ -1510,7 +1512,7 @@ impl TDigest { /// /// # Errors /// - /// Returns an error if `value` is NaN, even if this t-digest is empty. + /// Returns `InvalidArgument` if `value` is NaN, even if this t-digest is empty. /// /// # Examples /// @@ -1540,7 +1542,8 @@ impl TDigest { /// /// # Errors /// - /// Returns an error if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. + /// Returns `InvalidArgument` if `rank` is NaN or outside `[0.0, 1.0]`, even if this t-digest is + /// empty. /// /// # Examples /// @@ -1570,7 +1573,8 @@ impl TDigest { /// /// # Errors /// - /// Returns an error if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest is empty. + /// Returns `InvalidArgument` if any rank is NaN or outside `[0.0, 1.0]`, even if this t-digest + /// is empty. /// /// # Examples /// From 4e0ecc69d9ad5ba08681754e0a69a4f26c5e9b55 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 20:16:47 +0800 Subject: [PATCH 5/6] docs: show imports in doctest examples Make all hidden doctest imports visible and record the example convention. Keep trailing hidden Ok expressions that allow examples to use ?. --- CONTRIBUTING.md | 1 + datasketches/src/kll/mod.rs | 5 +++-- datasketches/src/kll/sketch.rs | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 28c9cbc1..8d9f5bad 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -81,6 +81,7 @@ cargo bench --package benchmarks --bench benchmarks -- cpc::serde - End summary sentences with punctuation, and format Rust identifiers, literals, and numeric ranges as inline code. - Put contract sections and compatibility notes before examples. When applicable, order sections as `# Errors`, `# Panics`, and `# Examples`. Include only sections that describe an actual contract. - Document return values and error conditions on the relevant methods rather than repeating them in crate or module overviews. +- Keep imports visible in doctests. A trailing hidden `Ok(...)` may be used to support `?`. ## Query semantics diff --git a/datasketches/src/kll/mod.rs b/datasketches/src/kll/mod.rs index 5d0f03cf..59e0fd62 100644 --- a/datasketches/src/kll/mod.rs +++ b/datasketches/src/kll/mod.rs @@ -31,8 +31,9 @@ //! # Usage //! //! ```rust -//! # use datasketches::common::SearchCriteria; -//! # use datasketches::kll::KllSketch; +//! use datasketches::common::SearchCriteria; +//! use datasketches::kll::KllSketch; +//! //! let mut sketch = KllSketch::::new(200)?; //! sketch.update(1); //! sketch.update(2); diff --git a/datasketches/src/kll/sketch.rs b/datasketches/src/kll/sketch.rs index f0c6dd3a..0ea9b451 100644 --- a/datasketches/src/kll/sketch.rs +++ b/datasketches/src/kll/sketch.rs @@ -79,7 +79,8 @@ impl KllSketch { /// # Examples /// /// ``` - /// # use datasketches::kll::KllSketch; + /// use datasketches::kll::KllSketch; + /// /// let sketch = KllSketch::::new(200).unwrap(); /// assert_eq!(sketch.k(), 200); /// ``` From 625c847a5daf1865bd0f2e77e0b57f37bc1b46b3 Mon Sep 17 00:00:00 2001 From: tison Date: Tue, 20 Oct 2026 21:01:47 +0800 Subject: [PATCH 6/6] fixup Signed-off-by: tison --- datasketches/src/kll/sketch.rs | 26 +++++++++++++++----------- datasketches/src/kll/sorted_view.rs | 18 +++++++++--------- datasketches/src/req/sketch.rs | 27 +++++++++++++-------------- datasketches/src/req/sorted_view.rs | 26 ++++++++++++-------------- datasketches/src/tdigest/sketch.rs | 8 ++++---- 5 files changed, 53 insertions(+), 52 deletions(-) diff --git a/datasketches/src/kll/sketch.rs b/datasketches/src/kll/sketch.rs index 0ea9b451..891c4bbf 100644 --- a/datasketches/src/kll/sketch.rs +++ b/datasketches/src/kll/sketch.rs @@ -123,12 +123,16 @@ impl KllSketch { self.levels.len() > 1 } - /// Returns the minimum item seen by the sketch, or `None` if it is empty. + /// Returns the minimum item seen by the sketch. + /// + /// Returns `None` if the sketch is empty. pub fn min_item(&self) -> Option<&T> { self.min_item.as_ref() } - /// Returns the maximum item seen by the sketch, or `None` if it is empty. + /// Returns the maximum item seen by the sketch. + /// + /// Returns `None` if the sketch is empty. pub fn max_item(&self) -> Option<&T> { self.max_item.as_ref() } @@ -226,8 +230,8 @@ impl KllSketch { /// Returns the quantile for the given normalized rank. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the sketch is empty. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the sketch is empty. /// /// # Errors /// @@ -253,9 +257,9 @@ impl KllSketch { /// Returns approximate quantiles for the given normalized ranks. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the sketch is empty. Otherwise, an empty `ranks` slice returns + /// `Ok(Some(vec![]))`. /// /// The sorted view is built once for the whole batch. /// @@ -273,8 +277,8 @@ impl KllSketch { /// Returns the approximate CDF for the given split points. /// - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the sketch is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// @@ -290,8 +294,8 @@ impl KllSketch { /// Returns the approximate PMF for the given split points. /// - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the sketch is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// diff --git a/datasketches/src/kll/sorted_view.rs b/datasketches/src/kll/sorted_view.rs index 60cff46d..35eba5d2 100644 --- a/datasketches/src/kll/sorted_view.rs +++ b/datasketches/src/kll/sorted_view.rs @@ -89,8 +89,8 @@ impl SortedView { /// Returns the approximate quantile for `rank`. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the view is empty. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the view is empty. /// /// # Errors /// @@ -132,9 +132,9 @@ impl SortedView { /// Returns approximate quantiles for all `ranks`. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the view is empty. - /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the view is empty. Otherwise, an empty `ranks` slice returns + /// `Ok(Some(vec![]))`. /// /// # Errors /// @@ -167,8 +167,8 @@ impl SortedView { /// Returns the approximate cumulative distribution over `split_points`. /// - /// Returns `Ok(None)` if the view is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the view is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// @@ -193,8 +193,8 @@ impl SortedView { /// Returns the approximate probability mass over `split_points`. /// - /// Returns `Ok(None)` if the view is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the view is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// diff --git a/datasketches/src/req/sketch.rs b/datasketches/src/req/sketch.rs index 73f5d25f..446906f1 100644 --- a/datasketches/src/req/sketch.rs +++ b/datasketches/src/req/sketch.rs @@ -176,9 +176,8 @@ where /// Returns the approximate rank of `item` in `[0.0, 1.0]`. /// - /// Computed directly from the retained items in a single `O(retained)` pass, - /// without building a sorted view. The result is identical to - /// [`SortedView::rank`] on [`Self::sorted_view`]. + /// Computed directly from the retained items in a single `O(retained)` pass, without building + /// a sorted view. The result is identical to [`SortedView::rank`] on [`Self::sorted_view`]. /// /// Returns `None` if the sketch is empty. pub fn rank(&self, item: &T, criteria: SearchCriteria) -> Option { @@ -196,8 +195,8 @@ where /// Returns the approximate quantile at the given normalized rank. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the sketch is empty. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the sketch is empty. /// /// For ranks strictly between `0.0` and `1.0`, builds a transient [`SortedView`]. /// For repeated queries at those ranks, take one snapshot with [`Self::sorted_view`]. @@ -226,9 +225,9 @@ where /// Returns approximate quantiles for the given normalized ranks. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the sketch is empty. Otherwise, an empty `ranks` slice returns + /// `Ok(Some(vec![]))`. /// /// The sorted view is built once and shared across all ranks. /// @@ -265,8 +264,8 @@ where /// Returns the Probability Mass Function over the given split points. /// - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the sketch is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// @@ -282,8 +281,8 @@ where /// Returns the Cumulative Distribution Function over the given split points. /// - /// Returns `Ok(None)` if the sketch is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// Returns `Ok(None)` if the sketch is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// @@ -304,8 +303,8 @@ where /// /// Building the view costs `O(retained · log retained)`; rank and quantile queries /// then take `O(log retained)`. Prefer reusing one view for repeated quantile - /// queries at ranks strictly between `0.0` and `1.0`, or repeated - /// [`Self::pmf`]/[`Self::cdf`] queries. + /// queries at ranks strictly between `0.0` and `1.0`, or repeated [`Self::pmf`]/[`Self::cdf`] + /// queries. pub fn sorted_view(&self) -> SortedView { let mut weighted_items = Vec::with_capacity(self.num_retained as usize); for compactor in &self.compactors { diff --git a/datasketches/src/req/sorted_view.rs b/datasketches/src/req/sorted_view.rs index a08133c4..78c4bc65 100644 --- a/datasketches/src/req/sorted_view.rs +++ b/datasketches/src/req/sorted_view.rs @@ -22,12 +22,11 @@ use crate::error::Error; /// An owned, sorted snapshot of a [`ReqSketch`](crate::req::ReqSketch). /// -/// Obtain one with [`ReqSketch::sorted_view`](crate::req::ReqSketch::sorted_view). -/// The view is independent of the sketch: it can be queried (and sent to other -/// threads) while the sketch keeps receiving updates, and it keeps answering -/// from the state it was taken at. Building it costs `O(retained · log retained)`; -/// each subsequent query is `O(log retained)`, so it is the right tool for -/// repeated quantile/rank queries. +/// Obtain one with [`ReqSketch::sorted_view`](crate::req::ReqSketch::sorted_view). The view is +/// independent of the sketch: it can be queried (and sent to other threads) while the sketch keeps +/// receiving updates, and it keeps answering from the state it was taken at. Building it costs +/// `O(retained · log retained)`; each subsequent query is `O(log retained)`, so it is the right +/// tool for repeated quantile/rank queries. #[derive(Debug, Clone)] pub struct SortedView { items: Vec, @@ -111,8 +110,8 @@ where /// Returns the approximate quantile at the given normalized rank. /// - /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. - /// Returns `Ok(None)` if the view is empty. + /// Ranks `0.0` and `1.0` return the exact minimum and maximum items of the stream. Returns + /// `Ok(None)` if the view is empty. /// /// # Errors /// @@ -155,9 +154,8 @@ where /// Returns the probability mass function (PMF) over the given split points. /// - /// The result contains one more value than `split_points`. - /// Returns `Ok(None)` if the view is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// The result contains one more value than `split_points`. Returns `Ok(None)` if the view is + /// empty. Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. /// /// # Errors /// @@ -179,9 +177,9 @@ where /// Returns the cumulative distribution function (CDF) over the given split points. /// - /// The result contains one more value than `split_points` and ends at `1.0`. - /// Returns `Ok(None)` if the view is empty. - /// Otherwise, an empty `split_points` slice returns `Ok(Some(vec![1.0]))`. + /// The result contains one more value than `split_points` and ends at `1.0`. Returns + /// `Ok(None)` if the view is empty. Otherwise, an empty `split_points` slice returns + /// `Ok(Some(vec![1.0]))`. /// /// # Errors /// diff --git a/datasketches/src/tdigest/sketch.rs b/datasketches/src/tdigest/sketch.rs index 32b9fc4e..d7599af3 100644 --- a/datasketches/src/tdigest/sketch.rs +++ b/datasketches/src/tdigest/sketch.rs @@ -537,8 +537,8 @@ impl TDigestMut { /// Returns the quantiles described by [`TDigest::quantiles`]. /// - /// Returns `Ok(None)` if this t-digest is empty. - /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. + /// Returns `Ok(None)` if this t-digest is empty. Otherwise, an empty `ranks` slice returns + /// `Ok(Some(vec![]))`. /// /// # Errors /// @@ -1568,8 +1568,8 @@ impl TDigest { /// Ranks in nondecreasing order are answered with one centroid scan. Ranks in any other order /// are accepted and results are returned in the same order as the input. /// - /// Returns `Ok(None)` if this t-digest is empty. - /// Otherwise, an empty `ranks` slice returns `Ok(Some(vec![]))`. + /// Returns `Ok(None)` if this t-digest is empty. Otherwise, an empty `ranks` slice returns + /// `Ok(Some(vec![]))`. /// /// # Errors ///