From 35223ffb6330cc7cf376b995ee106cadb74f2348 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Stojko?= Date: Mon, 9 Nov 2026 12:11:32 +0000 Subject: [PATCH] fix: kll_sketch throws when a sketch is merged into itself, as in Java merge() reads the items of the other sketch while it updates this one. When both are the same sketch, the merge fails once it compacts, with "Total weight does not match N", and with string items it reads destroyed strings. The Java KLL sketches throw a SketchesArgumentException with "A sketch cannot merge with itself." (apache/datasketches-java#516). kll_sketch now throws std::invalid_argument with the same message, before its other checks as in Java, so an empty sketch merged into itself throws too. Co-authored-by: Isaac --- kll/include/kll_sketch.hpp | 1 + kll/include/kll_sketch_impl.hpp | 1 + kll/test/kll_sketch_test.cpp | 10 ++++++++++ 3 files changed, 12 insertions(+) diff --git a/kll/include/kll_sketch.hpp b/kll/include/kll_sketch.hpp index 39cac445..274d5bec 100644 --- a/kll/include/kll_sketch.hpp +++ b/kll/include/kll_sketch.hpp @@ -241,6 +241,7 @@ class kll_sketch { * Merges another sketch into this one. * If sketches contain strings, callers are responsible for ensuring that * both sketches were built using compatible string encodings. + * If other is this sketch, this throws std::invalid_argument. * @param other sketch to merge into this one */ template diff --git a/kll/include/kll_sketch_impl.hpp b/kll/include/kll_sketch_impl.hpp index a1d0c4da..e1ed85ec 100644 --- a/kll/include/kll_sketch_impl.hpp +++ b/kll/include/kll_sketch_impl.hpp @@ -208,6 +208,7 @@ uint32_t kll_sketch::internal_update() { template template void kll_sketch::merge(FwdSk&& other) { + if (this == &other) { throw std::invalid_argument("A sketch cannot merge with itself."); } if (other.is_empty()) { return; } if (m_ != other.m_) { throw std::invalid_argument("incompatible M: " + std::to_string(m_) + " and " + std::to_string(other.m_)); diff --git a/kll/test/kll_sketch_test.cpp b/kll/test/kll_sketch_test.cpp index cb09473e..05eea991 100644 --- a/kll/test/kll_sketch_test.cpp +++ b/kll/test/kll_sketch_test.cpp @@ -566,6 +566,16 @@ TEST_CASE("kll sketch", "[kll_sketch]") { REQUIRE(sketch2.get_max_item() == 999999.0f); } + SECTION("merge into itself") { + kll_float_sketch sketch(200, std::less(), 0); + REQUIRE_THROWS_WITH(sketch.merge(sketch), "A sketch cannot merge with itself."); + for (int i = 0; i < 150; i++) sketch.update(static_cast(i)); + const auto bytes = sketch.serialize(); + REQUIRE_THROWS_WITH(sketch.merge(sketch), "A sketch cannot merge with itself."); + REQUIRE_THROWS_WITH(sketch.merge(static_cast(sketch)), "A sketch cannot merge with itself."); + REQUIRE(sketch.serialize() == bytes); + } + SECTION("sketch of ints") { kll_sketch sketch; REQUIRE_THROWS_AS(sketch.get_quantile(0), std::runtime_error);