Add update_theta_sketch::get_result() to trim to k in one pass

[stojkomilos]

What changed

Add get_result() to update_theta_sketch_alloc. It returns a compact_theta_sketch trimmed to at most the nominal size k (2^lg_k) in a single pass, without rebuilding the hash table.

Background: an update sketch's hash table retains up to ~15/16 * 2k entries between rebuilds, and compact() intentionally keeps all of them (extra entries below theta improve the estimate). To trim a result onk today, a caller does trim() then compact(), this is too slow and inneificent (does malloc, dealloc).

I named this function get_result() in reference to theta union (and likely intersectino) which gurantee to return a already trimmed result.

How tested

• get_result trims to k in one pass: builds an 8000-item sketch (retains more than k); asserts the default get_result() returns exactly k ordered entries matching trim() + compact(true) (same theta and retained set), and that get_result(false) returns the same trimmed set unordered.
• get_result on empty and below-k sketches: empty stays empty and ordered; a 100-item exact-mode sketch returns untrimmed with all entries, ordered by default.

Local run:

cmake --build build --target theta_test -j
./build/theta/test/theta_test "[theta_sketch]"
# All tests passed (85773 assertions in 34 test cases)

[proost]

Two problems

1. As you can see, Changing API need discussion first: https://github.com/apache/datasketches-cpp/blob/master/CONTRIBUTING.md#getting-your-proposed-changes-accepted

2. It is technically wrong.

TEST_CASE("theta sketch: get_result silently destroys exact mode", "[theta_sketch]") {
  const uint32_t k = 1 << theta_constants::DEFAULT_LG_K; // 4096

  const int n = 5000;
  update_theta_sketch sketch = update_theta_sketch::builder().build();
  for (int i = 0; i < n; i++) sketch.update(i);

  REQUIRE_FALSE(sketch.is_estimation_mode());
  REQUIRE(sketch.get_num_retained() == n);
  REQUIRE(sketch.get_num_retained() > k);
  REQUIRE(sketch.get_estimate() == Approx(5000.0));
  REQUIRE(sketch.get_lower_bound(2) == sketch.get_upper_bound(2));

  // --- control: compact() preserves exactness ---
  compact_theta_sketch exact = sketch.compact();
  REQUIRE_FALSE(exact.is_estimation_mode());
  REQUIRE(exact.get_num_retained() == n);
  REQUIRE(exact.get_lower_bound(2) == exact.get_upper_bound(2));

  // --- branch: get_result() trades the exact answer for an estimate ---
  compact_theta_sketch result = sketch.get_result();
  REQUIRE(result.is_estimation_mode());                       // was exact, now estimating
  REQUIRE(result.get_num_retained() == k);
  REQUIRE(result.get_theta64() < theta_constants::MAX_THETA);
  REQUIRE(result.get_estimate() == Approx(5000.0).epsilon(0.05));
  REQUIRE(result.get_lower_bound(2) < result.get_upper_bound(2));
  REQUIRE(result.get_lower_bound(2) < n);
  REQUIRE(result.get_upper_bound(2) > n);
}

[stojkomilos]

Two problems

1. As you can see, Changing API need discussion first: https://github.com/apache/datasketches-cpp/blob/master/CONTRIBUTING.md#getting-your-proposed-changes-accepted
2. It is technically wrong.

TEST_CASE("theta sketch: get_result silently destroys exact mode", "[theta_sketch]") {
  const uint32_t k = 1 << theta_constants::DEFAULT_LG_K; // 4096

  const int n = 5000;
  update_theta_sketch sketch = update_theta_sketch::builder().build();
  for (int i = 0; i < n; i++) sketch.update(i);

  REQUIRE_FALSE(sketch.is_estimation_mode());
  REQUIRE(sketch.get_num_retained() == n);
  REQUIRE(sketch.get_num_retained() > k);
  REQUIRE(sketch.get_estimate() == Approx(5000.0));
  REQUIRE(sketch.get_lower_bound(2) == sketch.get_upper_bound(2));

  // --- control: compact() preserves exactness ---
  compact_theta_sketch exact = sketch.compact();
  REQUIRE_FALSE(exact.is_estimation_mode());
  REQUIRE(exact.get_num_retained() == n);
  REQUIRE(exact.get_lower_bound(2) == exact.get_upper_bound(2));

  // --- branch: get_result() trades the exact answer for an estimate ---
  compact_theta_sketch result = sketch.get_result();
  REQUIRE(result.is_estimation_mode());                       // was exact, now estimating
  REQUIRE(result.get_num_retained() == k);
  REQUIRE(result.get_theta64() < theta_constants::MAX_THETA);
  REQUIRE(result.get_estimate() == Approx(5000.0).epsilon(0.05));
  REQUIRE(result.get_lower_bound(2) < result.get_upper_bound(2));
  REQUIRE(result.get_lower_bound(2) < n);
  REQUIRE(result.get_upper_bound(2) > n);
}

Thank you for the response. I will send a mail for the API discussion.

As for your 2. point, I don't understand your point? It is is estimate mode yes, that is intentional, that is the purpose (the purpose is to have 2^lg_k=nominal_size entires used, just like .get_result() for theta union will do, so it has the same behaviour for .get_result())

[proost]

@stojkomilos
Sorry, i missed I named this function get_result() in reference to theta union (and likely intersectino) which gurantee to return a already trimmed result. the line. I understand your point. Then can you add those intention to the doc string to the function too?

not calling to "shrink_to_fit" same reason too?

[stojkomilos]

@stojkomilos Sorry, i missed I named this function get_result() in reference to theta union (and likely intersectino) which gurantee to return a already trimmed result. the line. I understand your point. Then can you add those intention to the doc string to the function too?

not calling to "shrink_to_fit" same reason too?
@proost

Then can you add those intention to the doc string to the function too? - yes sure.

not calling to "shrink_to_fit" same reason too - yeah that would be a more descriptive name (or something like .trim_and_compact()), but I just wanted it to be called the same as union and intersection have it so we have parity. I mean we could name it shrink_to_fit and rename the unions .get_result into this aswell etc...? I guess this is discussion for the mailing list where I already sent a mail a few minutes ago to discuss this.

[stojkomilos]

@proost Since noone seems to have responded to the emailing list I sent about the API change, can we merge this? It's not a change to any existing API, it's just adding a new read-only method, so it won't mess it other peoples existing code paths.

[proost]

@apache/datasketches-committers
Hi! How do you think adding this API to theta sketch?

[c-dickens]

Thank you for the suggestion @stojkomilos and for the review @proost !

Could you explain more about the intended use-case? I think trim() + compact(true) already produces this sketch (your own test asserts the output matches it on theta and retained set) and doesn't require an api change?

Unsure we need union parity here because union clips its result to k hashes as its accumulating state has to stay bounded across arbitrarily many merges. An update sketch does not have this constraint. It holds up to ~15/16(2k) values between rebuilds, and compact() keeps all of them because relative error goes as 1/√(retained), not 1/√k — so trimming discards up to half the values and widens the bound.