jmalkin commented on a change in pull request #209:
URL: https://github.com/apache/datasketches-cpp/pull/209#discussion_r612050486
##########
File path: cpc/include/cpc_compressor_impl.hpp
##########
@@ -64,17 +64,17 @@ uint8_t* cpc_compressor<A>::make_inverse_permutation(const
uint8_t* permu, int l
of length at most 12, this builds a size-4096 decoding table */
// The second argument is typically 256, but can be other values such as 65.
template<typename A>
-uint16_t* cpc_compressor<A>::make_decoding_table(const uint16_t*
encoding_table, int num_byte_values) {
+uint16_t* cpc_compressor<A>::make_decoding_table(const uint16_t*
encoding_table, unsigned num_byte_values) {
uint16_t* decoding_table = new uint16_t[4096]; // use new for global
initialization
- for (int byte_value = 0; byte_value < num_byte_values; byte_value++) {
- const int encoding_entry = encoding_table[byte_value];
- const int code_value = encoding_entry & 0xfff;
- const int code_length = encoding_entry >> 12;
- const int decoding_entry = (code_length << 8) | byte_value;
- const int garbage_length = 12 - code_length;
- const int num_copies = 1 << garbage_length;
- for (int garbage_bits = 0; garbage_bits < num_copies; garbage_bits++) {
- const int extended_code_value = code_value | (garbage_bits <<
code_length);
+ for (unsigned byte_value = 0; byte_value < num_byte_values; byte_value++) {
+ const uint16_t encoding_entry = encoding_table[byte_value];
+ const uint16_t code_value = encoding_entry & 0xfff;
+ const uint8_t code_length = encoding_entry >> 12;
+ const uint16_t decoding_entry = static_cast<uint16_t>((code_length << 8) |
byte_value);
+ const uint8_t garbage_length = 12 - code_length;
+ const unsigned num_copies = 1 << garbage_length;
Review comment:
why `unsigned` rather than `uint32_t` in these cases?
##########
File path: theta/include/theta_jaccard_similarity_base.hpp
##########
@@ -131,9 +131,9 @@ class jaccard_similarity_base {
template<typename SketchA, typename SketchB>
static typename Union::CompactSketch compute_union(const SketchA& sketch_a,
const SketchB& sketch_b) {
- const unsigned count_a = sketch_a.get_num_retained();
- const unsigned count_b = sketch_b.get_num_retained();
- const unsigned lg_k = std::min(std::max(log2(ceiling_power_of_2(count_a +
count_b)), theta_constants::MIN_LG_K), theta_constants::MAX_LG_K);
+ const auto count_a = sketch_a.get_num_retained();
+ const auto count_b = sketch_b.get_num_retained();
+ const auto lg_k = std::min(std::max(log2(ceiling_power_of_2(count_a +
count_b)), theta_constants::MIN_LG_K), theta_constants::MAX_LG_K);
Review comment:
since we use `lg_k` to set the value in the union, shouldn't we know the
type we're getting and not need to use `auto`?
##########
File path: hll/test/HllArrayTest.cpp
##########
@@ -109,83 +109,83 @@ TEST_CASE("hll array: check corrupt bytearray",
"[hll_array]") {
uint8_t* bytes = sketchBytes.data();
const size_t size = sketchBytes.size();
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = 0;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
REQUIRE_THROWS_AS(HllArray<std::allocator<uint8_t>>::newHll(bytes, size,
std::allocator<uint8_t>()), std::invalid_argument);
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = HllUtil<>::HLL_PREINTS;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = hll_constants::HLL_PREINTS;
- bytes[HllUtil<>::SER_VER_BYTE] = 0;
+ bytes[hll_constants::SER_VER_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::SER_VER_BYTE] = HllUtil<>::SER_VER;
+ bytes[hll_constants::SER_VER_BYTE] = hll_constants::SER_VER;
- bytes[HllUtil<>::FAMILY_BYTE] = 0;
+ bytes[hll_constants::FAMILY_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::FAMILY_BYTE] = HllUtil<>::FAMILY_ID;
+ bytes[hll_constants::FAMILY_BYTE] = hll_constants::FAMILY_ID;
- uint8_t tmp = bytes[HllUtil<>::MODE_BYTE];
- bytes[HllUtil<>::MODE_BYTE] = 0x10; // HLL_6, LIST
+ uint8_t tmp = bytes[hll_constants::MODE_BYTE];
+ bytes[hll_constants::MODE_BYTE] = 0x10; // HLL_6, LIST
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::MODE_BYTE] = tmp;
+ bytes[hll_constants::MODE_BYTE] = tmp;
- tmp = bytes[HllUtil<>::LG_ARR_BYTE];
- bytes[HllUtil<>::LG_ARR_BYTE] = 0;
+ tmp = bytes[hll_constants::LG_ARR_BYTE];
+ bytes[hll_constants::LG_ARR_BYTE] = 0;
hll_sketch::deserialize(bytes, size);
// should work fine despite the corruption
- bytes[HllUtil<>::LG_ARR_BYTE] = tmp;
+ bytes[hll_constants::LG_ARR_BYTE] = tmp;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size - 1),
std::out_of_range);
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, 3), std::out_of_range);
}
TEST_CASE("hll array: check corrupt stream", "[hll_array]") {
- int lgK = 6;
+ uint8_t lgK = 6;
hll_sketch sk1(lgK);
for (int i = 0; i < 50; ++i) {
sk1.update(i);
}
std::stringstream ss;
sk1.serialize_compact(ss);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
REQUIRE_THROWS_AS(HllArray<std::allocator<uint8_t>>::newHll(ss,
std::allocator<uint8_t>()), std::invalid_argument);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
- ss.put(HllUtil<>::HLL_PREINTS);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
+ ss.put(hll_constants::HLL_PREINTS);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
+ ss.seekp(hll_constants::SER_VER_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
- ss.put(HllUtil<>::SER_VER);
+ ss.seekp(hll_constants::SER_VER_BYTE);
+ ss.put(hll_constants::SER_VER);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
+ ss.seekp(hll_constants::FAMILY_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
- ss.put(HllUtil<>::FAMILY_ID);
+ ss.seekp(hll_constants::FAMILY_BYTE);
+ ss.put(hll_constants::FAMILY_ID);
- ss.seekg(HllUtil<>::MODE_BYTE);
- uint8_t tmp = ss.get();
- ss.seekp(HllUtil<>::MODE_BYTE);
+ ss.seekg(hll_constants::MODE_BYTE);
+ auto tmp = ss.get();
+ ss.seekp(hll_constants::MODE_BYTE);
ss.put(0x11); // HLL_6, SET
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::MODE_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::MODE_BYTE);
+ ss.put((char)tmp);
- ss.seekg(HllUtil<>::LG_ARR_BYTE);
+ ss.seekg(hll_constants::LG_ARR_BYTE);
tmp = ss.get();
- ss.seekp(HllUtil<>::LG_ARR_BYTE);
+ ss.seekp(hll_constants::LG_ARR_BYTE);
ss.put(0);
ss.seekg(0);
hll_sketch::deserialize(ss);
// should work fine despite the corruption
- ss.seekp(HllUtil<>::LG_ARR_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::LG_ARR_BYTE);
+ ss.put((char)tmp);
Review comment:
and again
##########
File path: hll/test/CouponHashSetTest.cpp
##########
@@ -39,91 +39,91 @@ TEST_CASE("coupon hash set: check corrupt bytearray",
"[coupon_hash_set]") {
uint8_t* bytes = sketchBytes.data();
const size_t size = sketchBytes.size();
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = 0;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = 0;
// fail in HllSketchImpl
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
// fail in CouponHashSet
REQUIRE_THROWS_AS(CouponHashSet<std::allocator<uint8_t>>::newSet(bytes,
size, std::allocator<uint8_t>()), std::invalid_argument);
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = HllUtil<>::HASH_SET_PREINTS;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = hll_constants::HASH_SET_PREINTS;
- bytes[HllUtil<>::SER_VER_BYTE] = 0;
+ bytes[hll_constants::SER_VER_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::SER_VER_BYTE] = HllUtil<>::SER_VER;
+ bytes[hll_constants::SER_VER_BYTE] = hll_constants::SER_VER;
- bytes[HllUtil<>::FAMILY_BYTE] = 0;
+ bytes[hll_constants::FAMILY_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::FAMILY_BYTE] = HllUtil<>::FAMILY_ID;
+ bytes[hll_constants::FAMILY_BYTE] = hll_constants::FAMILY_ID;
- bytes[HllUtil<>::LG_K_BYTE] = 6;
+ bytes[hll_constants::LG_K_BYTE] = 6;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::LG_K_BYTE] = lgK;
+ bytes[hll_constants::LG_K_BYTE] = lgK;
- uint8_t tmp = bytes[HllUtil<>::MODE_BYTE];
- bytes[HllUtil<>::MODE_BYTE] = 0x10; // HLL_6, LIST
+ uint8_t tmp = bytes[hll_constants::MODE_BYTE];
+ bytes[hll_constants::MODE_BYTE] = 0x10; // HLL_6, LIST
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::MODE_BYTE] = tmp;
+ bytes[hll_constants::MODE_BYTE] = tmp;
- tmp = bytes[HllUtil<>::LG_ARR_BYTE];
- bytes[HllUtil<>::LG_ARR_BYTE] = 0;
+ tmp = bytes[hll_constants::LG_ARR_BYTE];
+ bytes[hll_constants::LG_ARR_BYTE] = 0;
hll_sketch::deserialize(bytes, size);
// should work fine despite the corruption
- bytes[HllUtil<>::LG_ARR_BYTE] = tmp;
+ bytes[hll_constants::LG_ARR_BYTE] = tmp;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size - 1),
std::out_of_range);
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, 3), std::out_of_range);
}
TEST_CASE("coupon hash set: check corrupt stream", "[coupon_hash_set]") {
- int lgK = 9;
+ uint8_t lgK = 9;
hll_sketch sk1(lgK);
for (int i = 0; i < 24; ++i) {
sk1.update(i);
}
std::stringstream ss;
sk1.serialize_compact(ss);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
ss.put(0);
ss.seekg(0);
// fail in HllSketchImpl
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
// fail in CouponHashSet
REQUIRE_THROWS_AS(CouponHashSet<std::allocator<uint8_t>>::newSet(ss,
std::allocator<uint8_t>()), std::invalid_argument);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
- ss.put(HllUtil<>::HASH_SET_PREINTS);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
+ ss.put(hll_constants::HASH_SET_PREINTS);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
+ ss.seekp(hll_constants::SER_VER_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
- ss.put(HllUtil<>::SER_VER);
+ ss.seekp(hll_constants::SER_VER_BYTE);
+ ss.put(hll_constants::SER_VER);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
+ ss.seekp(hll_constants::FAMILY_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
- ss.put(HllUtil<>::FAMILY_ID);
+ ss.seekp(hll_constants::FAMILY_BYTE);
+ ss.put(hll_constants::FAMILY_ID);
- ss.seekg(HllUtil<>::MODE_BYTE);
- uint8_t tmp = ss.get();
- ss.seekp(HllUtil<>::MODE_BYTE);
+ ss.seekg(hll_constants::MODE_BYTE);
+ auto tmp = ss.get();
+ ss.seekp(hll_constants::MODE_BYTE);
ss.put(0x22); // HLL_8, HLL
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::MODE_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::MODE_BYTE);
+ ss.put((char)tmp);
- ss.seekg(HllUtil<>::LG_ARR_BYTE);
+ ss.seekg(hll_constants::LG_ARR_BYTE);
tmp = ss.get();
- ss.seekp(HllUtil<>::LG_ARR_BYTE);
+ ss.seekp(hll_constants::LG_ARR_BYTE);
ss.put(0);
ss.seekg(0);
hll_sketch::deserialize(ss);
// should work fine despite the corruption
- ss.seekp(HllUtil<>::LG_ARR_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::LG_ARR_BYTE);
+ ss.put((char)tmp);
Review comment:
and again. we might not care, but pointing it out.
##########
File path: hll/include/HllUnion-internal.hpp
##########
@@ -32,151 +32,151 @@
namespace datasketches {
template<typename A>
-hll_union_alloc<A>::hll_union_alloc(const int lg_max_k, const A& allocator):
- lg_max_k(HllUtil<A>::checkLgK(lg_max_k)),
- gadget(lg_max_k, target_hll_type::HLL_8, false, allocator)
+hll_union_alloc<A>::hll_union_alloc(uint8_t lg_max_k, const A& allocator):
+ lg_max_k_(HllUtil<A>::checkLgK(lg_max_k)),
+ gadget_(lg_max_k, target_hll_type::HLL_8, false, allocator)
{}
template<typename A>
hll_sketch_alloc<A> hll_union_alloc<A>::get_result(target_hll_type
target_type) const {
- return hll_sketch_alloc<A>(gadget, target_type);
+ return hll_sketch_alloc<A>(gadget_, target_type);
}
template<typename A>
void hll_union_alloc<A>::update(const hll_sketch_alloc<A>& sketch) {
if (sketch.is_empty()) return;
- union_impl(sketch, lg_max_k);
+ union_impl(sketch, lg_max_k_);
}
template<typename A>
void hll_union_alloc<A>::update(hll_sketch_alloc<A>&& sketch) {
if (sketch.is_empty()) return;
- if (gadget.is_empty() && sketch.get_target_type() == HLL_8 &&
sketch.get_lg_config_k() <= lg_max_k) {
- if (sketch.get_current_mode() == HLL || sketch.get_lg_config_k() ==
lg_max_k) {
- gadget = std::move(sketch);
+ if (gadget_.is_empty() && sketch.get_target_type() == HLL_8 &&
sketch.get_lg_config_k() <= lg_max_k_) {
+ if (sketch.get_current_mode() == HLL || sketch.get_lg_config_k() ==
lg_max_k_) {
+ gadget_ = std::move(sketch);
}
}
- union_impl(sketch, lg_max_k);
+ union_impl(sketch, lg_max_k_);
}
template<typename A>
void hll_union_alloc<A>::update(const std::string& datum) {
- gadget.update(datum);
+ gadget_.update(datum);
}
template<typename A>
void hll_union_alloc<A>::update(const uint64_t datum) {
Review comment:
removed const from all the other primitive inputs. accidentally left
this one?
##########
File path: common/include/serde.hpp
##########
@@ -99,11 +99,11 @@ struct serde<std::string> {
bool failure = false;
try {
for (; i < num && os.good(); i++) {
- uint32_t length = items[i].size();
+ uint32_t length = static_cast<uint32_t>(items[i].size());
Review comment:
should we be concerned that this is 32 bits rather than 64? (i think no
but just wanted to mention it)
##########
File path: hll/test/HllArrayTest.cpp
##########
@@ -109,83 +109,83 @@ TEST_CASE("hll array: check corrupt bytearray",
"[hll_array]") {
uint8_t* bytes = sketchBytes.data();
const size_t size = sketchBytes.size();
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = 0;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
REQUIRE_THROWS_AS(HllArray<std::allocator<uint8_t>>::newHll(bytes, size,
std::allocator<uint8_t>()), std::invalid_argument);
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = HllUtil<>::HLL_PREINTS;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = hll_constants::HLL_PREINTS;
- bytes[HllUtil<>::SER_VER_BYTE] = 0;
+ bytes[hll_constants::SER_VER_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::SER_VER_BYTE] = HllUtil<>::SER_VER;
+ bytes[hll_constants::SER_VER_BYTE] = hll_constants::SER_VER;
- bytes[HllUtil<>::FAMILY_BYTE] = 0;
+ bytes[hll_constants::FAMILY_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::FAMILY_BYTE] = HllUtil<>::FAMILY_ID;
+ bytes[hll_constants::FAMILY_BYTE] = hll_constants::FAMILY_ID;
- uint8_t tmp = bytes[HllUtil<>::MODE_BYTE];
- bytes[HllUtil<>::MODE_BYTE] = 0x10; // HLL_6, LIST
+ uint8_t tmp = bytes[hll_constants::MODE_BYTE];
+ bytes[hll_constants::MODE_BYTE] = 0x10; // HLL_6, LIST
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::MODE_BYTE] = tmp;
+ bytes[hll_constants::MODE_BYTE] = tmp;
- tmp = bytes[HllUtil<>::LG_ARR_BYTE];
- bytes[HllUtil<>::LG_ARR_BYTE] = 0;
+ tmp = bytes[hll_constants::LG_ARR_BYTE];
+ bytes[hll_constants::LG_ARR_BYTE] = 0;
hll_sketch::deserialize(bytes, size);
// should work fine despite the corruption
- bytes[HllUtil<>::LG_ARR_BYTE] = tmp;
+ bytes[hll_constants::LG_ARR_BYTE] = tmp;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size - 1),
std::out_of_range);
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, 3), std::out_of_range);
}
TEST_CASE("hll array: check corrupt stream", "[hll_array]") {
- int lgK = 6;
+ uint8_t lgK = 6;
hll_sketch sk1(lgK);
for (int i = 0; i < 50; ++i) {
sk1.update(i);
}
std::stringstream ss;
sk1.serialize_compact(ss);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
REQUIRE_THROWS_AS(HllArray<std::allocator<uint8_t>>::newHll(ss,
std::allocator<uint8_t>()), std::invalid_argument);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
- ss.put(HllUtil<>::HLL_PREINTS);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
+ ss.put(hll_constants::HLL_PREINTS);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
+ ss.seekp(hll_constants::SER_VER_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
- ss.put(HllUtil<>::SER_VER);
+ ss.seekp(hll_constants::SER_VER_BYTE);
+ ss.put(hll_constants::SER_VER);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
+ ss.seekp(hll_constants::FAMILY_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
- ss.put(HllUtil<>::FAMILY_ID);
+ ss.seekp(hll_constants::FAMILY_BYTE);
+ ss.put(hll_constants::FAMILY_ID);
- ss.seekg(HllUtil<>::MODE_BYTE);
- uint8_t tmp = ss.get();
- ss.seekp(HllUtil<>::MODE_BYTE);
+ ss.seekg(hll_constants::MODE_BYTE);
+ auto tmp = ss.get();
+ ss.seekp(hll_constants::MODE_BYTE);
ss.put(0x11); // HLL_6, SET
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::MODE_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::MODE_BYTE);
+ ss.put((char)tmp);
Review comment:
(cast)
##########
File path: hll/test/CouponHashSetTest.cpp
##########
@@ -39,91 +39,91 @@ TEST_CASE("coupon hash set: check corrupt bytearray",
"[coupon_hash_set]") {
uint8_t* bytes = sketchBytes.data();
const size_t size = sketchBytes.size();
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = 0;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = 0;
// fail in HllSketchImpl
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
// fail in CouponHashSet
REQUIRE_THROWS_AS(CouponHashSet<std::allocator<uint8_t>>::newSet(bytes,
size, std::allocator<uint8_t>()), std::invalid_argument);
- bytes[HllUtil<>::PREAMBLE_INTS_BYTE] = HllUtil<>::HASH_SET_PREINTS;
+ bytes[hll_constants::PREAMBLE_INTS_BYTE] = hll_constants::HASH_SET_PREINTS;
- bytes[HllUtil<>::SER_VER_BYTE] = 0;
+ bytes[hll_constants::SER_VER_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::SER_VER_BYTE] = HllUtil<>::SER_VER;
+ bytes[hll_constants::SER_VER_BYTE] = hll_constants::SER_VER;
- bytes[HllUtil<>::FAMILY_BYTE] = 0;
+ bytes[hll_constants::FAMILY_BYTE] = 0;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::FAMILY_BYTE] = HllUtil<>::FAMILY_ID;
+ bytes[hll_constants::FAMILY_BYTE] = hll_constants::FAMILY_ID;
- bytes[HllUtil<>::LG_K_BYTE] = 6;
+ bytes[hll_constants::LG_K_BYTE] = 6;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::LG_K_BYTE] = lgK;
+ bytes[hll_constants::LG_K_BYTE] = lgK;
- uint8_t tmp = bytes[HllUtil<>::MODE_BYTE];
- bytes[HllUtil<>::MODE_BYTE] = 0x10; // HLL_6, LIST
+ uint8_t tmp = bytes[hll_constants::MODE_BYTE];
+ bytes[hll_constants::MODE_BYTE] = 0x10; // HLL_6, LIST
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size),
std::invalid_argument);
- bytes[HllUtil<>::MODE_BYTE] = tmp;
+ bytes[hll_constants::MODE_BYTE] = tmp;
- tmp = bytes[HllUtil<>::LG_ARR_BYTE];
- bytes[HllUtil<>::LG_ARR_BYTE] = 0;
+ tmp = bytes[hll_constants::LG_ARR_BYTE];
+ bytes[hll_constants::LG_ARR_BYTE] = 0;
hll_sketch::deserialize(bytes, size);
// should work fine despite the corruption
- bytes[HllUtil<>::LG_ARR_BYTE] = tmp;
+ bytes[hll_constants::LG_ARR_BYTE] = tmp;
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, size - 1),
std::out_of_range);
REQUIRE_THROWS_AS(hll_sketch::deserialize(bytes, 3), std::out_of_range);
}
TEST_CASE("coupon hash set: check corrupt stream", "[coupon_hash_set]") {
- int lgK = 9;
+ uint8_t lgK = 9;
hll_sketch sk1(lgK);
for (int i = 0; i < 24; ++i) {
sk1.update(i);
}
std::stringstream ss;
sk1.serialize_compact(ss);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
ss.put(0);
ss.seekg(0);
// fail in HllSketchImpl
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
// fail in CouponHashSet
REQUIRE_THROWS_AS(CouponHashSet<std::allocator<uint8_t>>::newSet(ss,
std::allocator<uint8_t>()), std::invalid_argument);
- ss.seekp(HllUtil<>::PREAMBLE_INTS_BYTE);
- ss.put(HllUtil<>::HASH_SET_PREINTS);
+ ss.seekp(hll_constants::PREAMBLE_INTS_BYTE);
+ ss.put(hll_constants::HASH_SET_PREINTS);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
+ ss.seekp(hll_constants::SER_VER_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::SER_VER_BYTE);
- ss.put(HllUtil<>::SER_VER);
+ ss.seekp(hll_constants::SER_VER_BYTE);
+ ss.put(hll_constants::SER_VER);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
+ ss.seekp(hll_constants::FAMILY_BYTE);
ss.put(0);
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::FAMILY_BYTE);
- ss.put(HllUtil<>::FAMILY_ID);
+ ss.seekp(hll_constants::FAMILY_BYTE);
+ ss.put(hll_constants::FAMILY_ID);
- ss.seekg(HllUtil<>::MODE_BYTE);
- uint8_t tmp = ss.get();
- ss.seekp(HllUtil<>::MODE_BYTE);
+ ss.seekg(hll_constants::MODE_BYTE);
+ auto tmp = ss.get();
+ ss.seekp(hll_constants::MODE_BYTE);
ss.put(0x22); // HLL_8, HLL
ss.seekg(0);
REQUIRE_THROWS_AS(hll_sketch::deserialize(ss), std::invalid_argument);
- ss.seekp(HllUtil<>::MODE_BYTE);
- ss.put(tmp);
+ ss.seekp(hll_constants::MODE_BYTE);
+ ss.put((char)tmp);
Review comment:
this is a c-style cast!
##########
File path: kll/include/kll_quantile_calculator_impl.hpp
##########
@@ -102,11 +102,11 @@ uint32_t kll_quantile_calculator<T, C,
A>::chunk_containing_pos(uint64_t pos) co
}
template <typename T, typename C, typename A>
-uint32_t kll_quantile_calculator<T, C,
A>::search_for_chunk_containing_pos(uint64_t pos, uint32_t l, uint32_t r) const
{
+uint32_t kll_quantile_calculator<T, C,
A>::search_for_chunk_containing_pos(uint64_t pos, uint64_t l, uint64_t r) const
{
Review comment:
any particular reason `l` and `r` are uint64_t if we can be certain that
we can always safely cast the result to uint32_t?
##########
File path: kll/test/kll_sketch_test.cpp
##########
@@ -115,7 +114,7 @@ TEST_CASE("kll sketch", "[kll_sketch]") {
sketch.update(std::numeric_limits<float>::quiet_NaN());
REQUIRE(sketch.is_empty());
- sketch.update(0.0);
+ sketch.update(0);
Review comment:
with the addition of `.0f` to all the other inputs, removing the decimal
jumped out. how odd it's ok with type conversion of zero but not other types.
##########
File path: tuple/include/tuple_sketch_impl.hpp
##########
@@ -315,7 +315,7 @@ uint64_t compact_tuple_sketch<S, A>::get_theta64() const {
template<typename S, typename A>
uint32_t compact_tuple_sketch<S, A>::get_num_retained() const {
- return entries_.size();
+ return static_cast<uint32_t>(entries_.size());
Review comment:
We need to do this cast enough that i wonder if we should define a
`getNumEntries()` method to handle it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
