diff --git a/req/include/req_common.hpp b/req/include/req_common.hpp index aaead872..4bd9f7f6 100755 --- a/req/include/req_common.hpp +++ b/req/include/req_common.hpp @@ -31,6 +31,8 @@ namespace datasketches { namespace req_constants { /// minimum value of parameter K const uint16_t MIN_K = 4; + /// maximum value of parameter K + const uint16_t MAX_K = 1024; /// initial number of sections const uint8_t INIT_NUM_SECTIONS = 3; /// multiplier for nominal capacity diff --git a/req/include/req_sketch.hpp b/req/include/req_sketch.hpp index 52295bd2..2e2bc309 100755 --- a/req/include/req_sketch.hpp +++ b/req/include/req_sketch.hpp @@ -98,12 +98,14 @@ class req_sketch { /** * Constructor - * @param k Controls the size and error of the sketch. It must be even and in the range [4, 1024], inclusive. + * @param k Controls the size and error of the sketch. It must be no greater than 1024. + * Values below 4 are raised to 4, and odd values are rounded down to the nearest even value. * Value of 12 roughly corresponds to 1% relative error guarantee at 95% confidence. * @param hra if true, the default, the high ranks are prioritized for better * accuracy. Otherwise the low ranks are prioritized for better accuracy. * @param comparator strict weak ordering function (see C++ named requirements: Compare) * @param allocator used by this sketch to allocate memory + * @throws std::invalid_argument if k is greater than 1024 */ explicit req_sketch(uint16_t k, bool hra = true, const Comparator& comparator = Comparator(), const Allocator& allocator = Allocator()); diff --git a/req/include/req_sketch_impl.hpp b/req/include/req_sketch_impl.hpp index 7f0b4557..8acea8bc 100755 --- a/req/include/req_sketch_impl.hpp +++ b/req/include/req_sketch_impl.hpp @@ -30,7 +30,7 @@ template req_sketch::req_sketch(uint16_t k, bool hra, const C& comparator, const A& allocator): comparator_(comparator), allocator_(allocator), -k_(std::max(static_cast(k) & -2, static_cast(req_constants::MIN_K))), //rounds down one if odd +k_(std::max(static_cast(k) & -2, static_cast(req_constants::MIN_K))), //rounds down one if odd hra_(hra), max_nom_size_(0), num_retained_(0), @@ -40,6 +40,9 @@ min_item_(), max_item_(), sorted_view_(nullptr) { + if (k > req_constants::MAX_K) { + throw std::invalid_argument("K must be <= " + std::to_string(req_constants::MAX_K) + ": " + std::to_string(k)); + } grow(); } diff --git a/req/test/req_sketch_test.cpp b/req/test/req_sketch_test.cpp index 8de8f865..c8cf5cbe 100755 --- a/req/test/req_sketch_test.cpp +++ b/req/test/req_sketch_test.cpp @@ -54,6 +54,39 @@ TEST_CASE("req sketch: empty", "[req_sketch]") { REQUIRE_THROWS_AS(sketch.get_PMF(split_points, 1), std::runtime_error); } +TEST_CASE("req sketch: k above 255", "[req_sketch]") { + // k used to be truncated to 8 bits: 256 became 4 and 500 became 244 + REQUIRE(req_sketch(256).get_k() == 256); + REQUIRE(req_sketch(500).get_k() == 500); + REQUIRE(req_sketch(1024).get_k() == 1024); + REQUIRE(req_sketch(1001).get_k() == 1000); // rounds down if odd + + req_sketch sketch(1024); + const size_t n = 100000; + for (size_t i = 0; i < n; i++) sketch.update(static_cast(i)); + REQUIRE(sketch.is_estimation_mode()); + auto bytes = sketch.serialize(); + auto sketch2 = req_sketch::deserialize(bytes.data(), bytes.size()); + REQUIRE(sketch2.get_k() == 1024); + REQUIRE(sketch2.get_n() == n); + REQUIRE(sketch2.get_num_retained() == sketch.get_num_retained()); +} + +TEST_CASE("req sketch: k bounds", "[req_sketch]") { + for (bool hra: {true, false}) { + REQUIRE(req_sketch(0, hra).get_k() == 4); + REQUIRE(req_sketch(3, hra).get_k() == 4); + REQUIRE(req_sketch(4, hra).get_k() == 4); + REQUIRE(req_sketch(5, hra).get_k() == 4); + REQUIRE(req_sketch(1023, hra).get_k() == 1022); + REQUIRE(req_sketch(1024, hra).get_k() == 1024); + // Reject the original k, even if rounding down would put it in range. + REQUIRE_THROWS_AS(req_sketch(1025, hra), std::invalid_argument); + REQUIRE_THROWS_AS(req_sketch(1026, hra), std::invalid_argument); + REQUIRE_THROWS_AS(req_sketch(std::numeric_limits::max(), hra), std::invalid_argument); + } +} + TEST_CASE("req sketch: single value, lra", "[req_sketch]") { req_sketch sketch(12, false); sketch.update(1.0f);