Skip to content

Add node-based catamorphism POC - #31

Open
Adam-Vandervorst wants to merge 15 commits into
masterfrom
osplit-valcount
Open

Add node-based catamorphism POC#31
Adam-Vandervorst wants to merge 15 commits into
masterfrom
osplit-valcount

Conversation

@Adam-Vandervorst

Copy link
Copy Markdown
Owner

I purpose we use this paradigm under the cata interface.

@Adam-Vandervorst

Copy link
Copy Markdown
Owner Author

@luketpeterson any problem merging this?

@luketpeterson

Copy link
Copy Markdown
Collaborator

@luketpeterson any problem merging this?

In concept no. There is a (small) bit of work to make the API is consistent with the cata that's there already.

@adamv-symbolica

Copy link
Copy Markdown

Giving Fable some time with it:

Blocking

B1 — goat_val_count double-counts the root value

trie_map.rs:509–521. PathMap::recursive_cata already ends with
collapse_f(self.root_val(), Some(w), &[]), so the closure counts the root value; the caller
then adds root_val again.

let mut map = PathMap::new();
map.insert(b"", ());
map.insert(b"a", ());
assert_eq!(map.val_count(), 2);      // ok
assert_eq!(map.goat_val_count(), 2); // FAILS: returns 3

Fix: drop the + root_val (and the now-pointless match self.root()), or make the collapse
closure ignore the final root-val invocation.

B2 — LineListNode "Case 10" (Val, Child with different first bytes) violates the branch contract

line_list_node.rs:2952–2977. The PR's own tests establish the contract:
at a branch, branch_f is called once per branch in ascending mask-bit order with the branch
mask, so the algebra can attribute each W to its byte (mask.indexed_bit(acc.idx)). Case 10
breaks it twice:

  • the first branch_f call passes &ByteMask::new() (empty) instead of &mask
    (line 2960 — contrast with Cases 4/5/8 which pass &mask on every call);
  • it processes the child slot first, but slots are stored sorted by first byte, so for
    (Val@a, Child@b) the calls arrive in descending byte order.
// keys {"a", "b1", "b2"} -> root pair node (Val@'a', Child@'b')
// Reconstructing paths via the documented mask contract yields:
//   [[98], [238,49], [238,50]]   (0xEE = sentinel for "mask had no bit at idx")
// instead of [[97], [98,49], [98,50]]

The value gets attributed to byte b, and the child subtrie to an empty mask. Any
mask-sensitive algebra (including the PR's own recursive_cata_jumping_total_len bench closure,
which would panic on .unwrap()) is wrong or crashes on tries that contain this node shape.
Fix: pass &mask on both calls and emit the value (lower byte) before the child.

B3 — Zipper recursive_cata silently returns "empty" for a mid-node focus

morphisms.rs:513–521. The blanket impl uses get_focus().0.borrow(), which
returns Some only for the BorrowedRc/OwnedRc variants of AbstractNodeRef. A zipper
focused part-way into a node (BorrowedTiny/BorrowedDyn — exactly what TinyRefNode exists
for) gets None and is treated as an empty trie:

let mut map = PathMap::new();
map.insert(b"abc1", ());
map.insert(b"abc2", ());
let mut rz = map.read_zipper();
rz.descend_to(b"ab");                       // path exists
rz.recursive_cata::<..>(count_vals ...)     // returns 0, expected 2

Fix: handle the remaining variants — e.g. go through as_tagged()/try_as_tagged() and add a
TaggedNodeRef-level entry point, or fall back to into_option() (accepting the clone), or
panic loudly rather than returning a wrong answer.

B4 — Branch-byte convention differs between node types, so results depend on physical layout

DenseByteNode includes the branch byte in the collapse_f prefix
(dense_byte_node.rs:415, core::slice::from_ref(&key_byte)) and
represents it in the mask given to branch_f. LineListNode strips the byte from the prefix
(&key0[1..]) and represents it only in the mask. An algebra therefore cannot know whether
prefix[0] is the branch byte or a distinct following byte:

// Path-reconstruction algebra (byte taken from mask, prefix appended):
//   correct on LineListNode pair shapes,
//   duplicates the first byte on DenseByteNode: [1,7,13] -> [1,1,7,13]

The same logical trie yields different W depending on which physical nodes back it. The PR's
own sum-digits tests only pass because the algebra threads a bool flag ("value was at empty
prefix") through W to compensate — a workaround that no external user will discover from the
docs. This is the "API consistency with the existing cata" work the reviewer already flagged on
the PR. Fix: pick one convention (LLN's byte-in-mask-only matches the existing
into_cata_jumping_* sub_path semantics best) and align ByteNode::node_recursive_cata;
then document it on Summarization.

B5 — Unbounded recursion: stack overflow on deep tries; PR's own test aborts the all_dense_nodes suite

recursive_cata_cached recurses once per physical node. The PR's
recursive_cata_stack_overflow_smoke (morphisms.rs:2232) documents overflow
between 8–10 KB of path depth on default features — and under --features all_dense_nodes
(1 byte per node) the very same test overflows and SIGABRTs the whole test binary at
PATH_LEN = 8_000. As a public API this is a panic-free-abort footgun on adversarial/deep data,
and as merged it leaves a red test config. Minimum: gate or shrink the smoke test per feature
and document the depth limit prominently; proper fix: explicit work-stack or segmented stacks
(e.g. stacker::maybe_grow) in recursive_cata_cached.

Non-blocking cleanups

  • Dead code: node_goat_val_count (trait method + 6 impls) lost its only consumer when
    traverse_physical was removed; either delete the chain or keep goat_val_count on it for
    the comparison's sake — not both.
  • Commented-out blocks left in line_list_node.rs (old generic implementation, old
    node_goat_val_count) and dense_byte_node.rs.
  • unreachable_unchecked on header patterns (line_list_node.rs:2980):
    header values 1–7 (slot1 used, slot0 free) are assumed impossible; if that invariant is ever
    violated this is UB rather than a panic. A debug_assert!/unreachable! in debug builds
    would be cheap insurance.
  • Option<Acc> dance in ByteNode::node_recursive_cata (Some(Acc::default()) +
    unwrap_unchecked) — ws is always Some; a plain Acc binding works.
  • pub(crate) values on ByteNode is only applied to the non-nightly field variant and no
    code outside the module reads it — revert the visibility change.
  • Docs: Summarization docs still carry GOAT/dev-branch placeholders;
    recursive_cata_stepping links to [Catamorphism::recursive_cata] (wrong trait); the
    COMPUTE_PATH=false caveat ("no reliable child_masks") deserves a loud, user-facing warning
    since it silently changes what the closures receive.
  • Bench honesty: recursive_cata_jumping_total_len asserts only the count, not the length —
    the two implementations being compared do not agree on total length today (a consequence of B4).
  • slim_dispatch's TaggedNodeRef::node_val_count now takes the gxhash map while the node impls
    take std — moot while the feature is bitrotted on master, but worth aligning if it's revived;
    same for the missing BridgeNode arm in recursive_cata_dispatch under bridge_nodes.

@adamv-symbolica

Copy link
Copy Markdown

Path byte should not be represented in prefix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants