Skip to content

feat(theta): add jaccard similarity#142

Open
hawkingrei wants to merge 9 commits into
apache:mainfrom
hawkingrei:feat/theta-jaccard-similarity
Open

feat(theta): add jaccard similarity#142
hawkingrei wants to merge 9 commits into
apache:mainfrom
hawkingrei:feat/theta-jaccard-similarity

Conversation

@hawkingrei

Copy link
Copy Markdown

Summary

  • Add ThetaJaccardSimilarity for Theta sketches.
  • Return lower bound, estimate, and upper bound for the Jaccard index.
  • Port core coverage from the C++ theta_jaccard_similarity_test.cpp cases.

Motivation

This closes one small P0 parity gap with apache/datasketches-cpp: Rust had Theta sketching and intersection support, but no Jaccard similarity API.

Implementation Notes

  • Reuses existing ThetaIntersection for the intersection side.
  • Builds the two-input union locally inside the Jaccard implementation without exposing a full ThetaUnion API in this PR.
  • Ports the C++ sampled-ratio bound approximation locally for Jaccard bounds.

Tests

  • cargo check -p datasketches --features theta
  • cargo test -p datasketches --features theta --test theta_jaccard_similarity_test
  • cargo test -p datasketches --features theta --lib --test theta_intersection_test --test theta_jaccard_similarity_test --test theta_sketch_test

Full cargo test -p datasketches --features theta currently reaches unrelated failures in theta_serialization_test because local datasketches/tests/serialization_test_data/... files are missing.

@tisonkun tisonkun requested a review from ZENOTME July 2, 2026 09:41
tisonkun added 2 commits July 2, 2026 18:13
Signed-off-by: tison <wander4096@gmail.com>
Comment thread datasketches/src/theta/jaccard_similarity.rs Outdated
Comment thread datasketches/src/theta/jaccard_similarity.rs Outdated
@ZENOTME

ZENOTME commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

We also miss exactly_equal but it's ok to postpone it at next PR.

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must not have a unit struct ThetaJaccardSimilarity for creating JaccardSimilarity.

Let me consider the API and review the helper functions location.

Typically, here are a few API issues that should be resolved:

  1. Should have JaccardSimilarity::between / JaccardSimilarity::between_with_seed, rather than ThetaJaccardSimilarity::jaccard / ThetaJaccardSimilarity::jaccard_with_seed.
  2. JaccardSimilarity should expose its internal fields with getter, not pub fields, because the fields are immutatble.

@hawkingrei hawkingrei marked this pull request as draft July 5, 2026 11:12
@hawkingrei

Copy link
Copy Markdown
Author

Addressed the API feedback from @tisonkun:

  • Removed the ThetaJaccardSimilarity unit struct.
  • Moved the public constructors to JaccardSimilarity::between(...) and JaccardSimilarity::between_with_seed(...).
  • Made JaccardSimilarity fields private and exposed immutable getters: lower_bound(), estimate(), and upper_bound().

The tests have been updated to use the new API.

@hawkingrei hawkingrei marked this pull request as ready for review July 5, 2026 14:55
@tisonkun

tisonkun commented Jul 6, 2026

Copy link
Copy Markdown
Member

@ZENOTME I have two questions:

  1. Java's ThetaSketch::getRetainedEntries has a valid param, but our CompactThetaSketch::num_retained doesn't. Could you describe a bit what the design difference between the Java impl and the Rust impl?

  2. This PR has a fake ThetaUnion impl. Did we have an equivalent ThetaUnion impl so far? I found it strange to have a pub(super) struct ThetaUnion;.

@ZENOTME

ZENOTME commented Jul 6, 2026

Copy link
Copy Markdown
Contributor
  1. Java's ThetaSketch::getRetainedEntries has a valid param, but our CompactThetaSketch::num_retained doesn't. Could you describe a bit what the design difference between the Java impl and the Rust impl?

Java has a specific update implementation, AlphaSketch, where theta can be reduced without immediately rebuilding the internal cache. This means some cached entries may become invalid under the new theta but remain in the cache until a later rebuild. Therefore, getRetainedEntries(true) returns the valid entries used for estimation, while getRetainedEntries(false) returns the raw internal cache count, including possibly invalid entries.
Personally, I do not think this is an ideal public API shape. num_retained / getRetainedEntries should mean the logical retained count used for estimation and set operations. If the raw cache count needs to be exposed, it would be clearer as a separate API.

  1. This PR has a fake ThetaUnion impl. Did we have an equivalent ThetaUnion impl so far? I found it strange to have a pub(super) struct ThetaUnion;.

We don't have ThetaUnion so far. I think it maybe better to implement it first.

I send a PR for it. #145 cc @tisonkun @hawkingrei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants