⏺ Stage B is built and green. #21
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fact-table-stage-B"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What landed
First-argument index — bound-key fact queries now binary-search instead of
full-scanning:
@plg_facts___idx — row indices sorted by (column-0 word, row), so
within a key they stay in program order. The entry passes the index pointer
(null for arity-0) to the lookup. One extra word per row.
bound atom/int (column 0's exact domain), equal_range binary-searches the
index for [lower, upper) and the scan iterates only that range, mapping
each position through the index. Unbound or non-immediate first args fall
back to the full scan — identical to Stage A. The fast path is only taken
where Word-equality coincides with what unify accepts, so the indexed set
is exactly the rows Stage A would have matched, in the same order.
#3 single-unify cleanup — added Machine::push_cp_at(retry, env, trail_mark,
heap_mark) (a choice point with an explicit pre-binding restore-point).
fact_scan now binds a matching row once: it pushes the CP with the captured
clean marks instead of unify → rewind → re-unify. Failed rows still rewind
and continue.
Verification
fact_table + family suites (bound-key queries like parent(tom,X),
parent(bob,ann) now route through the index) all pass.
tom at rows 0/3/4 → a,d,e, skipping interleaved rows; absent key fast-fails
via empty range); golden test asserts the _idx global is emitted.
fast-fails. Full workspace green, clippy + fmt clean.
Review — Fact-table Stage B (first-argument index)
Tight, focused PR that closes out two open items from prior reviews — the O(log n) bound-key lookup, and PR #20 review #3's double-unify-and-rewind cleanup via the new
push_cp_atprimitive. The correctness reasoning is the load-bearing piece, and it's right: the index is sorted by rawu64(Word) comparison, the binary search uses the same comparator, and the fast path is gated onTAG_ATOM | TAG_INT— the exact column-0 domain where Word equality coincides with whatunifyaccepts. So the indexed set equals what Stage A would have matched, in the same order. The comment nearequal_rangemakes this explicit, which is the right place for it.Other strong calls:
push_cp_atis the right abstraction, not a fact-table-private helper. Any future nondeterministic builtin that wants to bind-then-record-restore-point can reuse it. The single-unify cleanup is the immediate beneficiary; that's a reusable primitive earned by doing it.sort_by_key(|&r| (col0, r))— solution order = program order is preserved through the index, which is the whole point.indexed_lookup_gathers_nonadjacent_rows_in_program_orderpins it.(lower, lower), the loop seespos >= endimmediately, returns 0. Tested viarel(zzz, X).Issues below, ordered by impact — none are blockers.
Real concerns
1.
push_cp_at's contract is debug-only.debug_assert!(trail_mark <= self.trail.len() && heap_mark <= self.heap.len())only fires in debug builds. In release, an out-of-range mark passed by a future caller would silently no-op the truncate at backtrack time (Vec::truncate(N)whereN > len()is a no-op), which would manifest as state-leak bugs — bindings from an "undone" alternative surviving into the next. The single in-tree caller (fact_scan) is correct, but the primitive is now publicly visible (pub fn) and the next caller has to know this. Two options:let trail_mark = trail_mark.min(self.trail.len());— small overhead, defensive.Leaning toward the doc note since the clamp would hide bugs rather than expose them. But pick one — a
pubAPI with a debug-only contract is the failure mode that bites later.2. Stage B's perf claim is structural, not measured. FACT_TABLE.md now reads "Stage B (2026-06-18): a first-arg index ... gives bound-key queries an O(log n) binary search to the matching row range." Structurally true by construction. But Stage A's note in the same doc cited an actual measurement ("100k 2-column facts ... compile in ~1.0s total ... 4.5M binary ... a worst-case bound-key query over 100k rows ~90ms"). The natural close-the-loop for Stage B is "and the same 100k query now takes <Xms," which would be a one-line
criterionbench or even just atests/perf/one-off the dev runs. It's the same measurement gap PR #20 review #1 flagged — closer to closed now, since the structural path is in place, but still not measured. Worth either landing the number or being explicit that the Stage B perf claim is structural-not-measured.Small observations
3. Float column lookups will be a future Stage C concern. Today Stage A's
fact_columnsrejects float first args (column 0 is atom/int only), so the indexed path is correct for what's in the table. If Stage C adds float columns to fact tables, the binary search will need to be revisited: NaN doesn't have a total order, and float equality has its own gotchas (-0.0 == 0.0but their bit patterns differ; quiet NaN vs signaling NaN). A note in FACT_TABLE.md's Stage C section saying "float columns will need a non-u64comparator or a normalized bit representation in the index" would head off a "let's just use the same equal_range" reflex.4. The "IDX=0 means no index" sentinel. Works because no real
.rodataglobal lives at address 0 (page zero is unmapped). Standard C-FFI convention. Worth a one-line comment at the IDX constant declaration confirming the sentinel choice — a reader who isn't FFI-fluent might wonder why an array pointer ever equals zero.5. The frame-layout comments could cross-reference the cursor→row mapping. The IDX constant says "first-arg index pointer, or 0 for a full scan"; CURSOR says "next position to try — mutated in place, untrailed"; END says "exclusive upper bound of the cursor's range." The actual semantics — that
cursoris a position in[cursor_0, end)that maps to a row throughidx[cursor]when set, else IS the row — only appears infact_scan's body. A one-line "(see fact_scan for the cursor→row mapping)" at IDX/CURSOR would save the reader a hop.6.
equal_rangeis two explicit binary searches. Could be replaced byslice::partition_pointfor slightly more idiomatic Rust:But the explicit form is more transparent and matches the comment ("lower bound: first k with col0(k) >= key"). Keep it.
7. The new fact_scan loop bumps CURSOR unconditionally after the unify try. Old Stage A bumped CURSOR only on the matched branch; on failure, the bump happened implicitly via the next iteration's
i += 1. New shape: one write, both branches. Cleaner; doesn't change semantics. Just noting that the write-survives-backtrack invariant (CURSOR is untrailed, frame cells survive heap truncation) is the same as before — worth confirming with a one-line "the CURSOR write is intentionally untrailed; it advances monotonically across the CP's resume" at the bump site if a reader has to verify it.What's good
push_cp_atis reusable, not fact-table-private. The single-unify cleanup is the first beneficiary; future nondeterministic builtins (Stage C compound-column unify, or a future indexedbetween/3-style) can reuse the primitive without reinventing it.tomat rows 0/3/4 → solutionsa, d, ein that order), and absent-key fast-fail (binary search empty range). Plus the golden test asserts the_idxglobal is emitted.parent(tom, X)) now exercises the indexed path automatically without any test churn — the equivalence pin from Stage A still byte-matches the oracle through the new code.Suggested order
#1 (
push_cp_atcontract — pick docs or clamp) and #2 (measure the Stage B perf claim, or downgrade the FACT_TABLE.md wording from "gives O(log n)" to "structurally O(log n)") are worth doing before merge. #3 (float Stage C note in FACT_TABLE.md) is one line and saves future-you a reflex. #4+ are observations.