feat(req): port of reqsketch-rs by pmcgleenon · Pull Request #204 · apache/datasketches-rust

pmcgleenon · 2026-08-18T13:43:42Z

This is a port of reqsketch-rs as discussed here #90. This implementation is influenced heavily by the apache datasketches C++ implementation.

Some things present in reqsketch-rs that are not part of this PR:

benchmarks, I left these out since I didn't see any other benchmarks. Maybe we can add some in a later PR for all the sketches
program build the plots rank error plots. There are a bunch of dependencies needed for this so I left it out. But I think it's a great visualization of the reqsketch algorithm and helps with validation of the results since the apache-datasketches project has similar plots and they can be easily compared to validate the results

@tisonkun FYI if you get a chance please take a look

tisonkun

Thanks for your contribution @pmcgleenon! I'll review this patch in this week.

tisonkun · 2026-08-19T16:14:52Z

+        min_pre_longs: 2,
+        max_pre_longs: 4,


From datasketches-java, these fields are:

REQ(17, "REQ", 1 /* minPreLongs */, 2 /* maxPreLongs */),

But we don't use these fields for REQSketch anyway.

tisonkun · 2026-08-19T16:15:38Z

This should put under serde_test and need not to repeat serialization_test_data helper.

tisonkun · 2026-08-19T16:16:52Z

+[dependencies]
+rand = { workspace = true, optional = true }


This can be relevant to #203.

cc @jeffoodchain

We may use rand as a starting point and conclude before the next release.

tisonkun · 2026-08-19T16:19:41Z

+rand = { workspace = true, optional = true }
+
 [dev-dependencies]
+approx = { workspace = true }


googletest provides:

https://docs.rs/googletest/latest/googletest/matchers/fn.near.html

https://docs.rs/googletest/latest/googletest/matchers/fn.approx_eq.html

.. and we may need one more test lib. But you can leave it to me to do a global alignment.

tisonkun · 2026-08-19T16:21:46Z

+    ///
+    /// NaN inputs are silently ignored for floating-point types, matching the behavior
+    /// of the Java reference implementation (`checkNaNUpdate`). This is intentional and
+    /// documented in the cross-language differences doc.


in the cross-language differences doc.

Seems not included in this PR. We may rewrite this doc comment a bit.

tisonkun · 2026-08-19T16:25:45Z

+    /// Sets the `k` parameter.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `k` is odd or outside `[MIN_K, MAX_K]`.
+    pub fn k(mut self, k: u16) -> Result<Self, Error> {
+        if !(MIN_K..=MAX_K).contains(&k) {
+            return Err(Error::invalid_argument(format!(
+                "k must be in [{}, {}], got {k}",
+                MIN_K, MAX_K
+            )));
+        }
+        if k % 2 != 0 {
+            return Err(Error::invalid_argument(format!("k must be even, got {k}")));
+        }
+        self.k = k;
+        Ok(self)
+    }


Other builders use a panic flavor on setters.

I may prefer to follow the same flavor and propose a new flavor if desired later.

Besides, once we check k here, we may not need to call ReqSketch::try_new on build which check the same conditions.

And we can have a ReqSketch::new as the panic version for try_new. The current no-param new can be inlined to default and only default.

tisonkun · 2026-08-19T16:26:27Z

+    pub fn rank_accuracy(mut self, ra: RankAccuracy) -> Self {
+        self.rank_accuracy = ra;
+        self
+    }


Suggested change

pub fn rank_accuracy(mut self, ra: RankAccuracy) -> Self {

self.rank_accuracy = ra;

self

}

pub fn rank_accuracy(mut self, rank_accuracy: RankAccuracy) -> Self {

self.rank_accuracy = rank_accuracy;

self

}

nit: public API would prefer explicit over abbr.

tisonkun · 2026-08-19T16:27:36Z

+impl<T: ReqValue + fmt::Display> fmt::Display for ReqSketch<T> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        writeln!(f, "REQ Sketch Summary:")?;
+        writeln!(f, "  k                : {}", self.k)?;
+        writeln!(f, "  rank accuracy    : {:?}", self.rank_accuracy)?;
+        writeln!(f, "  n                : {}", self.n)?;
+        writeln!(f, "  num retained     : {}", self.num_retained)?;
+        writeln!(f, "  num levels       : {}", self.compactors.len())?;
+        writeln!(f, "  estimation mode  : {}", self.is_estimation_mode())?;
+        if let (Some(min), Some(max)) = (&self.min_item, &self.max_item) {
+            writeln!(f, "  min item         : {min}")?;
+            writeln!(f, "  max item         : {max}")?;
+        }
+        Ok(())
+    }
+}


Good point. I wonder if we'd have a tracking issue to impl fmt::Display for all sketches when applicible. This is what datasketches-java does IMO, but the format can be considered once more.

cc @notfilippo @ariesdevil @ZENOTME

tisonkun · 2026-08-19T16:28:07Z

+/// Internally wraps a `ReqSketch` configured for union semantics. The C++
+/// equivalent is `req_union<T>`.


I see no req_union<T> in datasketches-cpp.

cc @AlexanderSaydakov @leerho You can review the ReqUnion design here and give some high-level design comments.

tisonkun · 2026-08-19T16:28:53Z

