wrapped compact theta sketch#223
Conversation
Pull Request Test Coverage Report for Build 987577788Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
This is a comparison of union update time with compact sketches ready at hand, compact sketches with deserialize time included, and wrapped compact sketches (effectively avoiding some cost of deserialization). |
jmalkin
left a comment
jmalkin
left a comment
There was a problem hiding this comment.
maybe not a big deal, but none of the tests override the default seed to explicitly test for seed mismatch
| const size_t entries_start_u64 = has_theta ? COMPACT_SKETCH_ENTRIES_ESTIMATION_U64 : COMPACT_SKETCH_ENTRIES_EXACT_U64; | ||
| const uint64_t* entries = reinterpret_cast<const uint64_t*>(ptr) + entries_start_u64; | ||
| const size_t expected_size_bytes = (entries_start_u64 + num_entries) * sizeof(uint64_t); | ||
| if (size < expected_size_bytes) |
There was a problem hiding this comment.
would we want to count too large a size as an error?
There was a problem hiding this comment.
I am not sure. I can imagine a larger record and a user who does not know the size of the sketch. The primary goal here was to avoid reading past the end.
| const size_t expected_size_bytes = (entries_start_u64 + num_entries) * sizeof(uint64_t); | ||
| if (size < expected_size_bytes) | ||
| throw std::invalid_argument(std::to_string(expected_size_bytes) | ||
| + " bytes expected, actual " + std::to_string(size) + ", sketch dump: " + hex_dump(reinterpret_cast<const uint8_t*>(ptr), size)); |
There was a problem hiding this comment.
this dump can be quite large (MBs) in some cases. not sure if we need to consider the data security risk in that it'll happily dump arbitrary amounts of memory given a pointer and size
There was a problem hiding this comment.
Right. Perhaps this should be made configurable.
theta/test/theta_a_not_b_test.cpp
Outdated
| return std::vector<char>(other); | ||
| } | ||
|
|
||
| TEST_CASE() { |
There was a problem hiding this comment.
do we need this test case and its supporting functions?
There was a problem hiding this comment.
Sorry, this was left there accidentally after some experimenting.
|
👍 |
3 participants
No description provided.