feat: add theta sketch (part 1) by ZENOTME · Pull Request #45 · apache/datasketches-rust

ZENOTME · 2025-12-28T12:50:47Z

Part 1 of #30:

add specific hash table ref from: https://github.com/apache/datasketches-cpp/blob/master/theta/include/theta_update_sketch_base_impl.hpp
add ThetaSketch

cc @notfilippo @Xuanwo @PsiACE @tisonkun

tisonkun · 2025-12-30T06:23:22Z

+/// Canonicalize double value for compatibility with Java
+fn canonical_double(value: f64) -> i64 {
+    if value == 0.0 {
+        0i64
+    } else if value.is_nan() {
+        0x7ff8000000000000i64 // Java's Double.doubleToLongBits() NaN value
+    } else {
+        value.to_bits() as i64
+    }
+}


We may reduce one branch by leveraging this code snippet - https://github.com/reem/rust-ordered-float/blame/25da208e3e6cca1a1f9b1fcfeaec9e53f6497fa0/src/lib.rs#L2172-L2212

tisonkun · 2025-12-30T06:23:37Z

+//! # Serialization
+//!
+//! Sketches can be serialized and deserialized while preserving all state, enabling
+//! cross-platform sketch exchange with other Apache DataSketches implementations
+//! (Java, C++, Python).


I tend to remove this section until we implement it.

tisonkun · 2025-12-30T06:23:47Z

+//! - **ThetaUnion**: For computing union of multiple theta sketches
+//! - **ThetaIntersection**: For computing intersection of multiple theta sketches


tisonkun · 2025-12-30T06:24:53Z

