avantgardnerio commented on code in PR #2294:
URL:
https://github.com/apache/datafusion-ballista/pull/2294#discussion_r3786726007
##########
ballista/core/src/kll.rs:
##########
@@ -202,6 +202,39 @@ fn level_capacity(k: usize, num_levels: usize, height:
usize) -> usize {
raw.max(MIN_LEVEL_WIDTH)
}
+/// Summarizes rather than dumping the compactor stack, which holds on the
+/// order of `3k` items and would bury whatever else a caller was printing.
+impl<T: Ord + Clone> std::fmt::Debug for KllSketch<T> {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("KllSketch")
+ .field("k", &self.k)
+ .field("count", &self.count())
+ .field("levels", &self.levels.len())
+ .field("retained",
&self.levels.iter().map(Vec::len).sum::<usize>())
+ .finish()
+ }
+}
+
+/// Clone copies the compactor stack and the tracked extremes, and gives
+/// the copy a fresh PRNG rather than duplicating the original's position.
+///
+/// Written by hand because `StdRng` is not `Clone`. Reseeding is the
+/// correct behaviour anyway: two sketches sharing a coin-flip sequence
+/// would correlate their compaction decisions, and every quantile the
+/// clone can answer is already determined by the state that was copied.
+impl<T: Ord + Clone> Clone for KllSketch<T> {
Review Comment:
> **`Clone` for `KllSketch` reseeds from `rand::random()`**, so cloning is
non-deterministic
I agree this is a tough choice. I've updated the docs to make the behavior
explicit. I think it's safest if `clone()` re-seeds. I can add a
`clone_with_seed()` test-only method if you think it's merited.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
