feat: add tuple sketch implementation#152
Conversation
|
@ZENOTME Fixed, PTAL again. |
| mod private { | ||
| /// Sealed helper describing fixed-width little-endian encoding of a primitive. | ||
| pub trait LeBytes: Copy { | ||
| /// Number of bytes in the little-endian encoding. | ||
| const WIDTH: usize; | ||
| /// Appends the little-endian bytes of `self` to `out`. | ||
| fn write_le(self, out: &mut Vec<u8>); | ||
| /// Reads the value from exactly `WIDTH` leading bytes of `bytes`. | ||
| fn read_le(bytes: &[u8]) -> Self; | ||
| } | ||
|
|
||
| macro_rules! impl_le_bytes { | ||
| ($($t:ty),* $(,)?) => { | ||
| $( | ||
| impl LeBytes for $t { | ||
| const WIDTH: usize = std::mem::size_of::<$t>(); | ||
|
|
||
| fn write_le(self, out: &mut Vec<u8>) { | ||
| out.extend_from_slice(&self.to_le_bytes()); | ||
| } | ||
|
|
||
| fn read_le(bytes: &[u8]) -> Self { | ||
| let mut buf = [0u8; std::mem::size_of::<$t>()]; | ||
| buf.copy_from_slice(&bytes[..std::mem::size_of::<$t>()]); | ||
| <$t>::from_le_bytes(buf) | ||
| } | ||
| } | ||
| )* | ||
| }; | ||
| } | ||
|
|
||
| impl_le_bytes!(u32, u64, i32, i64, f32, f64); | ||
| } |
There was a problem hiding this comment.
Why we need this? The SketchSlice and SketchBytes in the codec module should implement all these functionalities. Or we should add new utilities there instead of randomly add some code in dedicated skecthes modules.
There was a problem hiding this comment.
SketchBytes / SketchSlice only offer per-type concrete methods (write_u64_le, read_f32_le, ...), but PrimitiveSummarySerde is a generic impl<S> SummarySerde<S> — it needs a trait to dispatch on the type's LE width and encoding, so the abstraction itself can't go away.
Moved LeBytes to codec/primitive.rs as a pub(crate) utility instead of hiding it in the tuple module; frequencies' impl_primitive! could migrate to it later if desired.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
Generally LGTM. Comments inline.
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
LGTM.
I pushed several commits to do post fine tune.
I'm going to submit a new PR to rework the Builder interface. Basically, no need for several construction point and avoid some strange interface change.
| #[cfg(feature = "theta")] | ||
| pub mod theta; | ||
| #[cfg(any(feature = "theta", feature = "tuple"))] | ||
| #[allow(dead_code)] // some utilities are only used for certain sketches |
There was a problem hiding this comment.
To be reviewed once the full tuple sketch implementation landed.
|
Thx for your patient review, let's move on. |
See #158 |
3 participants
Summary
Implement the Tuple sketch behind a new
tuplefeature (built on top oftheta). A Tuple sketch extends the Theta sketch by attaching a user-defined summary to every retained key, merging summaries on duplicate keys and during set operations.Summary behavior is injected externally via policies (C++-style) rather than baked into the type.
Note
This implementation is highly inspired by the C++ version.