refactor: migrate to asap_sketchlib module-restructure layout

[GordonYuanyc]

Summary

• Bump asap_sketchlib pin to the refactor/module-restructure branch and adapt to the new module layout.
• Accumulators now hold sketches::* types directly instead of going through the wire-format facade; serialization is centralized in a new sketchlib_runtime module.
• Wire byte shape is preserved (CountMinSketch / KLL DTOs unchanged on the wire).

Status

Draft — pinned to a branch in asap_sketchlib, not a tagged release. Holding as draft until the upstream branch lands on its target.

Verification

• cargo build clean
• cargo test -p query_engine_rust sketch → 41 passed, 0 failed
• cargo clippy -p query_engine_rust --lib clean apart from 2 pre-existing needless_range_loop style warnings in sketchlib_runtime.rs

Test plan

• CI green
• Re-pin asap_sketchlib to a merged commit on its target branch before marking ready
• (Optional) clear the two clippy style warnings in sketchlib_runtime.rs

🤖 Generated with Claude Code

[milindsrivastava1997]

cms_from_matrix doesn't validate that rows/cols match the matrix shape (sketchlib_runtime.rs:61-70)

rows/cols come from the JSON/binary envelope while matrix is parsed separately. If they mismatch (malformed payload), from_fn silently uses the envelope dimensions and pads/truncates with 0.0 — invisible corruption. A debug_assert! or early return error at the call sites in deserialize_from_json and deserialize_from_bytes would catch this.

[milindsrivastava1997]

cms_to_msgpack swallows serialization errors (sketchlib_runtime.rs:82-86)

rmp_serde::to_vec(&wire).unwrap_or_default() returns an empty Vec<u8> on failure. The caller serialize_to_bytes propagates that silently, so a downstream deserializer will fail with a confusing "buffer too short" error instead of pointing at the actual failure site. Change the return type to Result or use expect — silent empty bytes are worse than a panic.

[milindsrivastava1997]

Regression test context comments removed (count_min_sketch_accumulator.rs, test_update_and_query_use_same_key_encoding)

The removed multi-line comment explained a real past bug — key encoding mismatch between _update and query_key that caused silent lookup failures. That context is exactly what makes the test legible to a future reader wondering why this edge case is tested at all. Worth restoring.

[milindsrivastava1997]

@GordonYuanyc some review comments from Claude, pls check

[GordonYuanyc]

Thank you for the suggestion! Updated!

[milindsrivastava1997]

Code Review

Overview

This PR adapts asap-query-engine to the new asap_sketchlib module layout, where types that were previously accessed via deep paths (sketches::countminsketch::CountMinSketch, sketches::kll::KllSketch, etc.) are now re-exported at the crate root. A new sketchlib_runtime shim module is introduced to centralize wire-format encoding/decoding and to hold the concrete generic type aliases (RuntimeCountMin, RuntimeKll). The wire byte format is documented as unchanged. The PR is in draft, pinned to a non-merged upstream branch.

---

Bugs / Correctness

1. Silent failure in kll_to_msgpack vs. panic in cms_to_msgpack (inconsistency + latent bug)

sketchlib_runtime.rs:937

// cms_to_msgpack — correct:
rmp_serde::to_vec(&wire).expect("CountMinSketchWire msgpack serialization is infallible")

// kll_to_msgpack — wrong:
rmp_serde::to_vec(&wire).unwrap_or_default()

The CMS path correctly panics because a well-formed struct has no unrepresentable state. The same reasoning applies to KllSketchData. Using unwrap_or_default() for KLL silently produces empty bytes, which will be fed downstream and surface as a misleading "buffer too short" error at the reader — the exact failure mode the CMS comment warns against.

Also affects kll_sketch_bytes (line 927): if sk.serialize_to_bytes() fails, the JSON output becomes {"sketch": ""} — silently corrupted data.

Fix: Replace both with .expect("KllSketchData msgpack serialization is infallible").

---

2. _update on CountMinSketchAccumulator is now dead code

count_min_sketch_accumulator.rs:189

fn _update(&mut self, key: &KeyByLabelValues, value: f64) {
    cms_update(&mut self.inner, &key.to_semicolon_str(), value);
}

accumulator_factory.rs:112 now calls sketchlib_runtime::cms_update directly, bypassing _update. The method is private with no other callers visible in this diff. If confirmed dead, it should be removed.

---

3. cms_update zero/negative guard — behavioral change to verify