+    /// Creates a new union with default `k = 12` and `RankAccuracy::HighRank`.
+    pub fn new() -> Self {
+        Self {
+            inner: ReqSketch::new(),
+        }
+    }
+
+    /// Creates a new union with the given `k` and rank accuracy.
+    ///
+    /// # Errors
+    ///
+    /// Returns an error if `k` is invalid (see [`ReqSketch::try_new`]).
+    pub fn try_new(k: u16, rank_accuracy: RankAccuracy) -> Result<Self, Error> {
+        Ok(Self {
+            inner: ReqSketch::try_new(k, rank_accuracy)?,
+        })
+    }


Similarly, I'd prefer new to be a panic version of try_new and leave the no-param new as default.

tisonkun · 2026-08-19T16:35:10Z

Glanced at the surface and commented above. Below is a review comment from Codex. I'll dive into implementation details later this week. And I think it's OK to merge a starting implementation and iterate later. But let's discuss existing items first to decide when to converge.

[CODEX COMMENT STARTS]
I found two correctness issues that I believe need to be fixed before this can be merged.

Compactor::merge breaks the ordering invariant required by the wire format

In datasketches/src/req/compactor.rs, Compactor::merge appends the other compactor’s items and marks the result as unsorted. This is safe only while the sketch remains in memory, because rank() falls back to a linear scan.

However, the REQ wire format stores a sorted flag only for level 0. Higher levels are implicitly required to be sorted, and ReqSketch::deserialize consequently marks every level above 0 as sorted. A merged sketch can therefore serialize unsorted higher-level items, which are then binary-searched as if they were sorted after deserialization.

I reproduced this with two HRA sketches using k = 12, containing 1000..1072 and 0..72. After merging the latter into the former and performing a serialize/deserialize round trip, the restored sketch returned different results from its sorted view for the same rank query. In one run:

restored.rank(0.0, Inclusive)               = 0.097222...
restored.sorted_view().rank(0.0, Inclusive) = 0.013888...

This also means a merged image produced by Rust may be interpreted incorrectly by Java or C++.

Both reference implementations preserve ordering during compactor merge by sorting and performing an ordered merge. I suggest doing the same here, including validating that the two compactors have the same lg_weight.

Please also add a regression test that:

builds two estimation-mode sketches with reversed/disjoint value ranges;
merges them;
serializes and deserializes the result; and
verifies that direct rank queries remain equal to sorted-view rank queries.

Deserialization accepts impossible compactor states that later panic or violate public API contracts

The current validation checks section_size_raw and requires lg_weight < 64, but it does not validate several structural invariants controlled by the serialized input.

Two concrete examples:

An image with num_sections = 0 is accepted. Calling merge() on the resulting sketch then panics in ensure_enough_sections() when evaluating num_sections - 1.
A single-level image with lg_weight = 63 and n = 1 is accepted. Its inclusive rank query returns approximately 9.22e18, despite rank() promising a result in [0, 1].

A malformed serialized image should not be able to create a sketch for which safe public operations panic or return impossible results.

Before constructing the sketch, I suggest validating at least:

each compactor’s lg_weight equals its level index;
num_sections, section_size_raw, and state form a valid, non-zero compactor configuration;
the number of levels is representable by the u64 weight model;
sum(num_items * 2^level) == n;
levels declared as sorted are actually sorted; and
extrema and retained values are consistent and contain no invalid NaN state.

The relevant arithmetic should also use checked operations. Since these failures originate from malformed serialized input, they should return ErrorKind::InvalidData, not InvalidArgument.

Once these two issues are addressed, the existing algorithm and cross-language compatibility coverage will be on much firmer ground.
[CODEX COMMENT END]

pmcgleenon added 2 commits August 18, 2026 14:03

feat(req): port REQ sketch from reqsketch-rs

f15ec54

feat(req): cargo +nightly fmt --all

1f27cd6

pmcgleenon changed the title ~~REQ sketch : port of reqsketch-rs~~ feat REQ sketch : port of reqsketch-rs Aug 18, 2026

tisonkun changed the title ~~feat REQ sketch : port of reqsketch-rs~~ feat: REQ sketch port of reqsketch-rs Aug 18, 2026

pmcgleenon changed the title ~~feat: REQ sketch port of reqsketch-rs~~ feat: REQ sketch : port of reqsketch-rs Aug 18, 2026

tisonkun changed the title ~~feat: REQ sketch : port of reqsketch-rs~~ feat(req): port of reqsketch-rs Aug 18, 2026

tisonkun reviewed Aug 18, 2026

View reviewed changes

tisonkun reviewed Aug 19, 2026

View reviewed changes

Navigation Menu

Uh oh!

feat(req): port of reqsketch-rs - #204

feat(req): port of reqsketch-rs #204
pmcgleenon wants to merge 2 commits into
apache:mainfrom
pmcgleenon:feat-req

pmcgleenon commented Aug 18, 2026

Uh oh!

tisonkun left a comment

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun Aug 19, 2026

Uh oh!

tisonkun commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		/// Internally wraps a `ReqSketch` configured for union semantics. The C++
		/// equivalent is `req_union<T>`.

Uh oh!

Conversation

pmcgleenon commented Aug 18, 2026

Uh oh!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants