⏺ All Stage C column kinds work correctly — compound (enumerate + partial #22

Merged

navicore merged 3 commits from fact-table-stage-C into main 2026-06-18 19:33:06 +00:00

Conversation 1 Commits 3 Files changed 11 +559 -268

navicore commented 2026-06-18 18:51:23 +00:00

Owner

Copy link

bind), list, float (both directions), big-int, mixed immediate-col0 +
compound-col1, and compound col-0 with binary-search disabled (full scan +
restore + unify, including partial binding rel(point(N,4),Y) → N=3). Now
the detection widened to ground (not just immediate), so compound-column
facts that were per-clause now route through the table — let me run the
full suite to see exactly what that shifts:

bind), list, float (both directions), big-int, mixed immediate-col0 + compound-col1, and compound col-0 with binary-search disabled (full scan + restore + unify, including partial binding rel(point(N,4),Y) → N=3). Now the detection widened to ground (not just immediate), so compound-column facts that were per-clause now route through the table — let me run the full suite to see exactly what that shifts:

navicore added 1 commit 2026-06-18 18:51:23 +00:00

⏺ All Stage C column kinds work correctly — compound (enumerate + partial ... e2069a092f

bind), list, float (both directions), big-int, mixed immediate-col0 +
  compound-col1, and compound col-0 with binary-search disabled (full scan +
  restore + unify, including partial binding rel(point(N,4),Y) → N=3). Now
  the detection widened to ground (not just immediate), so compound-column
  facts that were per-clause now route through the table — let me run the
  full suite to see exactly what that shifts:

navicore added 1 commit 2026-06-18 19:06:04 +00:00

⏺ Stage C is built and green. ... 323c8d3d5c

