diff --git a/CHANGELOG.md b/CHANGELOG.md index 32b069ed..03756cb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,14 @@ signals a backwards-compatible change. ### Fixed +- **The portable `CountSketch` hashes every row independently at any width.** + It sliced every row's column and sign from one 64-bit hash whatever the + shape, so once `rows * (ceil_log2(cols) + 1)` passed 64, deep rows reused + shallow rows' bits (at `30000` columns only about four rows were + independent) and debug builds panicked on the shift. It now lays the hash + out by shape as the native fast-path sketches do: one 64-bit hash while the + rows fit, then one 128-bit hash, then one hash per row. Shapes that fit 64 + bits keep their cells, so the Go parity probe is unchanged. - `KLLDynamic` panicked on every quantile query once a NaN had entered the stream: the CDF was sorted with `partial_cmp(..).unwrap()`. It sorts with `total_cmp`, as the three sibling sorts beside it already did. diff --git a/docs/tests.md b/docs/tests.md index ccb1571c..00d52d71 100644 --- a/docs/tests.md +++ b/docs/tests.md @@ -1260,6 +1260,8 @@ Test file: [`src/message_pack_format/portable/countsketch.rs`](../src/message_pa | test_name | test_description | what_is_tested | | --- | --- | --- | +| `deep_shapes_hash_every_row_independently` | A shape too deep for one 64-bit hash hashes every row independently and does not panic. | For a `15x30000` sketch (15 column bits and a sign bit per row), verifies one update of `2.5` reads back `2.5`, and that over 200 keys no two rows put more than 2 keys in the same column. | +| `packed_shapes_keep_their_cells` | A shape that fits one 64-bit hash keeps the Go-parity cells. | For a `5x1024` sketch, verifies each row's column for `"api"` is the column the packed 64-bit hash at seed index `0` gives that row. | | `msgpack_delta_against_empty_round_trips` | A msgpack delta against an empty snapshot carries the signed cells exactly. | For a `5x1024` sketch fed 200 updates of `"hot"` at weight `1.0`, verifies the delta against a fresh sketch at threshold `1.0`, applied to another fresh sketch, reproduces `sketch()` cell for cell. | | `msgpack_delta_wire_layout_is_parallel_arrays` | The delta is a five-element array of scalars plus parallel index arrays. | For a `3x8` sketch fed `"x"` at weight `5.0`, verifies the delta bytes equal `rmp_serde::to_vec(&(rows as u64, cols as u64, row_idx: Vec, col_idx: Vec, d_count: Vec))` recomputed from the non-zero cells, and that decoding reports `rows` `3` and `cols` `8`. | | `test_new_empty` | A fresh sketch has the requested shape and zeroed cells. | For `CountSketch::new(2, 3)`, verifies `rows` is `2`, `cols` is `3`, and `sketch()` is `[[0.0, 0.0, 0.0], [0.0, 0.0, 0.0]]`. | diff --git a/src/message_pack_format/portable/countsketch.rs b/src/message_pack_format/portable/countsketch.rs index a94ad981..6e1a9461 100644 --- a/src/message_pack_format/portable/countsketch.rs +++ b/src/message_pack_format/portable/countsketch.rs @@ -148,24 +148,29 @@ impl CountSketch { } } - /// Insert a single weighted observation. Uses the shared - /// [`crate::MatrixHashType::Packed64`] fast-path hash so the matrix-cell - /// layout matches `sketchlib-go::CountSketch.UpdateString` - /// bit-for-bit. Locked in by - /// `tests/sketches_go_parity_probe.rs`. - /// - /// Constraints inherited from Go's Packed64 mode: - /// - `cols` must be a power of two; the column mask is `cols - 1`. - /// - `rows * (log2(cols) + 1) ≤ 64` so the packed hash holds all - /// row indices and sign bits. + /// The key's per-row hash, laid out for this shape as the native + /// fast-path sketches lay it out: one 64-bit hash while every row's + /// column bits and sign bit fit in it, then one 128-bit hash, then one + /// hash per row. Packed rows therefore never share hash bits, and a shape + /// that fits 64 bits keeps the cells `sketchlib-go::CountSketch.UpdateString` + /// gives it (locked in by `tests/sketches_go_parity_probe.rs`). One hash + /// per row draws row `r`'s seed from the seed list, so past + /// [`crate::MATRIX_MAX_ROWS`] rows, row `r + MATRIX_MAX_ROWS` repeats row `r`. + fn row_hash(&self, key: &str) -> crate::common::MatrixHashType { + crate::common::hash_for_matrix_seeded( + 0, + self.rows, + self.cols, + &crate::common::DataInput::String(key.to_owned()), + ) + } + + /// Insert a single weighted observation, hashed as [`Self::row_hash`] says. pub fn update(&mut self, key: &str, value: f64) { if self.rows == 0 || self.cols == 0 { return; } - let input = crate::common::DataInput::String(key.to_owned()); - let hashed = crate::common::MatrixHashType::Packed64( - ::hash64_seeded(0, &input), - ); + let hashed = self.row_hash(key); for r in 0..self.rows { let col = ::col_for_row( &hashed, r, self.cols, @@ -185,10 +190,7 @@ impl CountSketch { if self.rows == 0 || self.cols == 0 { return 0.0; } - let input = crate::common::DataInput::String(key.to_owned()); - let hashed = crate::common::MatrixHashType::Packed64( - ::hash64_seeded(0, &input), - ); + let hashed = self.row_hash(key); let mut estimates: Vec = Vec::with_capacity(self.rows); for r in 0..self.rows { let col = ::col_for_row( @@ -541,6 +543,57 @@ impl MessagePackCodec for CountSketchDelta { mod tests { use super::*; + /// The column each row gives `key`, read off a fresh sketch. + fn columns(rows: usize, cols: usize, key: &str) -> Vec { + let mut sketch = CountSketch::new(rows, cols); + sketch.update(key, 1.0); + sketch + .matrix + .iter() + .map(|row| row.iter().position(|cell| *cell != 0.0).unwrap()) + .collect() + } + + /// A shape too deep for one 64-bit hash (15 rows of 15 column bits and a + /// sign) reads back its weight, and its rows place keys independently: + /// two rows rarely put a key in the same column. + #[test] + fn deep_shapes_hash_every_row_independently() { + let (rows, cols) = (15, 30_000); + let mut sketch = CountSketch::new(rows, cols); + sketch.update("api", 2.5); + assert_eq!(sketch.estimate("api"), 2.5); + let placed: Vec> = (0..200) + .map(|i| columns(rows, cols, &format!("key-{i}"))) + .collect(); + for a in 0..rows { + for b in a + 1..rows { + let shared = placed.iter().filter(|c| c[a] == c[b]).count(); + // Expected 200 / 30000 < 1 per pair. + assert!(shared <= 2, "rows {a} and {b} share {shared} columns"); + } + } + } + + /// A shape that fits one 64-bit hash keeps its cells: the layout the Go + /// parity probe pins does not move. + #[test] + fn packed_shapes_keep_their_cells() { + let hash = ::hash64_seeded( + 0, + &crate::common::DataInput::String("api".to_owned()), + ); + let packed = crate::common::MatrixHashType::Packed64(hash); + let expected: Vec = (0..5) + .map(|row| { + ::col_for_row( + &packed, row, 1024, + ) + }) + .collect(); + assert_eq!(columns(5, 1024, "api"), expected); + } + #[test] fn msgpack_delta_against_empty_round_trips() { let (rows, cols) = (5, 1024);