refactor(theta): theta hash table into raw table, preparing for tuple reuse#146
Conversation
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
I remember we reject a general Hashable solution as in #118.
But this PR anyway seems like to introduce RawHashTableEntry rather than Hashable.
I noticed that it's a crate-level trait. Would it benefits from being an enum rather than a trait?
enum means that entry can be different type in single hash table. But in our case, hash table is reuse as Seems trait is more clear.
This hash table is for reuse logic between tuple and theta sketch. Not export to user. |
|
SGTM. We can merge #145 now and rebase this PR onto it and continue the review :D |
We can merge this first actually. Otherwise, this PR need to rewrite union. 🤣 |
| pub(crate) const MAX_THETA: u64 = i64::MAX as u64; | ||
| /// Minimum log2 of K | ||
| const MIN_LG_K: u8 = 5; | ||
| pub(crate) const MIN_LG_K: u8 = 5; | ||
| /// Maximum log2 of K | ||
| const MAX_LG_K: u8 = 26; | ||
| pub(crate) const MAX_LG_K: u8 = 26; | ||
| /// Default log2 of K | ||
| const DEFAULT_LG_K: u8 = 12; | ||
| pub(crate) const DEFAULT_LG_K: u8 = 12; | ||
| /// Resize threshold (0.5 = 50% load factor) | ||
| const HASH_TABLE_RESIZE_THRESHOLD: f64 = 0.5; | ||
| pub(crate) const HASH_TABLE_RESIZE_THRESHOLD: f64 = 0.5; | ||
| /// Rebuild threshold (15/16 = 93.75% load factor) | ||
| const HASH_TABLE_REBUILD_THRESHOLD: f64 = 15.0 / 16.0; | ||
| pub(crate) const HASH_TABLE_REBUILD_THRESHOLD: f64 = 15.0 / 16.0; | ||
| /// Stride hash bits (7 bits for stride calculation) | ||
| pub(crate) const STRIDE_HASH_BITS: u8 = 7; | ||
| /// Stride mask | ||
| pub(crate) const STRIDE_MASK: u64 = (1 << STRIDE_HASH_BITS) - 1; |
There was a problem hiding this comment.
These pub(crate) should be useless. They are limited to the theta module and can be already accessed by any sub module.
There was a problem hiding this comment.
These are defined as pub(crate) because they will be used in subsequent PRs in the stacked PRs; the tuple implementation requires using these constants.
If you feel this PR shouldn't be written this way, I can make the changes.
There was a problem hiding this comment.
Fair enough. Then let's keep the pub(crate) structure now and do a review (refactor) later when all the function code landed.
There was a problem hiding this comment.
I guess we can finally move the shared code to a datasketches/thetacommon module later. This is how datasketches-java organizes code also.
But let's review this then.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
LGTM. Further improvement suggestions can be addressed once we get all the functions landed.
3 participants
Summary
As @ZENOTME mentioned, we should first extract a reusable hashtable trait for the upcoming tuple implementation.
related: #145