feat: impl densitysketch #62
Open
+868
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tisonkun
reviewed
Jan 7, 2026
Signed-off-by: Chojan Shang <psiace@apache.org>
notfilippo
reviewed
Jan 16, 2026
|
|
||
| /// Density sketch for streaming density estimation. | ||
| pub struct DensitySketch<T: DensityValue> { | ||
| kernel: Box<dyn DensityKernel<T>>, |
Contributor
There was a problem hiding this comment.
Are the any reasons not to keep the kernel as a generic argument of type K: DensityKernel<T>?
| } | ||
| } | ||
|
|
||
| fn check_k(k: u16) { |
Contributor
There was a problem hiding this comment.
Can you move this directly where it's called. I think having asserts inline really helps with the readability when people are exploring code.
notfilippo
reviewed
Jan 16, 2026
| } | ||
|
|
||
| thread_local! { | ||
| static RNG_STATE: Cell<u64> = Cell::new(seed_rng()); |
Contributor
There was a problem hiding this comment.
I would love not to have thread local variables declared statically in the library. Is this really something we cannot store in another struct and pass it to the sketch when needed, delegating lifecycle decisions to the user?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Density sketch implementation for density estimation from streaming data.
Ref https://github.com/apache/datasketches-cpp/tree/master/density