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.