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.

[leerho]

I don't approve of this -- at least until I get a better understanding of why it is needed.

[leerho]

I'm not sure I understand the motivation for this. The role of the UpdateSketch is to allow updating while retaining as much accuracy as possible. Reducing the retained entries to K significantly reduces accuracy. The role of the CompactSketch is to maintain that accuracy in an immutable and smaller form.

Compared to the frequency of updates, obtaining a sketch in its compact form is normally far less frequent. Yet you complain that it is slow. That implies to me you must be doing this operation frequently. I am curious, why? Are you keeping a large number of reduced accuracy sketches around? If you subsequently merge these, your merge accuracy will be reduced also. Why in the world would you want to do that?

The motivation for reducing the result to K on the ThetaUnion::getResult() is for a whole different reason. After unioning a number of theta sketches and before obtaining any 'result' or answer from the union operation, the retained entries MUST be reduced to K (actually MinK) in order to preserve order insensitivity and accuracy of the merge operation itself. And that is the role of getResult(). Not doing that will result in a dramatic increase in the variance of the estimate, basically throwing the accuracy guarantees out the window.

This is why the ThetaUnion does not have a getEstimate(). To attempt to do that without reducing the retained entries to K first, would be a disaster. And why the ThetaSketch (in Java) does not provide a public trim() operation. I do note that there is a public trim() operation in the C++ version of theta_sketch.hpp, but I would argue that it is in the wrong place or should be exclusively used by the Union operations.

I'm not convinced that "getResult()" makes any sense for the UpdateSketch, because the UpdateSketch itself IS the result -- for subsequent set operations or for obtaining the current estimate.

[stojkomilos]

replying to @leerho

I'm not sure I understand the motivation for this. - As I explained above, the motivation is that the compacted sketches don't take more space than nominal_size elements, and also that the behaviour matches the union get_result.

Compared to the frequency of updates, obtaining a sketch in its compact form is normally far less frequent. Yet you complain that it is slow. That implies to me you must be doing this operation frequently. I am curious, why? Are you keeping a large number of reduced accuracy sketches around? - Yes, I am doing the compaction frequently, and I store lots of these compacted sketches at the same time. The reason is something I can not disclose as it is specific to the company I am working for.

If you subsequently merge these, your merge accuracy will be reduced also. - This I am not doing, although thanks for telling me this about the accuracy reduction.

I'm not convinced that "getResult()" makes any sense for the UpdateSketch, because the UpdateSketch itself IS the result -- for subsequent set operations or for obtaining the current estimate. - alright, then I can rename it to something other than getResult, perhaps getCompactTrimmed() or something?

TLDR - I claim that this does have a use case (proven by a example - use case for my company), and I can change the name to anything you want that would be more fitting, like getCompactTrimmed()

If you agree, can we reopen the PR?

[stojkomilos]

reply to @c-dickens

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? - As I mentioned, trim() + compact() is slow, as it does allocation of new memory and it's slower than the current implementation for other reasons also

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. - sure, we don't need parity, then we can rename the function from getResult (or get_result) to getCompactTrimmed()