tisonkun commented on issue #266:
URL:
https://github.com/apache/datasketches-rust/issues/266#issuecomment-5510650977
After reviewing the public APIs across the current sketch families, I lean
toward **A: keep absence separate from caller errors**, with the return type
chosen according to the actual failure modes of each query rather than
mechanically applying `Result<Option<_>>` everywhere.
The repository-wide pattern supports that distinction. Most existing
`Result` APIs are justified: constructors validate configuration, deserializers
validate an external representation, and unions/merges reject incompatible
sketches. The main inconsistency is specifically in distribution queries: REQ
currently treats an empty sketch as `InvalidArgument`, while T-Digest uses
`Option` for emptiness but panics on invalid runtime parameters.
I suggest the following policy:
- `Option<T>` when the only non-success condition is that a valid sketch has
no result yet.
- `Result<T, Error>` when a result always exists for a non-empty sketch but
the caller can supply an invalid argument.
- `Result<Option<T>, Error>` when emptiness and invalid caller input are
independent conditions.
- `InvalidData` only at representation boundaries such as deserialization.
Once `deserialize` returns `Ok`, queries should not surface serialized-data
errors.
- Panics only for internal invariant failures that cannot be produced
through safe public APIs, not for ranks or split points that may reasonably
come from runtime input.
Applied concretely, this would likely mean:
```rust
// No independently invalid query argument.
fn rank(&self, item: &T, criteria: SearchCriteria) -> Option<f64>;
// Empty is ordinary absence; an invalid rank is a caller error.
fn quantile(&self, rank: f64, criteria: SearchCriteria)
-> Result<Option<T>, Error>;
fn cdf(&self, split_points: &[T], criteria: SearchCriteria)
-> Result<Option<Vec<f64>>, Error>;
```
T-Digest `rank` is slightly different from generic REQ rank because `NaN` is
an invalid input, so its natural shape is also `Result<Option<f64>, Error>`.
`min_value` and `max_value` should remain plain `Option`: that matches
collection-style Rust APIs and there is no separate invalid argument to report.
Arguments should be validated before checking whether the sketch is empty.
Otherwise the same invalid rank would return `None` on an empty sketch and
`Err(InvalidArgument)` after the first update, making validation depend on
unrelated state. With `Result<Option<_>>`, the deterministic contract is:
- invalid argument: `Err(InvalidArgument)`;
- valid argument plus empty sketch: `Ok(None)`;
- valid argument plus non-empty sketch: `Ok(Some(value))`.
The main cost of A is nested handling. In practice, `?` removes the `Result`
layer cleanly and the remaining `Option` is the state the caller genuinely
needs to handle:
```rust
let Some(value) = sketch.quantile(rank)? else {
// no observations yet
return Ok(...);
};
```
I would not recommend B as the primary model. `EmptySketch` would provide a
single return layer and mirror the Java/C++ exception distinction, but it would
make a normal, serializable, mergeable sketch state exceptional and enlarge the
crate-wide error taxonomy mainly to avoid one level of nesting. We may still
want separate kinds such as `IncompatibleSketch` or `CapacityExceeded`
elsewhere, but that is independent of query absence.
C keeps the current T-Digest API compact, but ranks and split points are
commonly supplied by users, configuration, or query engines. Requiring every
caller to duplicate prevalidation to avoid a process-level panic is a poor
library boundary. D is attractive for workloads that reuse validated ranks,
especially batch queries, but a public `NormalizedRank` plus generic
split-point wrappers adds substantial API surface. It can remain a future
ergonomic layer without being required to settle the base contract.
For scope, I would first apply this policy to distribution-query families
(T-Digest, REQ, and KLL) and to their sorted views. It should not be imposed
mechanically on unrelated families: for example, an empty membership sketch can
still answer membership queries, and an intersection that has not received its
first input has its own existing state semantics.
Since changing the existing REQ and T-Digest signatures is breaking, the
cleanest path before 1.0 is to make the change once in a coordinated release.
If an additive transition is needed first, T-Digest can gain checked `try_*`
query methods while retaining the documented panic wrappers temporarily. I
would avoid inventing asymmetric names such as `quantile_opt` for REQ solely to
postpone the breaking change; that would leave the inconsistency in the
long-term API.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