What landed

  Detection widened from "immediate columns" to all-ground head args — any
  predicate whose clauses are all bodyless facts with no variables (however
  deeply nested) becomes a fact table. A body clause or a variable arg →
  per-clause, unchanged.

  Non-immediate columns (compound, list, float, big-int) are serialized at
  compile time into a per-predicate .rodata blob in the copyterm/TermBuf cell
  format. Each table cell stays one word: atom/int stored inline (Stage A/B
  fast path + index intact), else a STR/LST/FLT/BIG word whose payload
  indexes the blob. At lookup, fact_scan restores such a cell onto the heap
  via the new copyterm::restore_cells (a slice-based core factored out of
  restore_from_buf) before unifying — the cell tag distinguishes the two
  cases with no extra metadata.

  - Codegen: an iterative work-queue serializer (deep ground terms won't
  overflow the compiler stack), @..._blob emitted only when needed; the
  first-arg index is emitted only when column 0 is all-immediate (a compound
  column 0 isn't u64-sortable → full scan).
  - Runtime: blob ptr/len threaded through plg_rt_fact_first into the frame;
  restored cells live above the choice point's clean marks, so backtracking
  discards them — the single-unify / musttail-delivery discipline is
  unchanged.

  Verification (the coverage matrix, built)

  - Behavioral (fact_table.rs, 15 tests): compound enumerate + partial bind,
  list, float (both directions + ground), big-int, mixed
  indexed-immediate-col0 + restored-compound-col1, unindexed compound col-0
  (bound + partial key) — alongside the existing
  program-order/ground/index/findall/call/recursive/--limit/existence-error
  cases.
  - Structural (golden_ir.rs, 14): a fact predicate emits table (+ index when
  col-0 immediate, + blob when non-immediate columns) and no per-clause
  put_big; the per-clause path is still covered by tests repointed to rules /
  variable-arg facts (head unify, choice-point chains, first-arg switch,
  runtime big-int boxing) — that path is live for every rule, so it stays
  tested.
  - Equivalence: differential corpus (compound facts included) byte-identical
  to the v1 oracle; full workspace green, clippy + fmt clean; 2000-deep
  recursion through a fact table still constant-stack.

  FACT_TABLE.md marked implemented (Stages A–C); the float-keyed-indexing
  caveat and the unsafe seam remain the recorded deferrals. The whole
  fact-table effort (A–C) is complete.

navicore commented 2026-06-18 19:15:10 +00:00

Author

Owner

Copy link

Review — Fact-table Stage C (ground compound/list/float/big-int columns)

The architecture is right and reuses the existing infrastructure well. Three load-bearing calls:

1. Detection widened from "immediate" to "ground" head args. The right axis — a variable disqualifies; everything else qualifies. Clean criterion, well documented.
2. Cell-tag dispatch at scan time, no per-column metadata. A table cell IS the discriminator: tag_of(col) in {TAG_ATOM, TAG_INT} → unify inline; else → it's a blob-ref that restore_cells materializes onto the heap, then unify. The cell tag answers "immediate or blob?" with no extra word per column.
3. restore_cells extracted from restore_from_buf, so the same materialization code that handles TermBuf (dynamic, used by error preservation through backtracking) handles the read-only .rodata blob. Two consumers, one walker. The doc note that "those blobs are ground, so the TAG_REF arm never fires there" pins the invariant precisely.

Other strong calls:

• Iterative serialize_arg with a work queue (rather than recursion) means deep ground terms — long lists, nested compounds — can't overflow the compiler stack.
• Per-predicate blob, sized to content. No blob global at all when every column is immediate (pinned by the golden test's !ir.contains("_blob =")). Pay-for-what-you-use carries through.
• Mixed column kinds work: m(1, point(2, 3)) indexes on the immediate col 0 and restores the compound col 1 — the index gate is (0..nrows).all(|r| ATOM|INT in col 0) per predicate, not all-or-nothing on the whole table.
• Compound col 0 falls back to full scan rather than producing a wrong index. The rel(point(N, 4), Y) → N=3, Y=bb test pins that the fallback path correctly handles partial binding through the restored compound.
• Float round-trip is bit-identical via f64::to_bits() ↔ blob ↔ TAG_FLT heap cell. The pii(p, 3.14) test asserts a ground float query unifies (which is bit-equality, not numeric equality — the only sane choice).
• Big-int beyond i61 serializes to a BIG blob cell, not a runtime-built box. The golden test pins the absence of call i64 @plg_rt_put_big in the fact-table path — confirms that the big-int genuinely lives in .rodata rather than being boxed per-row at startup.

Issues below — none are blockers.

Real concerns

1. Tag constants are now duplicated across compiler and runtime without a compile-time agreement check. crates/compiler/src/codegen/facts.rs hardcodes:

const TAG_ATOM: u64 = 1;
const TAG_INT: u64 = 2;
const TAG_STR: u64 = 3;
const TAG_LST: u64 = 4;
const TAG_FLT: u64 = 5;
const TAG_BIG: u64 = 6;

fn mk(tag: u64, payload: u64) -> u64 { (payload << 3) | tag }
fn pack_functor(functor: u32, arity: u32) -> u64 { ((functor as u64) << 32) | arity as u64 }

These mirror plg-runtime/src/cell.rs. If the runtime's tag values, tag width (the & 7 mask assumes 3 bits), shift count (<< 3), or functor-packing layout ever changes one-sidedly, the codegen emits garbage into the .rodata blob and the runtime decodes it as something else — silently. The PR description calls this "the ABI contract for the serialized blob," but there's no enforcement.

Three options, increasingly principled:

• Cheapest: a unit test in either crate that imports the other and asserts TAG_ATOM_compiler == TAG_ATOM_runtime (and friends). One-shot drift detector.
• Better: lift the tag constants and mk/pack_functor helpers into plg-shared (or a tiny new plg-cell crate) and use them in both places. One source of truth.
• Best: add a static_assertions::const_assert_eq! at the compiler side that references the runtime's pub constant directly. Compile-time check, no runtime cost.

This is the same class of ABI-drift exposure as PR #15's NO_SITE constants (two crates, same value, hopefully). That one was small enough for a doc note; this one is six tag values plus two packing helpers, with the surface growing if Stage D/E adds more cell kinds. Lifting to plg-shared would close the class.

2. Compound col 0 missed-indexing opportunity, not a bug. Today: a compound col 0 cell stores a blob-ref STR word whose payload is the blob offset for that serialization. Two facts with identical compound col 0 (e.g. two clauses with point(1, 2) as col 0) serialize to different blob offsets, so their u64 values differ — even though they're semantically equal. The PR correctly avoids indexing on this. A future optimization (interning ground compounds in the blob — already-seen subterms reuse offsets) would make col-0 sort by blob-offset semantically meaningful and re-enable the index. Worth a one-line "interned blob would allow Stage D to re-index compound col 0" note in FACT_TABLE.md. Not in scope for Stage C.

Small observations

3. NaN in float columns is the same gotcha I flagged for Stage B's future float-indexing case. Today's design tests bit-equality (the to_bits()/restore path), so nan(value) with the same NaN bit pattern would round-trip; two NaNs with different bit patterns wouldn't unify even though both are NaN. This matches ISO Prolog semantics for == (term-identity), so the behavior is correct. Worth a one-line "ground-fact float columns compare by IEEE-754 bit pattern; different NaN encodings do not unify" in FACT_TABLE.md to head off "wait, why did nan not match my fact?" confusion.

4. No test for a deep ground term. The iterative serialize_arg was explicitly designed to avoid compiler-stack overflow on deep structures (the comment says so). A data(deep_nested_list_of_1000_atoms) test would confirm the iteration actually works at depth where recursion would have blown. Cheap to add; pins the pattern's value.

5. The frame slot count keeps growing. Stage A: 8 prefix slots. Stage B: 10 (added IDX, END). Stage C: 12 (added BLOB, BLOBLEN). Each non-immediate fact-table predicate's frame is 12+arity cells per invocation, and each path/2-style recursion lives one frame deep. Modest cost (cells live on the machine heap, not the C stack), but worth noting that the frame is now ~1.5× Stage A. If Stage D/E adds more, a struct-shaped frame layout (rather than positional constants) would scale better. Stylistic only.

6. has_blob is computed by !blob.is_empty(). Correct, but a predicate whose only non-immediate column happens to be the empty list [] actually doesn't need a blob (the empty list is an atom [], an immediate). So today the check matches "blob has content," which is the right semantics. Just confirming the flag's name reads as intended — "did serialization actually produce something" rather than "predicate has non-immediate columns."

7. Iterative work queue with LIFO ordering. work.push((a, dst)) followed by work.pop() processes in reverse argument order. Dst slots are captured at enqueue, so write order doesn't affect correctness. Worth one line of comment ("processing order is LIFO but each dst index is captured at enqueue, so write order doesn't matter") because a future contributor optimizing this might be tempted to switch to FIFO and wonder why.

8. Golden IR test renames are good housekeeping. fact_compiles_to_unify_and_continuation_jump → per_clause_fact_compiles_to_unify_and_continuation_jump makes the path it covers explicit. Same for multi_clause_predicate_pushes_choice_points. Future readers know without re-reading which path each test pins.

What's good

• The reuse of copyterm's walker is the load-bearing move. The fact-table doesn't reinvent term restoration; it borrows the existing one. One walker means one place to fix bugs and one place that needs to evolve when the cell format does.
• Cell-tag-as-discriminator at scan time is elegant. The runtime needs no extra per-column metadata to distinguish "immediate" from "blob-ref" — the cell itself answers via its tag bits. This keeps the table dense (one word per column, always).
• The detection widening is honest about what changes: PR #20's compound_column_predicate_stays_per_clause test was correctly removed (the predicate now takes the table path), and the new fact_table.rs preamble names exactly what the test coverage now spans.
• Each new column kind has its own integration test (compound, list, float, big-int, mixed, compound-col-0 with partial binding). Plus the golden tests pin the IR shape (blob present/absent, index absent for compound col 0). Strong coverage discipline — every new column kind ships with a behavior pin AND an IR-shape pin.
• is_ground walks both compounds and lists recursively. Nested vars (e.g. f(g(X))) correctly disqualify. The Term::List { head, tail } branch recurses into both — [1, X, 3] is not ground.
• The codegen mirrors the runtime's blob format explicitly (functor-arity-args layout, LST as two slots). The runtime's restore_cells doc note ("ground blobs, so TAG_REF never fires") makes the invariant the codegen depends on explicit.
• FACT_TABLE.md status update is honest — names what's done (Stages A–C), what's deferred (multi-arg indexing, float-keyed indexing, the unsafe seam), and why. Future contributors don't have to reconstruct the scope from the diff.

Suggested order

#1 (ABI agreement check between the two crates' tag constants) is the only thing worth landing before merge — it's the bug class that bites silently and grows with Stage D/E. A unit test importing both is sufficient; lifting to plg-shared is the principled fix. #2 (compound-col-0 interning note in FACT_TABLE.md) is one line. #3+ are observations.

## Review — Fact-table Stage C (ground compound/list/float/big-int columns) The architecture is right and reuses the existing infrastructure well. Three load-bearing calls: 1. **Detection widened from "immediate" to "ground" head args.** The right axis — a variable disqualifies; everything else qualifies. Clean criterion, well documented. 2. **Cell-tag dispatch at scan time, no per-column metadata.** A table cell IS the discriminator: `tag_of(col) in {TAG_ATOM, TAG_INT}` → unify inline; else → it's a blob-ref that `restore_cells` materializes onto the heap, then unify. The cell tag answers "immediate or blob?" with no extra word per column. 3. **`restore_cells` extracted from `restore_from_buf`**, so the same materialization code that handles `TermBuf` (dynamic, used by error preservation through backtracking) handles the read-only `.rodata` blob. Two consumers, one walker. The doc note that "those blobs are ground, so the `TAG_REF` arm never fires there" pins the invariant precisely. Other strong calls: - **Iterative `serialize_arg`** with a work queue (rather than recursion) means deep ground terms — long lists, nested compounds — can't overflow the compiler stack. - **Per-predicate blob, sized to content.** No blob global at all when every column is immediate (pinned by the golden test's `!ir.contains("_blob =")`). Pay-for-what-you-use carries through. - **Mixed column kinds** work: `m(1, point(2, 3))` indexes on the immediate col 0 *and* restores the compound col 1 — the index gate is `(0..nrows).all(|r| ATOM|INT in col 0)` per predicate, not all-or-nothing on the whole table. - **Compound col 0 falls back to full scan** rather than producing a wrong index. The `rel(point(N, 4), Y) → N=3, Y=bb` test pins that the fallback path correctly handles partial binding through the restored compound. - **Float round-trip is bit-identical** via `f64::to_bits()` ↔ blob ↔ TAG_FLT heap cell. The `pii(p, 3.14)` test asserts a ground float query unifies (which is bit-equality, not numeric equality — the only sane choice). - **Big-int beyond `i61` serializes to a BIG blob cell**, not a runtime-built box. The golden test pins the absence of `call i64 @plg_rt_put_big` in the fact-table path — confirms that the big-int genuinely lives in `.rodata` rather than being boxed per-row at startup. Issues below — none are blockers. ### Real concerns **1. Tag constants are now duplicated across compiler and runtime without a compile-time agreement check.** `crates/compiler/src/codegen/facts.rs` hardcodes: ```rust const TAG_ATOM: u64 = 1; const TAG_INT: u64 = 2; const TAG_STR: u64 = 3; const TAG_LST: u64 = 4; const TAG_FLT: u64 = 5; const TAG_BIG: u64 = 6; fn mk(tag: u64, payload: u64) -> u64 { (payload << 3) | tag } fn pack_functor(functor: u32, arity: u32) -> u64 { ((functor as u64) << 32) | arity as u64 } ``` These mirror `plg-runtime/src/cell.rs`. If the runtime's tag values, tag width (the `& 7` mask assumes 3 bits), shift count (`<< 3`), or functor-packing layout ever changes one-sidedly, the codegen emits garbage into the `.rodata` blob and the runtime decodes it as something else — silently. The PR description calls this "the ABI contract for the serialized blob," but there's no enforcement. Three options, increasingly principled: - **Cheapest:** a unit test in either crate that imports the other and asserts `TAG_ATOM_compiler == TAG_ATOM_runtime` (and friends). One-shot drift detector. - **Better:** lift the tag constants and `mk`/`pack_functor` helpers into `plg-shared` (or a tiny new `plg-cell` crate) and `use` them in both places. One source of truth. - **Best:** add a `static_assertions::const_assert_eq!` at the compiler side that references the runtime's pub constant directly. Compile-time check, no runtime cost. This is the same class of ABI-drift exposure as PR #15's `NO_SITE` constants (two crates, same value, hopefully). That one was small enough for a doc note; this one is six tag values plus two packing helpers, with the surface growing if Stage D/E adds more cell kinds. Lifting to `plg-shared` would close the class. **2. Compound col 0 missed-indexing opportunity, not a bug.** Today: a compound col 0 cell stores a blob-ref `STR` word whose payload is the blob offset for *that* serialization. Two facts with identical compound col 0 (e.g. two clauses with `point(1, 2)` as col 0) serialize to *different* blob offsets, so their u64 values differ — even though they're semantically equal. The PR correctly avoids indexing on this. A future optimization (interning ground compounds in the blob — already-seen subterms reuse offsets) would make col-0 sort by blob-offset semantically meaningful and re-enable the index. Worth a one-line "interned blob would allow Stage D to re-index compound col 0" note in FACT_TABLE.md. Not in scope for Stage C. ### Small observations **3. NaN in float columns is the same gotcha I flagged for Stage B's future float-indexing case.** Today's design tests bit-equality (the `to_bits()`/restore path), so `nan(value)` with the same NaN bit pattern would round-trip; two NaNs with *different* bit patterns wouldn't unify even though both are NaN. This matches ISO Prolog semantics for `==` (term-identity), so the behavior is correct. Worth a one-line "ground-fact float columns compare by IEEE-754 bit pattern; different NaN encodings do not unify" in FACT_TABLE.md to head off "wait, why did `nan` not match my fact?" confusion. **4. No test for a deep ground term.** The iterative `serialize_arg` was explicitly designed to avoid compiler-stack overflow on deep structures (the comment says so). A `data(deep_nested_list_of_1000_atoms)` test would confirm the iteration actually works at depth where recursion would have blown. Cheap to add; pins the pattern's value. **5. The frame slot count keeps growing.** Stage A: 8 prefix slots. Stage B: 10 (added IDX, END). Stage C: 12 (added BLOB, BLOBLEN). Each non-immediate fact-table predicate's frame is 12+arity cells per invocation, and each `path/2`-style recursion lives one frame deep. Modest cost (cells live on the machine heap, not the C stack), but worth noting that the frame is now ~1.5× Stage A. If Stage D/E adds more, a struct-shaped frame layout (rather than positional constants) would scale better. Stylistic only. **6. `has_blob` is computed by `!blob.is_empty()`.** Correct, but a predicate whose only non-immediate column happens to be the empty list `[]` actually doesn't need a blob (the empty list is an atom `[]`, an immediate). So today the check matches "blob has content," which is the right semantics. Just confirming the flag's name reads as intended — "did serialization actually produce something" rather than "predicate has non-immediate columns." **7. Iterative work queue with LIFO ordering.** `work.push((a, dst))` followed by `work.pop()` processes in reverse argument order. Dst slots are captured at enqueue, so write order doesn't affect correctness. Worth one line of comment ("processing order is LIFO but each dst index is captured at enqueue, so write order doesn't matter") because a future contributor optimizing this might be tempted to switch to FIFO and wonder why. **8. Golden IR test renames are good housekeeping.** `fact_compiles_to_unify_and_continuation_jump` → `per_clause_fact_compiles_to_unify_and_continuation_jump` makes the path it covers explicit. Same for `multi_clause_predicate_pushes_choice_points`. Future readers know without re-reading which path each test pins. ### What's good - **The reuse of `copyterm`'s walker** is the load-bearing move. The fact-table doesn't reinvent term restoration; it borrows the existing one. One walker means one place to fix bugs and one place that needs to evolve when the cell format does. - **Cell-tag-as-discriminator at scan time** is elegant. The runtime needs no extra per-column metadata to distinguish "immediate" from "blob-ref" — the cell itself answers via its tag bits. This keeps the table dense (one word per column, always). - **The detection widening** is honest about what changes: PR #20's compound_column_predicate_stays_per_clause test was correctly removed (the predicate now takes the table path), and the new fact_table.rs preamble names exactly what the test coverage now spans. - **Each new column kind has its own integration test** (compound, list, float, big-int, mixed, compound-col-0 with partial binding). Plus the golden tests pin the IR shape (blob present/absent, index absent for compound col 0). Strong coverage discipline — every new column kind ships with a behavior pin AND an IR-shape pin. - **`is_ground` walks both compounds and lists recursively.** Nested vars (e.g. `f(g(X))`) correctly disqualify. The `Term::List { head, tail }` branch recurses into both — `[1, X, 3]` is not ground. - **The codegen mirrors the runtime's blob format** explicitly (functor-arity-args layout, LST as two slots). The runtime's `restore_cells` doc note ("ground blobs, so TAG_REF never fires") makes the invariant the codegen depends on explicit. - **FACT_TABLE.md status update is honest** — names what's done (Stages A–C), what's deferred (multi-arg indexing, float-keyed indexing, the `unsafe` seam), and why. Future contributors don't have to reconstruct the scope from the diff. ### Suggested order #1 (ABI agreement check between the two crates' tag constants) is the only thing worth landing before merge — it's the bug class that bites silently and grows with Stage D/E. A unit test importing both is sufficient; lifting to `plg-shared` is the principled fix. #2 (compound-col-0 interning note in FACT_TABLE.md) is one line. #3+ are observations.

navicore referenced this pull request from a commit 2026-06-18 19:30:38 +00:00

⏺ Done — all gates green (fmt clean, full workspace 24 ok, clippy clean), and

navicore added 1 commit 2026-06-18 19:30:38 +00:00

⏺ Done — all gates green (fmt clean, full workspace 24 ok, clippy clean), and ... fb6b98ff0a

the plg-shared::cell tests now run there.

  Review disposition (PR #22)

  Before-merge:
  - #1 (ABI drift — duplicated tag constants) — took the principled fix:
  lifted the entire word/cell encoding (tag constants, make/tag_of/payload,
  pack_functor, make_atom/make_int, INT_MIN/MAX) into plg-shared::cell as the
  single source of truth. The runtime's cell.rs is now pub use
  plg_shared::cell::*; (so crate::cell::* is unchanged everywhere), and both
  codegen sites (facts.rs and term_emit.rs) use the shared definitions — no
  more hand-copied TAG_*/mk/pack_functor. The class is closed: there's
  nowhere left for codegen and runtime to drift one-sidedly, and the encoding
  unit tests live in plg-shared.

  Small items (done):
  - #4 — added deep_ground_term_serializes_iteratively: a 2000-element ground
  list round-trips (serialize → .rodata → restore → unify), pinning that the
  iterative serializer doesn't overflow at depth.
  - #7 — comment in serialize_arg noting the LIFO work queue is
  order-independent (each dst captured at enqueue).
  - #2 — FACT_TABLE.md note that interning ground subterms would let a later
  stage re-index compound column 0.
  - #3 — FACT_TABLE.md note that float columns compare by IEEE-754 bit
  pattern (distinct NaN encodings don't unify).

  No change (reviewer marked stylistic/confirmation): #5 (positional frame
  layout — fine for now), #6 (has_blob name reads as intended), #8 (praise on
  the test renames).

  Equivalence held throughout — differential corpus (compound facts included)
  still byte-identical to the v1 oracle.

navicore merged commit 4efa61b928 into main 2026-06-18 19:33:06 +00:00

navicore deleted branch fact-table-stage-C 2026-06-18 19:33:07 +00:00

navicore referenced this pull request from a commit 2026-06-18 19:33:07 +00:00

Merge pull request '⏺ All Stage C column kinds work correctly — compound (enumerate + partial' (#22) from fact-table-stage-C into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-prolog!22

No description provided.