From d43afe840269df6ddd7744ea32077a84301fa251 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Fri, 6 Nov 2026 12:38:55 +0000 Subject: [PATCH 1/5] Return the exact min and max at ranks 0 and 1 in KLL and quantiles sketches kll_sketch and quantiles_sketch keep the exact min and max, but get_quantile(0) and get_quantile(1) read only the retained items, which compaction can drop the min and max from. Since 6.0.0, datasketches-java adds them back in its sorted views (IncludeMinMax, apache/datasketches-java#547). Port that step: quantiles_sorted_view::include_min_max() adds a missing min or max as an entry of weight 1, taken from its neighbor, so the total weight does not change. kll_sketch and quantiles_sketch call it from get_sorted_view(). REQ does not change, because its get_rank() does not read the sorted view. The min and max tests build 1000 sketches and check the quantiles at ranks 0 and 1, also on a deserialized sketch and on its merge with a second sketch. Co-authored-by: Isaac --- common/include/quantiles_sorted_view.hpp | 3 + common/include/quantiles_sorted_view_impl.hpp | 14 +++ common/test/quantiles_sorted_view_test.cpp | 66 ++++++++++++++ kll/include/kll_sketch_impl.hpp | 1 + kll/test/kll_sketch_test.cpp | 90 +++++++++++++++++++ quantiles/include/quantiles_sketch_impl.hpp | 1 + quantiles/test/quantiles_sketch_test.cpp | 69 ++++++++++++++ 7 files changed, 244 insertions(+) diff --git a/common/include/quantiles_sorted_view.hpp b/common/include/quantiles_sorted_view.hpp index c3ab6af0..e016d801 100755 --- a/common/include/quantiles_sorted_view.hpp +++ b/common/include/quantiles_sorted_view.hpp @@ -52,6 +52,9 @@ class quantiles_sorted_view { /// @private void convert_to_cummulative(); + /// @private + void include_min_max(const T& min_item, const T& max_item); + class const_iterator; /** diff --git a/common/include/quantiles_sorted_view_impl.hpp b/common/include/quantiles_sorted_view_impl.hpp index 3663950f..c8520376 100755 --- a/common/include/quantiles_sorted_view_impl.hpp +++ b/common/include/quantiles_sorted_view_impl.hpp @@ -60,6 +60,20 @@ void quantiles_sorted_view::convert_to_cummulative() { } } +// Compaction can drop the min and max, but the sketch stores them exactly. Add each back as an entry +// of weight 1, taken from its neighbor, so ranks 0 and 1 return them (IncludeMinMax in Java). +template +void quantiles_sorted_view::include_min_max(const T& min_item, const T& max_item) { + const bool add_min = comparator_(min_item, deref_helper(entries_.front().first)); + const bool add_max = comparator_(deref_helper(entries_.back().first), max_item); + entries_.reserve(entries_.size() + (add_min ? 1 : 0) + (add_max ? 1 : 0)); + if (add_max) { + entries_.back().second = total_weight_ - 1; + entries_.push_back(Entry(ref_helper(max_item), total_weight_)); + } + if (add_min) entries_.insert(entries_.begin(), Entry(ref_helper(min_item), 1)); +} + template double quantiles_sorted_view::get_rank(const T& item, bool inclusive) const { if (entries_.empty()) throw std::runtime_error("operation is undefined for an empty sketch"); diff --git a/common/test/quantiles_sorted_view_test.cpp b/common/test/quantiles_sorted_view_test.cpp index 609408dd..938c4110 100644 --- a/common/test/quantiles_sorted_view_test.cpp +++ b/common/test/quantiles_sorted_view_test.cpp @@ -456,4 +456,70 @@ TEST_CASE("set 4", "sorted view") { REQUIRE(view.get_quantile(1, false) == 40); } +TEST_CASE("include min and max", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); + std::vector l1 {2, 4, 6, 7}; + view.add(l1.begin(), l1.end(), 2); + view.convert_to_cummulative(); + view.include_min_max(1, 8); + REQUIRE(view.size() == 6); + + const std::vector> expected {{1, 1}, {2, 2}, {4, 4}, {6, 6}, {7, 7}, {8, 8}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); + + REQUIRE(view.get_quantile(0, true) == 1); + REQUIRE(view.get_quantile(0, false) == 1); + REQUIRE(view.get_quantile(1, true) == 8); + REQUIRE(view.get_quantile(1, false) == 8); + + REQUIRE(view.get_rank(1, true) == 0.125); + REQUIRE(view.get_rank(8, false) == 0.875); +} + +TEST_CASE("include min and max already retained", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); + std::vector l1 {2, 4, 6, 7}; + view.add(l1.begin(), l1.end(), 2); + view.convert_to_cummulative(); + view.include_min_max(2, 7); + REQUIRE(view.size() == 4); + + const std::vector> expected {{2, 2}, {4, 4}, {6, 6}, {7, 8}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); +} + +TEST_CASE("include min and max with one retained item", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(1, std::less(), std::allocator()); + std::vector l2 {5}; + view.add(l2.begin(), l2.end(), 4); + view.convert_to_cummulative(); + view.include_min_max(1, 9); + REQUIRE(view.size() == 3); + + const std::vector> expected {{1, 1}, {5, 3}, {9, 4}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); + + REQUIRE(view.get_quantile(0) == 1); + REQUIRE(view.get_quantile(0.5) == 5); + REQUIRE(view.get_quantile(1) == 9); +} + } /* namespace datasketches */ diff --git a/kll/include/kll_sketch_impl.hpp b/kll/include/kll_sketch_impl.hpp index b12a39c8..42891261 100644 --- a/kll/include/kll_sketch_impl.hpp +++ b/kll/include/kll_sketch_impl.hpp @@ -775,6 +775,7 @@ quantiles_sorted_view kll_sketch::get_sorted_view() const { view.add(from, to, 1ULL << level); } view.convert_to_cummulative(); + if (!is_empty()) view.include_min_max(*min_item_, *max_item_); return view; } diff --git a/kll/test/kll_sketch_test.cpp b/kll/test/kll_sketch_test.cpp index b8d0b031..f6cd63ac 100644 --- a/kll/test/kll_sketch_test.cpp +++ b/kll/test/kll_sketch_test.cpp @@ -202,6 +202,96 @@ TEST_CASE("kll sketch", "[kll_sketch]") { REQUIRE(total_weight == sketch.get_n()); } + SECTION("estimation mode, exact min and max at ranks 0 and 1") { + const int n = 1000; + int num_min_dropped = 0; + int num_max_dropped = 0; + for (int t = 0; t < 1000; ++t) { + kll_float_sketch sketch(8, std::less(), 0); + // min and max go in first, so compaction often drops them + sketch.update(0.0f); + sketch.update(static_cast(n - 1)); + for (int i = 1; i < n - 1; ++i) sketch.update(static_cast(i)); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == n - 1) max_retained = true; + } + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + if (!min_retained) { + ++num_min_dropped; + REQUIRE(sketch.get_rank(0.0f) == 1.0 / n); + } + if (!max_retained) { + ++num_max_dropped; + REQUIRE(sketch.get_rank(static_cast(n - 1), false) == (n - 1.0) / n); + } + } + REQUIRE(num_min_dropped > 0); + REQUIRE(num_max_dropped > 0); + } + + SECTION("estimation mode, exact min and max at ranks 0 and 1, strings") { + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + kll_string_sketch sketch(8, std::less(), 0); + sketch.update(std::string("a")); + sketch.update(std::string("z")); + for (int i = 0; i < 1000; ++i) sketch.update("m" + std::to_string(i)); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == "a") min_retained = true; + if (pair.first == "z") max_retained = true; + } + if (!min_retained || !max_retained) ++num_dropped; + REQUIRE(sketch.get_quantile(0) == "a"); + REQUIRE(sketch.get_quantile(0, false) == "a"); + REQUIRE(sketch.get_quantile(1) == "z"); + REQUIRE(sketch.get_quantile(1, false) == "z"); + } + REQUIRE(num_dropped > 0); + } + + SECTION("estimation mode, exact min and max at ranks 0 and 1 after deserialize and merge") { + const int n = 1000; + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + kll_float_sketch sketch1(8, std::less(), 0); + kll_float_sketch sketch2(8, std::less(), 0); + // sketch1 gets the min first and sketch2 gets the max first, so compaction often drops them + sketch1.update(0.0f); + for (int i = 1; i < n; ++i) sketch1.update(static_cast(i)); + sketch2.update(static_cast(2 * n - 1)); + for (int i = n; i < 2 * n - 1; ++i) sketch2.update(static_cast(i)); + + auto bytes = sketch1.serialize(); + auto sketch = kll_float_sketch::deserialize(bytes.data(), bytes.size(), serde(), std::less(), 0); + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + + sketch.merge(sketch2); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == 2 * n - 1) max_retained = true; + } + if (!min_retained || !max_retained) ++num_dropped; + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == 2 * n - 1); + REQUIRE(sketch.get_quantile(1, false) == 2 * n - 1); + } + REQUIRE(num_dropped > 0); + } + SECTION("consistency between get_rank and get_PMF/CDF") { kll_float_sketch sketch(200, std::less(), 0); const int n = 200; diff --git a/quantiles/include/quantiles_sketch_impl.hpp b/quantiles/include/quantiles_sketch_impl.hpp index 2dacf21e..96205912 100644 --- a/quantiles/include/quantiles_sketch_impl.hpp +++ b/quantiles/include/quantiles_sketch_impl.hpp @@ -746,6 +746,7 @@ quantiles_sorted_view quantiles_sketch::get_sorted_view() cons } view.convert_to_cummulative(); + if (!is_empty()) view.include_min_max(*min_item_, *max_item_); return view; } diff --git a/quantiles/test/quantiles_sketch_test.cpp b/quantiles/test/quantiles_sketch_test.cpp index 8230f6a1..198b6621 100644 --- a/quantiles/test/quantiles_sketch_test.cpp +++ b/quantiles/test/quantiles_sketch_test.cpp @@ -218,6 +218,75 @@ TEST_CASE("quantiles sketch", "[quantiles_sketch]") { REQUIRE(total_weight == sketch.get_n()); } + SECTION("estimation mode, exact min and max at ranks 0 and 1") { + const int n = 1000; + int num_min_dropped = 0; + int num_max_dropped = 0; + for (int t = 0; t < 1000; ++t) { + quantiles_float_sketch sketch(8, std::less(), 0); + // min and max go in first, so compaction often drops them + sketch.update(0.0f); + sketch.update(static_cast(n - 1)); + for (int i = 1; i < n - 1; ++i) sketch.update(static_cast(i)); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == n - 1) max_retained = true; + } + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + if (!min_retained) { + ++num_min_dropped; + REQUIRE(sketch.get_rank(0.0f) == 1.0 / n); + } + if (!max_retained) { + ++num_max_dropped; + REQUIRE(sketch.get_rank(static_cast(n - 1), false) == (n - 1.0) / n); + } + } + REQUIRE(num_min_dropped > 0); + REQUIRE(num_max_dropped > 0); + } + + SECTION("estimation mode, exact min and max at ranks 0 and 1 after deserialize and merge") { + const int n = 1000; + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + quantiles_float_sketch sketch1(8, std::less(), 0); + quantiles_float_sketch sketch2(8, std::less(), 0); + // sketch1 gets the min first and sketch2 gets the max first, so compaction often drops them + sketch1.update(0.0f); + for (int i = 1; i < n; ++i) sketch1.update(static_cast(i)); + sketch2.update(static_cast(2 * n - 1)); + for (int i = n; i < 2 * n - 1; ++i) sketch2.update(static_cast(i)); + + auto bytes = sketch1.serialize(); + auto sketch = quantiles_float_sketch::deserialize(bytes.data(), bytes.size(), serde(), + std::less(), 0); + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + + sketch.merge(sketch2); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == 2 * n - 1) max_retained = true; + } + if (!min_retained || !max_retained) ++num_dropped; + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == 2 * n - 1); + REQUIRE(sketch.get_quantile(1, false) == 2 * n - 1); + } + REQUIRE(num_dropped > 0); + } + SECTION("consistency between get_rank and get_PMF/CDF") { quantiles_float_sketch sketch(64, std::less(), 0); const int n = 1000; From 8cd37cca9fc30cec70a0970aff783a48c8f81d0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Sat, 7 Nov 2026 23:28:52 +0000 Subject: [PATCH 2/5] Document the sorted view lifetime and return the exact min and max in REQ For non-arithmetic types, a sorted view stores pointers to the items of the sketch, and get_quantile() returns a reference to one of them. Since the view now also points to the min and max inside the sketch object, a move of the sketch also makes them invalid. Say so in get_sorted_view(), get_quantile() and the quantiles_sorted_view class. REQ get_rank() summed the weights in the compactors and did not read the sorted view, so it did not see the min and max that only the view has. Move get_rank() to the cached sorted view, as in KLL, quantiles and Java ReqSketch, and call include_min_max() from get_sorted_view(). This removes the only caller of req_compactor::compute_weight(), so remove it. Co-authored-by: Isaac --- common/include/quantiles_sorted_view.hpp | 4 +- kll/include/kll_sketch.hpp | 7 +- quantiles/include/quantiles_sketch.hpp | 7 +- req/include/req_compactor.hpp | 2 - req/include/req_compactor_impl.hpp | 9 --- req/include/req_sketch.hpp | 6 +- req/include/req_sketch_impl.hpp | 8 +- req/test/req_sketch_test.cpp | 94 ++++++++++++++++++++++++ 8 files changed, 117 insertions(+), 20 deletions(-) diff --git a/common/include/quantiles_sorted_view.hpp b/common/include/quantiles_sorted_view.hpp index e016d801..9528c9b8 100755 --- a/common/include/quantiles_sorted_view.hpp +++ b/common/include/quantiles_sorted_view.hpp @@ -28,7 +28,9 @@ namespace datasketches { /** - * Sorted view for quantiles sketches (REQ, KLL and Quantiles) + * Sorted view for quantiles sketches (REQ, KLL and Quantiles). + * For non-arithmetic types, the view refers to the items of the sketch it came from, + * so it must not be used after that sketch is updated, merged, assigned to, moved or destroyed. */ template< typename T, diff --git a/kll/include/kll_sketch.hpp b/kll/include/kll_sketch.hpp index d672c419..a397f3af 100644 --- a/kll/include/kll_sketch.hpp +++ b/kll/include/kll_sketch.hpp @@ -308,6 +308,9 @@ class kll_sketch { * *

If the sketch is empty this throws std::runtime_error. * + *

For non-arithmetic types, the returned reference refers to an item of this sketch, so it + * must not be used after this sketch is updated, merged, assigned to, moved or destroyed. + * * @param rank of an item in the hypothetical sorted stream. * @param inclusive if true, the given rank is considered inclusive (includes weight of an item) * @@ -520,7 +523,9 @@ class kll_sketch { const_iterator end() const; /** - * Gets the sorted view of this sketch + * Gets the sorted view of this sketch. + * For non-arithmetic types, the view refers to the items of this sketch, so it must not be used + * after this sketch is updated, merged, assigned to, moved or destroyed. * @return the sorted view of this sketch */ quantiles_sorted_view get_sorted_view() const; diff --git a/quantiles/include/quantiles_sketch.hpp b/quantiles/include/quantiles_sketch.hpp index e995e3e3..2e851ea5 100644 --- a/quantiles/include/quantiles_sketch.hpp +++ b/quantiles/include/quantiles_sketch.hpp @@ -288,6 +288,9 @@ class quantiles_sketch { * of a hypothetical sorted version of the input stream so far. *

* If the sketch is empty this throws std::runtime_error. + *

+ * For non-arithmetic types, the returned reference refers to an item of this sketch, so it + * must not be used after this sketch is updated, merged, assigned to, moved or destroyed. * * @param rank the specified normalized rank in the hypothetical sorted stream. * @param inclusive if true the weight of the given item is included into the rank. @@ -475,7 +478,9 @@ class quantiles_sketch { const_iterator end() const; /** - * Gets the sorted view of this sketch + * Gets the sorted view of this sketch. + * For non-arithmetic types, the view refers to the items of this sketch, so it must not be used + * after this sketch is updated, merged, assigned to, moved or destroyed. * @return the sorted view of this sketch */ quantiles_sorted_view get_sorted_view() const; diff --git a/req/include/req_compactor.hpp b/req/include/req_compactor.hpp index f013e49d..7344f26b 100755 --- a/req/include/req_compactor.hpp +++ b/req/include/req_compactor.hpp @@ -51,8 +51,6 @@ class req_compactor { T* begin(); T* end(); - uint64_t compute_weight(const T& item, bool inclusive) const; - template void append(FwdT&& item); diff --git a/req/include/req_compactor_impl.hpp b/req/include/req_compactor_impl.hpp index 72aafc94..03ed8126 100755 --- a/req/include/req_compactor_impl.hpp +++ b/req/include/req_compactor_impl.hpp @@ -184,15 +184,6 @@ uint8_t req_compactor::get_lg_weight() const { return lg_weight_; } -template -uint64_t req_compactor::compute_weight(const T& item, bool inclusive) const { - if (!sorted_) const_cast(this)->sort(); // allow sorting as a side effect - auto it = inclusive ? - std::upper_bound(begin(), end(), item, comparator_) : - std::lower_bound(begin(), end(), item, comparator_); - return std::distance(begin(), it) << lg_weight_; -} - template template void req_compactor::append(FwdT&& item) { diff --git a/req/include/req_sketch.hpp b/req/include/req_sketch.hpp index 2e2bc309..2df727d7 100755 --- a/req/include/req_sketch.hpp +++ b/req/include/req_sketch.hpp @@ -291,6 +291,8 @@ class req_sketch { * Returns an approximate quantile of the given normalized rank. * The normalized rank must be in the range [0.0, 1.0] (both inclusive). *

If the sketch is empty this throws std::runtime_error. + *

For non-arithmetic types, the returned reference refers to an item of this sketch, so it + * must not be used after this sketch is updated, merged, assigned to, moved or destroyed. * * @param rank of an item in the hypothetical sorted stream. * @param inclusive if true, the given rank is considered inclusive (includes weight of an item) @@ -419,7 +421,9 @@ class req_sketch { const_iterator end() const; /** - * Gets the sorted view of this sketch + * Gets the sorted view of this sketch. + * For non-arithmetic types, the view refers to the items of this sketch, so it must not be used + * after this sketch is updated, merged, assigned to, moved or destroyed. * @return the sorted view of this sketch */ quantiles_sorted_view get_sorted_view() const; diff --git a/req/include/req_sketch_impl.hpp b/req/include/req_sketch_impl.hpp index 8acea8bc..06f926ff 100755 --- a/req/include/req_sketch_impl.hpp +++ b/req/include/req_sketch_impl.hpp @@ -237,11 +237,8 @@ A req_sketch::get_allocator() const { template double req_sketch::get_rank(const T& item, bool inclusive) const { if (is_empty()) throw std::runtime_error("operation is undefined for an empty sketch"); - uint64_t weight = 0; - for (const auto& compactor: compactors_) { - weight += compactor.compute_weight(item, inclusive); - } - return static_cast(weight) / n_; + setup_sorted_view(); + return sorted_view_->get_rank(item, inclusive); } template @@ -281,6 +278,7 @@ quantiles_sorted_view req_sketch::get_sorted_view() const { } view.convert_to_cummulative(); + if (!is_empty()) view.include_min_max(*min_item_, *max_item_); return view; } diff --git a/req/test/req_sketch_test.cpp b/req/test/req_sketch_test.cpp index c8cf5cbe..c6226b3b 100755 --- a/req/test/req_sketch_test.cpp +++ b/req/test/req_sketch_test.cpp @@ -213,6 +213,100 @@ TEST_CASE("req sketch: estimation mode", "[req_sketch]") { REQUIRE(count == sketch.get_num_retained()); } +TEST_CASE("req sketch: estimation mode, exact min and max at ranks 0 and 1", "[req_sketch]") { + const int n = 1000; + for (const bool hra: {true, false}) { + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + req_sketch sketch(4, hra); + // min and max go in first, so compaction often drops one of them + sketch.update(0.0f); + sketch.update(static_cast(n - 1)); + for (int i = 1; i < n - 1; ++i) sketch.update(static_cast(i)); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == n - 1) max_retained = true; + } + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + if (!min_retained) { + ++num_dropped; + REQUIRE(sketch.get_rank(0.0f) == 1.0 / n); + } + if (!max_retained) { + ++num_dropped; + REQUIRE(sketch.get_rank(static_cast(n - 1), false) == (n - 1.0) / n); + } + } + REQUIRE(num_dropped > 0); + } +} + +TEST_CASE("req sketch: estimation mode, exact min and max at ranks 0 and 1, strings", "[req_sketch]") { + for (const bool hra: {true, false}) { + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + req_sketch sketch(4, hra); + sketch.update(std::string("a")); + sketch.update(std::string("z")); + for (int i = 0; i < 1000; ++i) sketch.update("m" + std::to_string(i)); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == "a") min_retained = true; + if (pair.first == "z") max_retained = true; + } + if (!min_retained || !max_retained) ++num_dropped; + REQUIRE(sketch.get_quantile(0) == "a"); + REQUIRE(sketch.get_quantile(0, false) == "a"); + REQUIRE(sketch.get_quantile(1) == "z"); + REQUIRE(sketch.get_quantile(1, false) == "z"); + } + REQUIRE(num_dropped > 0); + } +} + +TEST_CASE("req sketch: exact min and max at ranks 0 and 1 after deserialize and merge", "[req_sketch]") { + const int n = 1000; + for (const bool hra: {true, false}) { + int num_dropped = 0; + for (int t = 0; t < 1000; ++t) { + req_sketch sketch1(4, hra); + req_sketch sketch2(4, hra); + // sketch1 gets the min first and sketch2 gets the max first, so compaction often drops them + sketch1.update(0.0f); + for (int i = 1; i < n; ++i) sketch1.update(static_cast(i)); + sketch2.update(static_cast(2 * n - 1)); + for (int i = n; i < 2 * n - 1; ++i) sketch2.update(static_cast(i)); + + auto bytes = sketch1.serialize(); + auto sketch = req_sketch::deserialize(bytes.data(), bytes.size()); + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == n - 1); + REQUIRE(sketch.get_quantile(1, false) == n - 1); + + sketch.merge(sketch2); + bool min_retained = false; + bool max_retained = false; + for (auto pair: sketch) { + if (pair.first == 0) min_retained = true; + if (pair.first == 2 * n - 1) max_retained = true; + } + if (!min_retained || !max_retained) ++num_dropped; + REQUIRE(sketch.get_quantile(0) == 0); + REQUIRE(sketch.get_quantile(0, false) == 0); + REQUIRE(sketch.get_quantile(1) == 2 * n - 1); + REQUIRE(sketch.get_quantile(1, false) == 2 * n - 1); + } + REQUIRE(num_dropped > 0); + } +} + TEST_CASE("req sketch: stream serialize-deserialize empty", "[req_sketch]") { req_sketch sketch(12); From 72aec0468f4fba56abfc5177f8f711298ea6a99b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Mon, 9 Nov 2026 12:18:34 +0000 Subject: [PATCH 3/5] Speed up adding items to the sorted view add() merged the whole view into a new vector also for an empty range, and a KLL sketch usually has one or two empty levels once n is much larger than k. Return early for an empty range. Fill the new entries by index instead of push_back(), which updates the end pointer for every item. Co-authored-by: Isaac --- common/include/quantiles_sorted_view_impl.hpp | 7 ++++++- common/test/quantiles_sorted_view_test.cpp | 21 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/common/include/quantiles_sorted_view_impl.hpp b/common/include/quantiles_sorted_view_impl.hpp index c8520376..41e83cbb 100755 --- a/common/include/quantiles_sorted_view_impl.hpp +++ b/common/include/quantiles_sorted_view_impl.hpp @@ -21,6 +21,7 @@ #define QUANTILES_SORTED_VIEW_IMPL_HPP_ #include +#include #include #include @@ -38,8 +39,12 @@ entries_(allocator) template template void quantiles_sorted_view::add(Iterator first, Iterator last, uint64_t weight) { + if (first == last) return; const size_t size_before = entries_.size(); - for (auto it = first; it != last; ++it) entries_.push_back(Entry(ref_helper(*it), weight)); + // faster than push_back, which updates the end pointer for every item + entries_.resize(size_before + std::distance(first, last)); + auto out = entries_.begin() + size_before; + for (auto it = first; it != last; ++it, ++out) *out = Entry(ref_helper(*it), weight); if (size_before > 0) { Container tmp(entries_.get_allocator()); tmp.reserve(entries_.capacity()); diff --git a/common/test/quantiles_sorted_view_test.cpp b/common/test/quantiles_sorted_view_test.cpp index 938c4110..07a8b4d3 100644 --- a/common/test/quantiles_sorted_view_test.cpp +++ b/common/test/quantiles_sorted_view_test.cpp @@ -456,6 +456,27 @@ TEST_CASE("set 4", "sorted view") { REQUIRE(view.get_quantile(1, false) == 40); } +TEST_CASE("add empty items", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); + std::vector empty; + std::vector l0 {1, 3}; + std::vector l2 {2, 5}; + view.add(empty.begin(), empty.end(), 1); + view.add(l0.begin(), l0.end(), 1); + view.add(empty.begin(), empty.end(), 2); + view.add(l2.begin(), l2.end(), 4); + view.convert_to_cummulative(); + REQUIRE(view.size() == 4); + + const std::vector> expected {{1, 1}, {2, 5}, {3, 6}, {5, 10}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); +} TEST_CASE("include min and max", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); std::vector l1 {2, 4, 6, 7}; From 765d17e6d8502f683132a3dd69e9f512ccd7e2c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Mon, 9 Nov 2026 12:18:55 +0000 Subject: [PATCH 4/5] Add a missing min to the sorted view before the other items include_min_max() inserted a missing min at the front of the view, which moves every entry, and the view was reserved for exactly the retained items, so adding the min or max also reallocated and copied it. Once n is much larger than k the min and max are usually missing, so this made get_sorted_view() and the first query after an update slower. Reserve room for the min and max. The levels are sorted, so the sketch finds out if the min is retained by comparing it with the first item of each level, and if not, adds it with add_min() before the other items. add() skips the merge when the new items start at or after the last entry, so the first level is appended after the min. The new convert_to_cummulative(max_item) replaces include_min_max(): it takes the weight 1 of the min from the next entry, and appends a missing max with weight 1 taken from the entry before it. Co-authored-by: Isaac --- common/include/quantiles_sorted_view.hpp | 6 +- common/include/quantiles_sorted_view_impl.hpp | 28 +++++--- common/test/quantiles_sorted_view_test.cpp | 72 +++++++++++++++++-- kll/include/kll_sketch_impl.hpp | 10 ++- quantiles/include/quantiles_sketch_impl.hpp | 10 ++- req/include/req_sketch_impl.hpp | 10 ++- 6 files changed, 113 insertions(+), 23 deletions(-) diff --git a/common/include/quantiles_sorted_view.hpp b/common/include/quantiles_sorted_view.hpp index 9528c9b8..0f51b7d2 100755 --- a/common/include/quantiles_sorted_view.hpp +++ b/common/include/quantiles_sorted_view.hpp @@ -55,7 +55,10 @@ class quantiles_sorted_view { void convert_to_cummulative(); /// @private - void include_min_max(const T& min_item, const T& max_item); + void add_min(const T& min_item); + + /// @private + void convert_to_cummulative(const T& max_item); class const_iterator; @@ -160,6 +163,7 @@ class quantiles_sorted_view { private: Comparator comparator_; uint64_t total_weight_; + bool min_added_; Container entries_; static inline const T& deref_helper(const T* t) { return *t; } diff --git a/common/include/quantiles_sorted_view_impl.hpp b/common/include/quantiles_sorted_view_impl.hpp index 41e83cbb..ba5f6358 100755 --- a/common/include/quantiles_sorted_view_impl.hpp +++ b/common/include/quantiles_sorted_view_impl.hpp @@ -31,9 +31,10 @@ template quantiles_sorted_view::quantiles_sorted_view(uint32_t num, const C& comparator, const A& allocator): comparator_(comparator), total_weight_(0), +min_added_(false), entries_(allocator) { - entries_.reserve(num); + if (num > 0) entries_.reserve(num + 2); // room for the min and max } template @@ -41,11 +42,13 @@ template void quantiles_sorted_view::add(Iterator first, Iterator last, uint64_t weight) { if (first == last) return; const size_t size_before = entries_.size(); + // no merge needed if the new items start at or after the last entry + const bool merge = size_before > 0 && comparator_(*first, deref_helper(entries_.back().first)); // faster than push_back, which updates the end pointer for every item entries_.resize(size_before + std::distance(first, last)); auto out = entries_.begin() + size_before; for (auto it = first; it != last; ++it, ++out) *out = Entry(ref_helper(*it), weight); - if (size_before > 0) { + if (merge) { Container tmp(entries_.get_allocator()); tmp.reserve(entries_.capacity()); std::merge( @@ -67,16 +70,21 @@ void quantiles_sorted_view::convert_to_cummulative() { // Compaction can drop the min and max, but the sketch stores them exactly. Add each back as an entry // of weight 1, taken from its neighbor, so ranks 0 and 1 return them (IncludeMinMax in Java). +// The sketch adds a missing min before the other items, so no entry has to move. template -void quantiles_sorted_view::include_min_max(const T& min_item, const T& max_item) { - const bool add_min = comparator_(min_item, deref_helper(entries_.front().first)); - const bool add_max = comparator_(deref_helper(entries_.back().first), max_item); - entries_.reserve(entries_.size() + (add_min ? 1 : 0) + (add_max ? 1 : 0)); - if (add_max) { - entries_.back().second = total_weight_ - 1; - entries_.push_back(Entry(ref_helper(max_item), total_weight_)); +void quantiles_sorted_view::add_min(const T& min_item) { + entries_.push_back(Entry(ref_helper(min_item), 1)); + min_added_ = true; +} + +template +void quantiles_sorted_view::convert_to_cummulative(const T& max_item) { + if (min_added_) --entries_[1].second; + if (comparator_(deref_helper(entries_.back().first), max_item)) { + --entries_.back().second; + entries_.push_back(Entry(ref_helper(max_item), 1)); } - if (add_min) entries_.insert(entries_.begin(), Entry(ref_helper(min_item), 1)); + convert_to_cummulative(); } template diff --git a/common/test/quantiles_sorted_view_test.cpp b/common/test/quantiles_sorted_view_test.cpp index 07a8b4d3..ddb1f66c 100644 --- a/common/test/quantiles_sorted_view_test.cpp +++ b/common/test/quantiles_sorted_view_test.cpp @@ -480,9 +480,9 @@ TEST_CASE("add empty items", "sorted view") { TEST_CASE("include min and max", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); std::vector l1 {2, 4, 6, 7}; + view.add_min(1); view.add(l1.begin(), l1.end(), 2); - view.convert_to_cummulative(); - view.include_min_max(1, 8); + view.convert_to_cummulative(8); REQUIRE(view.size() == 6); const std::vector> expected {{1, 1}, {2, 2}, {4, 4}, {6, 6}, {7, 7}, {8, 8}}; @@ -507,8 +507,7 @@ TEST_CASE("include min and max already retained", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); std::vector l1 {2, 4, 6, 7}; view.add(l1.begin(), l1.end(), 2); - view.convert_to_cummulative(); - view.include_min_max(2, 7); + view.convert_to_cummulative(7); REQUIRE(view.size() == 4); const std::vector> expected {{2, 2}, {4, 4}, {6, 6}, {7, 8}}; @@ -524,9 +523,9 @@ TEST_CASE("include min and max already retained", "sorted view") { TEST_CASE("include min and max with one retained item", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(1, std::less(), std::allocator()); std::vector l2 {5}; + view.add_min(1); view.add(l2.begin(), l2.end(), 4); - view.convert_to_cummulative(); - view.include_min_max(1, 9); + view.convert_to_cummulative(9); REQUIRE(view.size() == 3); const std::vector> expected {{1, 1}, {5, 3}, {9, 4}}; @@ -543,4 +542,65 @@ TEST_CASE("include min and max with one retained item", "sorted view") { REQUIRE(view.get_quantile(1) == 9); } +TEST_CASE("include only max", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(3, std::less(), std::allocator()); + std::vector l1 {2, 4}; + std::vector l0 {6}; + view.add(l1.begin(), l1.end(), 2); + view.add(l0.begin(), l0.end(), 1); + view.convert_to_cummulative(8); + REQUIRE(view.size() == 4); + + // 6 gives its only weight to the max + const std::vector> expected {{2, 2}, {4, 4}, {6, 4}, {8, 5}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); +} + +TEST_CASE("include only min", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(3, std::less(), std::allocator()); + std::vector l1 {4, 6}; + std::vector l0 {2}; + view.add_min(1); + view.add(l1.begin(), l1.end(), 2); + view.add(l0.begin(), l0.end(), 1); + view.convert_to_cummulative(6); + REQUIRE(view.size() == 4); + + // 2 gives its only weight to the min + const std::vector> expected {{1, 1}, {2, 1}, {4, 3}, {6, 5}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); +} + +TEST_CASE("add in-order items", "sorted view") { + auto view = quantiles_sorted_view, std::allocator>(5, std::less(), std::allocator()); + std::vector l0 {1, 3}; + std::vector l2 {3, 5, 7}; + view.add(l0.begin(), l0.end(), 1); + view.add(l2.begin(), l2.end(), 4); + view.convert_to_cummulative(); + REQUIRE(view.size() == 5); + + // the earlier 3 stays first, as in a merge + const std::vector> expected {{1, 1}, {3, 2}, {3, 6}, {5, 10}, {7, 14}}; + auto it = view.begin(); + for (const auto& entry: expected) { + REQUIRE(it->first == entry.first); + REQUIRE(it->second == entry.second); + ++it; + } + REQUIRE(it == view.end()); +} + } /* namespace datasketches */ diff --git a/kll/include/kll_sketch_impl.hpp b/kll/include/kll_sketch_impl.hpp index 42891261..3d9d8829 100644 --- a/kll/include/kll_sketch_impl.hpp +++ b/kll/include/kll_sketch_impl.hpp @@ -769,13 +769,19 @@ template quantiles_sorted_view kll_sketch::get_sorted_view() const { const_cast(this)->sort_level_zero(); // allow this side effect quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); + if (is_empty()) return view; + // the levels are sorted, so the min is retained only as the first item of a level + bool min_retained = false; + for (uint8_t level = 0; level < num_levels_ && !min_retained; ++level) { + min_retained = levels_[level] < levels_[level + 1] && !comparator_(*min_item_, items_[levels_[level]]); + } + if (!min_retained) view.add_min(*min_item_); for (uint8_t level = 0; level < num_levels_; ++level) { const auto from = items_ + levels_[level]; const auto to = items_ + levels_[level + 1]; // exclusive view.add(from, to, 1ULL << level); } - view.convert_to_cummulative(); - if (!is_empty()) view.include_min_max(*min_item_, *max_item_); + view.convert_to_cummulative(*max_item_); return view; } diff --git a/quantiles/include/quantiles_sketch_impl.hpp b/quantiles/include/quantiles_sketch_impl.hpp index 96205912..a561acea 100644 --- a/quantiles/include/quantiles_sketch_impl.hpp +++ b/quantiles/include/quantiles_sketch_impl.hpp @@ -736,6 +736,13 @@ quantiles_sorted_view quantiles_sketch::get_sorted_view() cons const_cast(this)->is_base_buffer_sorted_ = true; } quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); + if (is_empty()) return view; + // the base buffer and the levels are sorted, so the min is retained only as the first item of one + bool min_retained = !base_buffer_.empty() && !comparator_(*min_item_, base_buffer_.front()); + for (auto it = levels_.begin(); it != levels_.end() && !min_retained; ++it) { + min_retained = !it->empty() && !comparator_(*min_item_, it->front()); + } + if (!min_retained) view.add_min(*min_item_); uint64_t weight = 1; view.add(base_buffer_.begin(), base_buffer_.end(), weight); @@ -745,8 +752,7 @@ quantiles_sorted_view quantiles_sketch::get_sorted_view() cons view.add(level.begin(), level.end(), weight); } - view.convert_to_cummulative(); - if (!is_empty()) view.include_min_max(*min_item_, *max_item_); + view.convert_to_cummulative(*max_item_); return view; } diff --git a/req/include/req_sketch_impl.hpp b/req/include/req_sketch_impl.hpp index 06f926ff..d15782f7 100755 --- a/req/include/req_sketch_impl.hpp +++ b/req/include/req_sketch_impl.hpp @@ -272,13 +272,19 @@ quantiles_sorted_view req_sketch::get_sorted_view() const { const_cast(compactors_[0]).sort(); // allow this side effect } quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); + if (is_empty()) return view; + // the compactors are sorted, so the min is retained only as the first item of one + bool min_retained = false; + for (auto it = compactors_.begin(); it != compactors_.end() && !min_retained; ++it) { + min_retained = it->begin() != it->end() && !comparator_(*min_item_, *it->begin()); + } + if (!min_retained) view.add_min(*min_item_); for (auto& compactor: compactors_) { view.add(compactor.begin(), compactor.end(), 1ULL << compactor.get_lg_weight()); } - view.convert_to_cummulative(); - if (!is_empty()) view.include_min_max(*min_item_, *max_item_); + view.convert_to_cummulative(*max_item_); return view; } From 039653684e1fd3cf796fc1c5fd3088b659085f8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Mon, 9 Nov 2026 13:39:19 +0000 Subject: [PATCH 5/5] Add the min and max in the cumulative pass Go back to a simpler version: remove add_min(), the checks in the sketches for a retained min, and the changes to add(). The sketches call the new convert_to_cummulative(min_item, max_item). It appends a missing max with weight 1 taken from the entry before it, and adds a missing min with weight 1 taken from the entry after it, in the same pass that makes the weights cumulative, so the entries shift without a second pass. The view reserves room for both, so it is not reallocated. Co-authored-by: Isaac --- common/include/quantiles_sorted_view.hpp | 6 +-- common/include/quantiles_sorted_view_impl.hpp | 39 +++++++------- common/test/quantiles_sorted_view_test.cpp | 54 ++----------------- kll/include/kll_sketch_impl.hpp | 9 +--- quantiles/include/quantiles_sketch_impl.hpp | 9 +--- req/include/req_sketch_impl.hpp | 9 +--- 6 files changed, 27 insertions(+), 99 deletions(-) diff --git a/common/include/quantiles_sorted_view.hpp b/common/include/quantiles_sorted_view.hpp index 0f51b7d2..53048e3f 100755 --- a/common/include/quantiles_sorted_view.hpp +++ b/common/include/quantiles_sorted_view.hpp @@ -55,10 +55,7 @@ class quantiles_sorted_view { void convert_to_cummulative(); /// @private - void add_min(const T& min_item); - - /// @private - void convert_to_cummulative(const T& max_item); + void convert_to_cummulative(const T& min_item, const T& max_item); class const_iterator; @@ -163,7 +160,6 @@ class quantiles_sorted_view { private: Comparator comparator_; uint64_t total_weight_; - bool min_added_; Container entries_; static inline const T& deref_helper(const T* t) { return *t; } diff --git a/common/include/quantiles_sorted_view_impl.hpp b/common/include/quantiles_sorted_view_impl.hpp index ba5f6358..5033faab 100755 --- a/common/include/quantiles_sorted_view_impl.hpp +++ b/common/include/quantiles_sorted_view_impl.hpp @@ -21,7 +21,6 @@ #define QUANTILES_SORTED_VIEW_IMPL_HPP_ #include -#include #include #include @@ -31,24 +30,17 @@ template quantiles_sorted_view::quantiles_sorted_view(uint32_t num, const C& comparator, const A& allocator): comparator_(comparator), total_weight_(0), -min_added_(false), entries_(allocator) { - if (num > 0) entries_.reserve(num + 2); // room for the min and max + entries_.reserve(num + 2); // room for the min and max } template template void quantiles_sorted_view::add(Iterator first, Iterator last, uint64_t weight) { - if (first == last) return; const size_t size_before = entries_.size(); - // no merge needed if the new items start at or after the last entry - const bool merge = size_before > 0 && comparator_(*first, deref_helper(entries_.back().first)); - // faster than push_back, which updates the end pointer for every item - entries_.resize(size_before + std::distance(first, last)); - auto out = entries_.begin() + size_before; - for (auto it = first; it != last; ++it, ++out) *out = Entry(ref_helper(*it), weight); - if (merge) { + for (auto it = first; it != last; ++it) entries_.push_back(Entry(ref_helper(*it), weight)); + if (size_before > 0) { Container tmp(entries_.get_allocator()); tmp.reserve(entries_.capacity()); std::merge( @@ -70,21 +62,26 @@ void quantiles_sorted_view::convert_to_cummulative() { // Compaction can drop the min and max, but the sketch stores them exactly. Add each back as an entry // of weight 1, taken from its neighbor, so ranks 0 and 1 return them (IncludeMinMax in Java). -// The sketch adds a missing min before the other items, so no entry has to move. +// The min is added in the cumulative pass, which shifts the entries without a second pass. template -void quantiles_sorted_view::add_min(const T& min_item) { - entries_.push_back(Entry(ref_helper(min_item), 1)); - min_added_ = true; -} - -template -void quantiles_sorted_view::convert_to_cummulative(const T& max_item) { - if (min_added_) --entries_[1].second; +void quantiles_sorted_view::convert_to_cummulative(const T& min_item, const T& max_item) { if (comparator_(deref_helper(entries_.back().first), max_item)) { --entries_.back().second; entries_.push_back(Entry(ref_helper(max_item), 1)); } - convert_to_cummulative(); + if (!comparator_(min_item, deref_helper(entries_.front().first))) { + convert_to_cummulative(); + return; + } + --entries_.front().second; + entries_.push_back(entries_.back()); // the slot that the last entry shifts into + Entry carry(ref_helper(min_item), 1); + for (auto& entry: entries_) { + const Entry next = entry; + total_weight_ += carry.second; + entry = Entry(carry.first, total_weight_); + carry = next; + } } template diff --git a/common/test/quantiles_sorted_view_test.cpp b/common/test/quantiles_sorted_view_test.cpp index ddb1f66c..f9b7b699 100644 --- a/common/test/quantiles_sorted_view_test.cpp +++ b/common/test/quantiles_sorted_view_test.cpp @@ -456,33 +456,11 @@ TEST_CASE("set 4", "sorted view") { REQUIRE(view.get_quantile(1, false) == 40); } -TEST_CASE("add empty items", "sorted view") { - auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); - std::vector empty; - std::vector l0 {1, 3}; - std::vector l2 {2, 5}; - view.add(empty.begin(), empty.end(), 1); - view.add(l0.begin(), l0.end(), 1); - view.add(empty.begin(), empty.end(), 2); - view.add(l2.begin(), l2.end(), 4); - view.convert_to_cummulative(); - REQUIRE(view.size() == 4); - - const std::vector> expected {{1, 1}, {2, 5}, {3, 6}, {5, 10}}; - auto it = view.begin(); - for (const auto& entry: expected) { - REQUIRE(it->first == entry.first); - REQUIRE(it->second == entry.second); - ++it; - } - REQUIRE(it == view.end()); -} TEST_CASE("include min and max", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); std::vector l1 {2, 4, 6, 7}; - view.add_min(1); view.add(l1.begin(), l1.end(), 2); - view.convert_to_cummulative(8); + view.convert_to_cummulative(1, 8); REQUIRE(view.size() == 6); const std::vector> expected {{1, 1}, {2, 2}, {4, 4}, {6, 6}, {7, 7}, {8, 8}}; @@ -507,7 +485,7 @@ TEST_CASE("include min and max already retained", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(4, std::less(), std::allocator()); std::vector l1 {2, 4, 6, 7}; view.add(l1.begin(), l1.end(), 2); - view.convert_to_cummulative(7); + view.convert_to_cummulative(2, 7); REQUIRE(view.size() == 4); const std::vector> expected {{2, 2}, {4, 4}, {6, 6}, {7, 8}}; @@ -523,9 +501,8 @@ TEST_CASE("include min and max already retained", "sorted view") { TEST_CASE("include min and max with one retained item", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(1, std::less(), std::allocator()); std::vector l2 {5}; - view.add_min(1); view.add(l2.begin(), l2.end(), 4); - view.convert_to_cummulative(9); + view.convert_to_cummulative(1, 9); REQUIRE(view.size() == 3); const std::vector> expected {{1, 1}, {5, 3}, {9, 4}}; @@ -548,7 +525,7 @@ TEST_CASE("include only max", "sorted view") { std::vector l0 {6}; view.add(l1.begin(), l1.end(), 2); view.add(l0.begin(), l0.end(), 1); - view.convert_to_cummulative(8); + view.convert_to_cummulative(2, 8); REQUIRE(view.size() == 4); // 6 gives its only weight to the max @@ -566,10 +543,9 @@ TEST_CASE("include only min", "sorted view") { auto view = quantiles_sorted_view, std::allocator>(3, std::less(), std::allocator()); std::vector l1 {4, 6}; std::vector l0 {2}; - view.add_min(1); view.add(l1.begin(), l1.end(), 2); view.add(l0.begin(), l0.end(), 1); - view.convert_to_cummulative(6); + view.convert_to_cummulative(1, 6); REQUIRE(view.size() == 4); // 2 gives its only weight to the min @@ -583,24 +559,4 @@ TEST_CASE("include only min", "sorted view") { REQUIRE(it == view.end()); } -TEST_CASE("add in-order items", "sorted view") { - auto view = quantiles_sorted_view, std::allocator>(5, std::less(), std::allocator()); - std::vector l0 {1, 3}; - std::vector l2 {3, 5, 7}; - view.add(l0.begin(), l0.end(), 1); - view.add(l2.begin(), l2.end(), 4); - view.convert_to_cummulative(); - REQUIRE(view.size() == 5); - - // the earlier 3 stays first, as in a merge - const std::vector> expected {{1, 1}, {3, 2}, {3, 6}, {5, 10}, {7, 14}}; - auto it = view.begin(); - for (const auto& entry: expected) { - REQUIRE(it->first == entry.first); - REQUIRE(it->second == entry.second); - ++it; - } - REQUIRE(it == view.end()); -} - } /* namespace datasketches */ diff --git a/kll/include/kll_sketch_impl.hpp b/kll/include/kll_sketch_impl.hpp index 3d9d8829..bf37ec04 100644 --- a/kll/include/kll_sketch_impl.hpp +++ b/kll/include/kll_sketch_impl.hpp @@ -769,19 +769,12 @@ template quantiles_sorted_view kll_sketch::get_sorted_view() const { const_cast(this)->sort_level_zero(); // allow this side effect quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); - if (is_empty()) return view; - // the levels are sorted, so the min is retained only as the first item of a level - bool min_retained = false; - for (uint8_t level = 0; level < num_levels_ && !min_retained; ++level) { - min_retained = levels_[level] < levels_[level + 1] && !comparator_(*min_item_, items_[levels_[level]]); - } - if (!min_retained) view.add_min(*min_item_); for (uint8_t level = 0; level < num_levels_; ++level) { const auto from = items_ + levels_[level]; const auto to = items_ + levels_[level + 1]; // exclusive view.add(from, to, 1ULL << level); } - view.convert_to_cummulative(*max_item_); + if (!is_empty()) view.convert_to_cummulative(*min_item_, *max_item_); return view; } diff --git a/quantiles/include/quantiles_sketch_impl.hpp b/quantiles/include/quantiles_sketch_impl.hpp index a561acea..604a8a57 100644 --- a/quantiles/include/quantiles_sketch_impl.hpp +++ b/quantiles/include/quantiles_sketch_impl.hpp @@ -736,13 +736,6 @@ quantiles_sorted_view quantiles_sketch::get_sorted_view() cons const_cast(this)->is_base_buffer_sorted_ = true; } quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); - if (is_empty()) return view; - // the base buffer and the levels are sorted, so the min is retained only as the first item of one - bool min_retained = !base_buffer_.empty() && !comparator_(*min_item_, base_buffer_.front()); - for (auto it = levels_.begin(); it != levels_.end() && !min_retained; ++it) { - min_retained = !it->empty() && !comparator_(*min_item_, it->front()); - } - if (!min_retained) view.add_min(*min_item_); uint64_t weight = 1; view.add(base_buffer_.begin(), base_buffer_.end(), weight); @@ -752,7 +745,7 @@ quantiles_sorted_view quantiles_sketch::get_sorted_view() cons view.add(level.begin(), level.end(), weight); } - view.convert_to_cummulative(*max_item_); + if (!is_empty()) view.convert_to_cummulative(*min_item_, *max_item_); return view; } diff --git a/req/include/req_sketch_impl.hpp b/req/include/req_sketch_impl.hpp index d15782f7..548b20a0 100755 --- a/req/include/req_sketch_impl.hpp +++ b/req/include/req_sketch_impl.hpp @@ -272,19 +272,12 @@ quantiles_sorted_view req_sketch::get_sorted_view() const { const_cast(compactors_[0]).sort(); // allow this side effect } quantiles_sorted_view view(get_num_retained(), comparator_, allocator_); - if (is_empty()) return view; - // the compactors are sorted, so the min is retained only as the first item of one - bool min_retained = false; - for (auto it = compactors_.begin(); it != compactors_.end() && !min_retained; ++it) { - min_retained = it->begin() != it->end() && !comparator_(*min_item_, *it->begin()); - } - if (!min_retained) view.add_min(*min_item_); for (auto& compactor: compactors_) { view.add(compactor.begin(), compactor.end(), 1ULL << compactor.get_lg_weight()); } - view.convert_to_cummulative(*max_item_); + if (!is_empty()) view.convert_to_cummulative(*min_item_, *max_item_); return view; }