sketchlib_runtime.rs:801-804

pub fn cms_update(sk: &mut RuntimeCountMin, key: &str, value: f64) {
    if value <= 0.0 {
        return;
    }
    ...
}

The old CountMinSketch::update path did not have this explicit guard in the QE layer. If the upstream CountMin::insert_many accepts zero or negative values, this guard silently drops those updates and changes semantics. Needs a comment explaining the invariant, or a test verifying this was always the intended contract.

---

4. unsafe impl Send/Sync on DatasketchesKLLAccumulator may now be unnecessary

datasketches_kll_accumulator.rs:542-543

unsafe impl Send for DatasketchesKLLAccumulator {}
unsafe impl Sync for DatasketchesKLLAccumulator {}

The original unsafe impls were needed because KllSketch wrapped a C++ library. RuntimeKll = KLL<f64> is a pure-Rust type. If the compiler would auto-derive Send + Sync for it, these are unnecessary boilerplate (though harmless). Worth checking with static_assertions::assert_impl_all!(RuntimeKll: Send, Sync) — if that compiles, the unsafe impls can be dropped.

---

Code Quality

5. Abstraction boundary is incomplete — accumulator code still reaches into inner

The sketchlib_runtime shim is supposed to isolate the upstream sketch internals, but callers still access inner.k(), inner.count(), inner.rows(), inner.cols() directly across multiple accumulator files. Either:

• Complete the isolation: add kll_count(), kll_k(), cms_rows(), cms_cols() helpers to sketchlib_runtime, or
• Drop the shim abstraction and just use the new module-path imports directly.

The current half-measure means future upstream API changes still break N callsites across multiple files.

---

6. Error type inconsistency across sketchlib_runtime functions

• cms_merge_refs, kll_merge_refs → Box<dyn std::error::Error + Send + Sync>
• cms_from_matrix, cms_from_msgpack, kll_from_msgpack → Box<dyn std::error::Error>

Pick one convention for the whole module. The Send + Sync variant is safer for async contexts.

---

7. cms_matrix silent zero-fill on storage.get() returning None

sketchlib_runtime.rs:818-821

if let Some(v) = storage.get(r, c) {
    *cell = *v;
}
// else: leaves 0.0 silently

If storage.get() can return None for valid (r, c) within the declared dimensions (e.g. sparse storage), this is a silent correctness issue. If it cannot, unwrap() / expect() would be more appropriate.

---

Test Coverage

8. No unit tests for error paths in merge functions

kll_merge_refs checks for k mismatch and cms_merge_refs checks for dimension mismatch — both return errors. Neither error path has a test in this diff. These are the failure modes most likely to surface in prod; each deserves a dedicated test case.

9. Go-wire compatibility test pointer missing

The PR notes asap_sketchlib::tests::sketches_go_parity_probe covers cross-language byte parity upstream. A comment in sketchlib_runtime.rs pointing to it would help future readers understand why there's no explicit cross-language test in this crate.

---

Nits

• The two pre-existing needless_range_loop clippy warnings in cms_matrix are trivially fixable now since the function is new: iterate with .iter().enumerate() instead of index ranges.
• windows-sys downgrade (0.61.2 → 0.48.0) is an indirect dep change from the upstream bump — fine, but worth a sanity check if this ever targets Windows.

---

Summary

The migration is mechanically correct and wire format is preserved. Issues to fix before marking ready:

Priority	Issue
Bug	kll_to_msgpack / kll_sketch_bytes should panic, not silently return empty bytes
Bug	Dead _update method should be removed
Verify	cms_update zero/negative guard — confirm behavioral equivalence with old path
Verify	unsafe impl Send/Sync may be removable now that FFI is gone
Design	Decide: complete the sketchlib_runtime abstraction boundary, or flatten it
Tests	Add negative cases: KLL k-mismatch merge, CMS dimension-mismatch merge

[GordonYuanyc]

Closing this draft PR.

After rebasing onto current main, I don’t think this refactor is the right direction anymore. main already uses crates.io asap_sketchlib = "0.2.2", and the current accumulator code relies on the portable asap_sketchlib::CountMinSketch / KllSketch APIs.

Those wrapper types are important because they preserve the MessagePack/wire-format boundary used for Rust/Go sketch interoperability. This PR moves parts of that responsibility into ASAPQuery by using lower-level runtime sketch types, which is probably not a cleaner ownership boundary.

So I’m closing this draft and keeping the current main approach.