+    }
+
+    /// Get cardinality estimate
+    pub fn get_estimate(&self) -> f64 {


Suggested change

pub fn get_estimate(&self) -> f64 {

pub fn estimate(&self) -> f64 {

Similar to this comment - #44 (comment)

Will these resolve the errors?

Can you retry the workflows? Or shall I.

Nope. The style check should be resolved by running cargo x lint --fix locally. It's a formatting issue where there are extra blank lines.

Is that something you can do?

Can you retry the workflows? Or shall I.

No need. It's a permanent failure before a new commit applying the formatter.

Or does @ZENOTME have to do it?

No. I think with a write commit I can push a new commit to this PR but I'm yet to have one.

Currently, @ZENOTME would handle these comments and commit for updates and fixes.

tisonkun · 2025-12-30T06:50:06Z

+    /// 
+    /// # Panics
+    ///
+    /// If lg_k is not in range [MIN_LG_K, MAX_LG_K]


Suggested change

/// If lg_k is not in range [MIN_LG_K, MAX_LG_K]

/// If lg_k is not in range [5, 26]

Make it clear in the docs.

tisonkun · 2025-12-30T06:50:34Z

+    /// # Panics
+    ///
+    /// If lg_k is not in range [MIN_LG_K, MAX_LG_K]
+    pub fn set_lg_k(mut self, lg_k: u8) -> Self {


Suggested change

pub fn set_lg_k(mut self, lg_k: u8) -> Self {

pub fn lg_k(mut self, lg_k: u8) -> Self {

I suppose we can also drop the set_ prefix in a builder.

Ditto other methods.

tisonkun · 2025-12-30T06:51:39Z

+/// Default seed for hash function
+pub const DEFAULT_SEED: u64 = 9001;


Once #48 merged you can leverage the shared DEFAULT_UPDATE_SEED as other impls do.

tisonkun · 2025-12-30T07:03:10Z

@ZENOTME To be clear, you make some design decisions to mapping:

C++'s theta_update_sketch_base
Java's UpdatableThetaSketch

.. to this ThetaHashTable and wrap it into a ThetaSketch.

Since it's not a 1:1 mapping, may you describe the concept and struct mapping details?

I'm not quite familiar with ThetaSketch so I'd count on @leerho on verifying the design correctness.

ZENOTME · 2025-12-30T13:35:40Z

Since it's not a 1:1 mapping, may you describe the concept and struct mapping details?

In C++ and Java, the class hierarchy of the theta sketch looks like this:

- ThetaSketch
 |--- UpdateThetaSketch (extend with update method)
   |--- In Java, it has more implementation classes
 |--- CompactThetaSketch (extend with serialize/deserialize method)
   |--- ...

In Rust it doesn't have inheritance. So we only have two types:

- ThetaSketch: mutable
- CompactThetaSketch: mutable

For implementation, in C++, update_theta_sketch_alloc uses theta_update_sketch_base as a hash table. I don't know why it's called xxx_base, it looks like it doesn't have inheritance. And that's why I refer to this class and rename it as ThetaHashTable.

For implementation in Java, it has more than one implementation inheriting from UpdateThetaSketch: DirectQuickSelectSketch, ConcurrentDirectQuickSelectSketch, and HeapAlphaSketch... There are different ways to abstract the implementation: ThetaSketch or wrap it as Box internally. I'm not sure we should consider this now, and is there any good practice for this? @tisonkun

- add hash table for theta sketch - add ThetaSketch

leerho · 2025-12-30T14:53:57Z

Is this ready to merge?

ZENOTME · 2025-12-30T15:09:41Z

Is this ready to merge?

I have fixed the review above, and it's mergeable. cc @tisonkun

tisonkun · 2025-12-30T15:22:45Z

+            return false;
+        }
+
+        assert!(self.entries[index] == 0, "Entry should be empty");


Suggested change

assert!(self.entries[index] == 0, "Entry should be empty");

assert_eq!(self.entries[index], 0, "Entry should be empty");

tisonkun · 2025-12-30T15:24:32Z

+        assert!(
+            num_inserted == k as usize,
+            "Number of inserted entries should be equal to k."
+        );


Suggested change

assert!(

num_inserted == k as usize,

"Number of inserted entries should be equal to k."

);

assert_eq!(

num_inserted, k as usize,

"Number of inserted entries should be equal to k."

);

Ditto others in tests.

tisonkun · 2025-12-30T15:27:30Z

+/// Specific hash table for theta sketch
+///
+/// It maintain a array capacity max to [2 ** (lg_max_size)]:
+/// - Before it reach the max capacity, it will extend the array based on resize_factor.
+/// - After it reach the capacity bigger than 2 ** (lg_nom_size), every time the number of entries
+///   exceeds the threshold, it will rebuild the table: only keep the min 2 ** lg_nom_size entries
+///   and update the theta to the k-th smallest entry.


Suggested change

/// Specific hash table for theta sketch

///

/// It maintain a array capacity max to [2 ** (lg_max_size)]:

/// - Before it reach the max capacity, it will extend the array based on resize_factor.

/// - After it reach the capacity bigger than 2 ** (lg_nom_size), every time the number of entries

/// exceeds the threshold, it will rebuild the table: only keep the min 2 ** lg_nom_size entries

/// and update the theta to the k-th smallest entry.

/// Specific hash table for theta sketch

///

/// It maintains an array capacity max to 2^lg_max_size:

/// * Before it reaches the max capacity, it will extend the array based on resize_factor.

/// * After it reaches the capacity bigger than 2^lg_nom_size, every time the number of entries

/// exceeds the threshold, it will rebuild the table: only keep the min 2^lg_nom_size entries

/// and update the theta to the k-th smallest entry.

tisonkun · 2025-12-30T15:28:10Z

+
+    /// Find an entry in the hash table.
+    ///
+    /// Returns the index of the entry if found, otherwise None. The entry may has been inserted or


Suggested change

/// Returns the index of the entry if found, otherwise None. The entry may has been inserted or

/// Returns the index of the entry if found, otherwise None. The entry may have been inserted or

tisonkun · 2025-12-30T15:28:18Z

+
+    /// Find index in a given entries.
+    ///
+    /// Returns the index of the entry if found, otherwise None. The entry may has been inserted or


Suggested change

/// Returns the index of the entry if found, otherwise None. The entry may has been inserted or

/// Returns the index of the entry if found, otherwise None. The entry may have been inserted or

@ZENOTME seems the final missing one here.

tisonkun

LGTM.

Some style issues can be resolved here or as a follow-up.

ZENOTME · 2025-12-30T15:48:56Z

LGTM.

Some style issues can be resolved here or as a follow-up.

resolved

leerho · 2025-12-30T19:13:46Z

@ZENOTME, I saw this comment above:

In Rust it doesn't have inheritance. So we only have two types:

ThetaSketch: mutable //not really, but not sure how you implement it in Rust

CompactThetaSketch: mutable //this is not correct.

See the Quick Summary.

ZENOTME force-pushed the theta_sketch_p1 branch 2 times, most recently from 8f1e59f to 38c61a0 Compare December 29, 2025 15:00

tisonkun reviewed Dec 30, 2025

View reviewed changes

ZENOTME added 3 commits December 30, 2025 21:52

feat: add theta sketch (part 1)

7881aac

- add hash table for theta sketch - add ThetaSketch

refine some function name and comment

4d654e8

refine: use unified default seed DEFAULT_UPDATE_SEED

b2bcd76

ZENOTME force-pushed the theta_sketch_p1 branch from 38c61a0 to b2bcd76 Compare December 30, 2025 13:56

tisonkun reviewed Dec 30, 2025

View reviewed changes

tisonkun approved these changes Dec 30, 2025

View reviewed changes

refine some style

a726052

ZENOTME force-pushed the theta_sketch_p1 branch from c44408d to a726052 Compare December 30, 2025 15:55

leerho approved these changes Dec 30, 2025

View reviewed changes

leerho merged commit 0a8543e into apache:main Dec 30, 2025
9 checks passed

		//! - ThetaUnion: For computing union of multiple theta sketches
		//! - ThetaIntersection: For computing intersection of multiple theta sketches

	pub fn get_estimate(&self) -> f64 {
	pub fn estimate(&self) -> f64 {

	/// If lg_k is not in range [MIN_LG_K, MAX_LG_K]
	/// If lg_k is not in range [5, 26]

	pub fn set_lg_k(mut self, lg_k: u8) -> Self {
	pub fn lg_k(mut self, lg_k: u8) -> Self {

		/// Default seed for hash function
		pub const DEFAULT_SEED: u64 = 9001;

	assert!(self.entries[index] == 0, "Entry should be empty");
	assert_eq!(self.entries[index], 0, "Entry should be empty");

	/// Returns the index of the entry if found, otherwise None. The entry may has been inserted or
	/// Returns the index of the entry if found, otherwise None. The entry may have been inserted or

Conversation

ZENOTME commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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 Dec 30, 2025

Uh oh!

ZENOTME commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leerho commented Dec 30, 2025

Uh oh!

ZENOTME commented Dec 30, 2025

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 left a comment

Choose a reason for hiding this comment

Uh oh!

ZENOTME commented Dec 30, 2025

Uh oh!

Uh oh!

leerho commented Dec 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ZENOTME commented Dec 28, 2025 •

edited

Loading

ZENOTME commented Dec 30, 2025 •

edited

Loading