feat(hpc): universal builder for CLAM — stateful distances, two arms, one core - #277
Conversation
ClamTree konnte nur bare fn pointers (DistanceFn) tragen. Eine
konfigurierte Distanz — eine RailSpec, ein Codebook, eine aufgeloeste
ClassView-Lesart — passt in keinen fn pointer; der Wellen-Probe musste
sich mit einem const-Workaround behelfen.
Jetzt: privater Traeger TreeDistance { Ptr(DistanceFn) | Dyn { f, metric } }.
Beide Arme landen im EINEN Partition-Kern (build_core_nodes) und im
EINEN Dispatch (dist) — derselbe Name kann nie zwei Dinge bedeuten
(I-LEGACY-API-FEATURE-GATED).
build_with_fn / build / build_with_config unveraendert, Ptr-Arm
build_with_distance<D: Distance<Point=[u8]>> NEU, der stateful Arm
is_metric() wird GETRAGEN statt angenommen: der Ptr-Arm meldet true
(bewahrt die historische Pruning-Annahme, segnet sie nicht), der
Dyn-Arm traegt die Antwort der Distanz — der Haken, an dem
Dreiecksungleichungs-Pruning ueber eine Pseudometrik kuenftig
verweigert werden kann.
distance_fn() behaelt seine Signatur; auf dem stateful Arm panict es
LAUT mit benanntem Ausweg, statt einen Ersatz auszugeben, der etwas
anderes misst. Kein vorbestehender Konstruktionspfad erreicht das.
Der Compressed-Search-Pfad laeuft ueber einen Traeger-Borrow (beide
Arme); beide hamming_to_compressed-Varianten (es gab ZWEI — clam.rs
und clam_compress.rs) nehmen jetzt impl Fn, quell-kompatibel fuer
jeden bestehenden Aufrufer.
Der Transplantations-Beweis steht im Code: mein erster Umzug des
Build-Rumpfs hatte still den RNG-Seed getauscht (0xDEAD_BEEF_CAFE_BABE
gegen einen Golden-Ratio-Reflex), assert und Leer-Return vertauscht
und die Integer-Radiussumme in eine f64-Neusummierung verwandelt —
drei Aenderungen, die jedem bestehenden Aufrufer einen anderen Baum
gebaut haetten. Per Diff der eigenen Loeschung gefangen; die
Kommentare im Kern benennen alle drei, damit der naechste Umbau sie
nicht neu erfindet.
Gepinnt statt behauptet: Arm-Identitaet (build_with_fn vs
build_with_distance liefern den byte-identischen Baum UND identische
rho_nn-Treffer), eine echt zustandsbehaftete Distanz gegen ihren
hartkodierten fn-pointer-Zwilling, is_metric in beide Richtungen, die
Accessor-Verweigerung als #[should_panic] mit gepruefter Meldung.
clam 96/96 · gesamte Lib 2197/0 · clippy im Modul 0.
|
Warning Review limit reached
Next review available in: 102 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5f01fedd-857b-4dcb-9279-bf4be57c8ba0) |
CI-Gate cargo fmt --all --check war rot — reine Formatierung, kein semantisches Byte geaendert; clam 96/96 danach unveraendert gruen.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c6e74de15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let metric = dist.is_metric(); | ||
| let f: Box<dyn Fn(&[u8], &[u8]) -> u64 + Send + Sync> = Box::new(move |a, b| dist.distance(a, b)); | ||
| // Build against a borrow of the SAME closure that will be stored — | ||
| // one callable, never a rebuilt twin that could drift. | ||
| let nodes_input = &*f as &dyn Fn(&[u8], &[u8]) -> u64; | ||
| // SAFETY-free reborrow dance: build_core only borrows `dist` during | ||
| // construction; the box is moved into the carrier afterwards. | ||
| let tmp = Self::build_core_nodes(data, vec_len, count, config, nodes_input); | ||
| Self::assemble(tmp, TreeDistance::Dyn { f, metric }) |
There was a problem hiding this comment.
When a stateful Distance returns false from is_metric, this constructor still produces a tree that rho_nn, knn_repeated_rho, knn_dfs_sieve, and rho_nn_candidates prune using triangle-inequality radius bounds. Those bounds are invalid for such a distance, so relevant clusters can be skipped and searches can silently return false negatives; merely exposing the stored flag does not protect callers. Reject non-metric distances in the pruned routines or bypass the bounds and scan exhaustively when this flag is false.
Useful? React with 👍 / 👎.
| let dist_fn: &dyn Fn(&[u8], &[u8]) -> u64 = match &self.distance { | ||
| TreeDistance::Ptr(f) => f, | ||
| TreeDistance::Dyn { f, .. } => &**f, | ||
| }; |
There was a problem hiding this comment.
When query_compressed is called on the new dynamic arm with anything other than full-slice bitwise Hamming—such as the added TailHamming or V3RailGeodesic—this closure supplies the baseline distance, but hamming_from_query subsequently adjusts it using full-record XOR popcounts. That identity is valid only for ordinary Hamming distance: for example, a byte difference before TailHamming::from changes the reported result even though the configured distance ignores it, yielding incorrect distances and hit filtering. Restrict this optimized path to full Hamming or decode each candidate and evaluate the tree's actual distance.
Useful? React with 👍 / 👎.
The universal-builder pass:
ClamTreeaccepts stateful distances without touching a single existing caller. Two arms, one core.Why
A configured distance — a
RailSpec, a codebook, a resolved ClassView reading — does not fit in a bare fn pointer. The wave probe (MedCare-rs #478) had to smuggle itsRailSpecthrough aconstto ridebuild_with_fn. That workaround only works for compile-time-constant configuration; a ClassView resolved at runtime cannot ride at all.The shape
Private carrier, both arms through one partition core and one dispatch, so the same name can never mean two things (
I-LEGACY-API-FEATURE-GATED):is_metric()is carried, not assumed. ThePtrarm reportstrue— preserving, explicitly not blessing, the historical triangle-inequality-pruning assumption. TheDynarm carries the distance's own answer: the hook for refusingrho_nn-style pruning over a pseudometric that says so.distance_fn()keeps its signature; on the stateful arm it panics loudly with the fix named, rather than handing out a substitute that measures something else. No pre-existing construction path can reach it.hamming_to_compressedvariants — there were two, inclam.rsandclam_compress.rs— now takeimpl Fn, source-compatible for every existing caller.The transplant proof, left in the code
My first move of the build body silently swapped the RNG seed (
0xDEAD_BEEF_CAFE_BABEfor a golden-ratio reflex), reordered the assert against the empty return, and turned the integer radius sum into an f64 re-summation — three changes that would have rebuilt every existing caller's tree differently while every test name stayed green-sounding. Caught by diffing my own deletion before trusting it; the core's comments now name all three so the next refactor does not re-invent them.Pinned, not claimed
build_with_fnvsbuild_with_distanceon equal inputs produce the byte-identical tree (per-cluster fields + reordering) and identicalrho_nnhits.TailHamming { from }) matches its hardcoded fn-pointer twin.is_metriccarriage in both directions; the accessor's refusal as#[should_panic]with the message asserted.clam 96/96 · whole lib 2197 passed / 0 failed · clippy in the module: 0.
Downstream
MedCare-rs #478's probe can drop its
constworkaround in a follow-up:V3RailGeodesic(spec)straight intobuild_with_distance, spec resolved from the ClassView at runtime. This PR is additive; nothing waits on it, but the lance-graph rail-geometry registration and the medcare rail lane both stand on it.🤖 Generated with Claude Code