[stojkomilos]

What changed

kll_sketch, quantiles_sketch and req_sketch keep the exact min and max of the stream. But get_quantile(0) and get_quantile(1) read only the retained items, and compaction can drop the min and max from them. Then ranks 0 and 1 give a retained item, not the exact min and max:

kll_sketch<float> sketch(8);
sketch.update(0);
sketch.update(999);
for (int i = 1; i < 999; ++i) sketch.update(i);
sketch.get_min_item();   // 0
sketch.get_quantile(0);  // 23 (changes from run to run)
sketch.get_max_item();   // 999
sketch.get_quantile(1);  // 998

Since 6.0.0, datasketches-java returns the exact min and max here (apache/datasketches-java#547, IncludeMinMax). Its sorted views add the min and max back when the retained items do not hold them. So for the same data, the two libraries give different answers at ranks 0 and 1.

This PR ports that step to C++:

• New quantiles_sorted_view::include_min_max(), with the same arithmetic as Java. A missing min goes first, with cumulative weight 1. A missing max goes last, and the entry before it gives 1 of its weight to the max. The total weight does not change.
• get_sorted_view() of kll_sketch, quantiles_sketch and req_sketch calls it. So get_quantile(), get_rank(), get_PMF(), get_CDF() and the sorted view iterator all see the min and max, as in Java.
• REQ get_rank() now reads the cached sorted view, as in KLL, quantiles and Java ReqSketch. Before, it summed the weights in the compactors, so it did not see the min and max that only the view has. The view is built from the REQ compactors, so the REQ algorithm does not change: on master, the old get_rank() and the view agree in all 240,048 checks (HRA and LRA, k = 4, 12 and 50). This removes the only caller of req_compactor::compute_weight(), so it is removed.
• Cost of the REQ change: queries with no update in between are about 4x faster (k = 12: 204 ns to 54 ns per get_rank()). A get_rank() after each update() rebuilds the view and is about 20x slower (k = 12: 1.8 us to 36 us). That is the cost that get_quantile() already has after an update.

Behavior change: only ranks within 1/n of 0 or 1 give a different quantile, and a rank changes by at most 1/n. For example, the inclusive rank of a dropped min is now 1/n, not 0. These are the values that Java returns for the same data.

For non-arithmetic types, the view and the reference that get_quantile() returns now can point to the min and max in the sketch object, not only to its item buffer. So a move of the sketch now also makes them invalid. The docs of get_sorted_view(), get_quantile() and quantiles_sorted_view now say that, for non-arithmetic types, they must not be used after the sketch is updated, merged, assigned to, moved or destroyed.

How tested

Added

quantiles_sorted_view_test (3 tests), with the data of the Java IncludeMinMaxTest:

• Edge cases (3 separate tests): both missing, both retained, and one retained item. Each checks the entries and their cumulative weights.

kll_sketch_test, quantiles_sketch_test, req_sketch_test (8 tests): each builds 1000 sketches, adds the min and max first so compaction drops them, and requires at least one sketch that dropped one. The REQ tests run in HRA and LRA.

• Built by update (KLL float and string, quantiles float, REQ float and string = 5 tests): ranks 0 and 1 give the exact min and max. The float tests also check the Java ranks of a dropped min and max.
• After deserialize and merge (KLL, quantiles, REQ = 3 tests): the same checks on a deserialized sketch and on its merge.

Test strength: without the fix, the 5 KLL and quantiles tests fail in 1000 of 1000 runs, and the 3 REQ tests fail. With include_min_max() changed to do nothing, the sorted view tests fail, except "both retained".

This pull request and its description were written by Isaac.

[leerho]

Thanks for this. Matching Java at ranks 0 and 1 is worthwhile, and the arithmetic in include_min_max() matches IncludeMinMax. CI passes, and the common, KLL and classic quantiles tests also pass for me locally under ASan and UBSan.

Timing: this changes the results users get, so it belongs in a minor release, not in the bug-fix release we're finishing now. We're holding other PRs until that release is out.

There are two things to settle before merging.

1. Sorted view lifetime. As you noted, for non-arithmetic types the view now points at the min and max inside the sketch object, so a view held across a move of the sketch no longer works. That fits the existing rule, as in Java, that a sorted view shouldn't be used after the sketch changes, but in C++ that rule isn't written down, and the failure is silent. Could you add it to the get_sorted_view() documentation in both sketches, saying that for non-arithmetic types the view refers to the sketch's items and must not be used after the sketch is updated, merged, moved or destroyed?

2. REQ. With this PR, KLL and classic quantiles match Java, but REQ doesn't. I'd like to see the REQ follow-up you offered planned alongside this, so all three sketches change in the same release. Moving REQ get_rank() to the sorted view is a reasonable way to do it.

[stojkomilos]

Removed as it's now unused.