From 73a658550e59b8d0ea81e7f5fd45d26acafd6946 Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Sun, 20 Sep 2026 10:29:50 -0400 Subject: [PATCH 1/4] =?UTF-8?q?docs(plans):=20ADR-007,=20ADR-008,=20ADR-00?= =?UTF-8?q?9=20=E2=80=94=20the=20cache=20redesign,=20as=20one=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings the three draft ADRs from the 2026-09-18 cache audit, and the spikes they cite, onto one branch so they can be reviewed together. They were drafted as separate PRs (#180, #181, #182) and revised there during the 2026-09-19/20 design session; this commit is their state at the end of that session, unchanged. - ADR-007: tallyman owns result materialization (no xorq cache nodes in builds); Buckaroo is a displayer; every diff is built as an entry. - ADR-008: every file carries a visible __row_order column and every page request sorts by it. - ADR-009: materialization runs single-partition, and result_digest is a digest of the snapshot's content. Co-Authored-By: Claude Fable 5.1 --- .../ADR-007-tallyman-owned-materialization.md | 498 ++++++++++++++++++ plans/ADR-008-row-order-of-reads.md | 382 ++++++++++++++ plans/ADR-009-digest-stability.md | 268 ++++++++++ scripts/spike_bare_read_chaining.py | 117 ++++ scripts/spike_float_aggregate_digest.py | 77 +++ scripts/spike_logical_digest.py | 229 ++++++++ scripts/spike_row_order_paging.py | 159 ++++++ scripts/spike_window_read_order.py | 148 ++++++ 8 files changed, 1878 insertions(+) create mode 100644 plans/ADR-007-tallyman-owned-materialization.md create mode 100644 plans/ADR-008-row-order-of-reads.md create mode 100644 plans/ADR-009-digest-stability.md create mode 100644 scripts/spike_bare_read_chaining.py create mode 100644 scripts/spike_float_aggregate_digest.py create mode 100644 scripts/spike_logical_digest.py create mode 100644 scripts/spike_row_order_paging.py create mode 100644 scripts/spike_window_read_order.py diff --git a/plans/ADR-007-tallyman-owned-materialization.md b/plans/ADR-007-tallyman-owned-materialization.md new file mode 100644 index 0000000..404cd5d --- /dev/null +++ b/plans/ADR-007-tallyman-owned-materialization.md @@ -0,0 +1,498 @@ +# ADR: Tallyman owns result materialization (no xorq cache nodes in builds) + +- **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling + session, which added the governing rule, decision D10, and the resolution + recorded under D5). Supersedes two decisions of + `plans/ADR-006-read-path-loads-builds.md`: its D4 (chaining inlines the + parent's cache node) and its D8 (the manifest records the snapshot key and + reads assert it). Five other ADR-006 decisions keep their intent: D5 (the + canonical sort), D6 (a missing build is a hard error), D7 (verification runs + in production and is loud), D10 (an unfaithful heal wipes the entry's + Buckaroo state) and D12 (unfaithful entries are pinned and badged). The last + three attach to `ensure_materialized`, which is this ADR's D5. +- **Reading decision labels:** a bare label such as "D5" in this document + always means this ADR's own decision. Another ADR's decision is always + written with its ADR number and a few words saying what it decides. +- **Context:** the 2026-09-18 cache audit (tallyman @ `a748ea6`, buckaroo + 0.15.4, xorq 0.3.26). One finding is filed upstream of tallyman: + buckaroo-data/buckaroo#972 (`/load_expr` has no `cache_dir`). The direction + was set by Paddy the same day: "I want to depend on xorq as little as + possible for caching." +- **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`), + `src/tallyman_xorq/result_cache.py` (`_resolve_result_plan`, + `cached_result_expr`, `entry_graph_expr`, `baked_snapshot_path`, + `_cached_node_path`, `_assert_recorded_snapshot_key`, `_verify_self_heal`), + `src/tallyman_xorq/build.py` (the execute-once step, `build.py:496-557`), + `src/tallyman_xorq/io.py` (`tracked_expr_from_alias`, + `pinned_expr_from_alias`), `src/tallyman_xorq/portable.py` + (`rewrite_cache_dirs`), `src/tallyman_core/manifest.py` (`snapshot_key`), + `src/tallyman_companion/buckaroo_lifecycle.py` (`load_session`), + `docs/system-contract.md`. +- **Related ADRs:** `plans/ADR-002-source-identity-content-hash.md` (content in + the path; D3 reuses the device), `plans/ADR-003-result-cache-cost-rubric.md` + (its motivating misclassification; see Consequences), + `plans/ADR-008-row-order-of-reads.md` and + `plans/ADR-009-digest-stability.md` (the other two hash- or digest-changing + decisions that share this ADR's corpus rebuild). +- **Evidence:** `scripts/spike_bare_read_chaining.py` (results under D3), and + the audit measurements quoted in the Problem section. + +## Terms + +- **Entry:** one catalog computation, stored under its content hash. +- **Build:** the `xorq_build/` directory inside an entry, which is the entry's + computation graph written to disk by xorq. +- **Replay a build:** load that directory back into a live expression and + execute it. +- **Materialize:** run an entry's computation once and write the result to a + parquet file. That file is the entry's **snapshot**, and it lives under + `compute_cache/result_cache/`. +- **Worthy entry:** an entry tallyman materializes, because its graph does work + that is expensive or that cannot inherit a row order (an aggregate, join, + sort, window function, UDF, union). **Cheap entry:** one it does not, whose + small plan re-runs on every read. +- **Cache node:** xorq's `CachedNode`, a marker inside a graph meaning "look for + a file with this key, and if it is missing run the graph below me and write + it". +- **Chaining:** a recipe building on another entry through + `tracked_expr_from_alias` or `pinned_expr_from_alias`. +- **Heal:** re-create a snapshot that is missing from disk by re-running the + entry's build. +- **Session:** one grid's state inside the Buckaroo server process. +- **View build:** a build whose whole graph is one step, "read this parquet + file". +- **Live diff:** the compare grid shown when two versions are diffed. A + **promoted diff** is one that has been saved as a catalog entry. +- **Checkpoint:** tallyman's step that zips new entries and makes one git + commit in the catalog repository. + +## Problem + +Tallyman's result cache is xorq's cache. `rewrite_for_build` +(`source_cache.py:241-243`) wraps every worthy expression in +`.cache(cache=ParquetSnapshotCache(...))`, and that one call decides everything +else: the file's name is xorq's snapshot key, its directory is a base path that +xorq does not serialize, its writer is xorq's `ParquetStorage`, it is written as +a side effect of `loaded.count().execute()` (`build.py:540`, +`result_cache.py:733`), a missing ancestor is regenerated by a nested cache node +noticing its own file is gone, and Buckaroo is handed a build that contains all +of this. + +The audit found the following, each a consequence of that arrangement. + +1. **Snapshots escape the project.** xorq serializes a cache node's + `relative_path` and never its base path, and `load_expr(cache_dir=...)` + redirects only the root cache node. + - Buckaroo's `/load_expr` calls `load_expr(build_dir)` with no `cache_dir`, + so the viewer never reads tallyman's snapshot. The first view re-executes + the whole graph inside Buckaroo and writes a second copy under + `~/.cache/xorq/result_cache/`. On the dev machine that directory held 68 + files and 14 GB, with the same keys and sizes as the project's + `compute_cache` (buckaroo#972). + - `build.py:496` loads the new build with `cache_dir` but without + `rewrite_cache_dirs`, so nested ancestor nodes resolve to `~/.cache/xorq` + while a child builds. Every child build re-executes its expensive parent + and leaves a duplicate there. Reproduced in scratch homes where Buckaroo + never ran. +2. **The writer is unsafe under concurrency.** `ParquetStorage` writes through + a fixed `.parquet.tmp`. Four concurrent cold writers of one key gave + three `FileNotFoundError`s and a corrupt final file, and because a hit is + decided by file existence the corrupt file is then served forever. + `_heal_lock` covers threads in one process only, builds take no lock, and + FastMCP runs sync tools in a threadpool, so parallel tool calls do build + concurrently. +3. **The file shape is poor.** One row group per DataFusion batch (at most + 8,192 rows), Snappy. A real 3.68 GB snapshot has 9,525 row groups and a + 44.9 MB footer. `cached_result_expr` constructs a fresh read on every call + (`result_cache.py:742`), so that footer is opened again for every page + request, and every call registers another table in the shared backend + (40 reads, 40 tables). +4. **The bytes depend on how the node was executed.** xorq stamps provenance + metadata only when the cache node is the root of the executed expression: + 1,234 bytes against 858 for the same rows. Build and heal agree today only + because both happen to call `count()`. +5. **Inlined chaining misclassifies children.** Under ADR-006 decision D4 + (chaining inlines the parent's cache node) a child's + graph contains its parent's Aggregate and canonical Sort, so a filter over a + cached aggregate is itself worthy and writes a full copy. This is ADR-003's + motivating bug. Graphs also grow with chain depth, which is the cost the + pre-#74 per-entry parquet boundary existed to avoid (#82). +6. **Tallyman carries code whose only job is to aim xorq's cache.** + `rewrite_cache_dirs`, the contract's "every loader must supply `cache_dir`", + `manifest.snapshot_key` with `_assert_recorded_snapshot_key` (ADR-006 + decision D8, the snapshot-key tripwire), + and `_cached_node_path`. + +xorq is frozen upstream for this project, so every item above needs a +tallyman-side workaround for as long as xorq's cache is in the path. + +## Governing rule + +Stated by Paddy in the grilling session (2026-09-19 and 2026-09-20), and the +reason several decisions below are consequences and not separate choices: + +> Buckaroo is a very good displayer. It runs queries only for summary stats, +> sorting and paging. Anything more is done by tallyman first. If something +> tallyman asked Buckaroo to display does not exist, that is on tallyman. + +> When the MCP asks tallyman to create an entry, tallyman should run the query +> and materialize the parquet if necessary immediately. Tallyman shouldn't call +> Buckaroo to display an entry until the original query has finished. + +> I want a cohesive system that works reliably, then we can worry about speed +> problems as they come up. We don't have a cohesive system now. + +That last statement sets the priority for all three ADRs of this set (this one, +`plans/ADR-008-row-order-of-reads.md` and `plans/ADR-009-digest-stability.md`): +where a uniform rule and a faster special case compete, the uniform rule is the +decision and the faster path is noted for later. + +So tallyman runs an entry's computation, or a diff, to completion before it +asks Buckaroo to show anything. No other process runs an entry's expensive +computation, writes result files, or repairs tallyman's cache. Besides being +simpler, this puts every failure of a computation in tallyman's process, where +it can be logged and reported, and none inside a grid query in Buckaroo. + +## Decisions + +### D1. Builds carry no cache nodes + +`rewrite_for_build` keeps the in-memory-read rejection and the canonical sort +(ADR-006 decision D5, so the sort stays inside the build) and stops calling +`.cache()`: + +```python +if _is_worthy_expr(expr): + expr = _canonical_sorted(expr) +``` + +The source-read injection (`source_cache.py:215-234`) is deleted with it. It has +no producer today: `deferred_read_csv` is a build error and no JSON reader +exists. A recipe that arrives already containing a `CachedNode` becomes a build +error next to the in-memory check, because a node with default storage would +write under `~/.cache/xorq`. + +`source_cache.py:229` and `:243` are the only places in `src/` that create a +cache node, so this one change removes xorq's cache from tallyman's data path. +xorq remains the expression, build, load, hashing and execution layer. + +Removing the node alone would turn every worthy entry into a recompute entry +(`_resolve_result_plan` treats "no `CachedNode` on top" as recompute, +`result_cache.py:642-649`), so D2 to D6 land in the same change. + +*Rejected:* patch ADR-006 decision D4 (inlined chaining) in place. Deep-rewrite +the build's load (a one-line +change, verified to stop the build leak with no hash change), wait for +buckaroo#972, put a cross-process lock around xorq's writer, and pre-seed +xorq's files with a better writer (a hit is a bare existence check, so tallyman +can write `.parquet` itself). This keeps every hash and needs no rebuild. +It also keeps the file's name as xorq's tokenization of the graph (so the +snapshot-key tripwire of ADR-006 decision D8 stays), keeps item 5, and leaves each fix as a workaround for +behaviour in a dependency that cannot be changed from here. + +### D2. A snapshot's location is a function of the content hash + +`snapshot_path(project, content_hash)` returns +`compute_cache/result_cache/.parquet`. Whether an entry has a +snapshot comes from the manifest's `cache_worthy`, as it does now. + +`cached_result_expr` keeps its name and signature (ADR-006 decision D2, "keep +the facade"). For a worthy +entry it returns one bare read of `snapshot_path`, memoized per +`(project, content_hash)` for the life of the process, so repeated reads stop +registering tables and stop re-opening the footer. A worthy entry whose file +exists is served without loading its build at all. For a cheap entry it returns +the loaded graph, as now. + +Deleted: `manifest.snapshot_key`, `_assert_recorded_snapshot_key`, +`_cached_node_path`, `rewrite_cache_dirs`, and the `cache_dir` argument. With +the path computed from the hash there is no second derivation for a tripwire to +compare against. + +*Rejected:* `-.parquet`, which would make a child name +its parent's exact bytes. An unfaithful heal (a heal whose result does not +match the recorded digest, ADR-006 decision D12) would then write a +file no existing child can find, and a flagged condition would become a hard +failure for the whole subtree. The digest stays in the manifest, where verify +already looks. + +### D3. Chaining through a worthy parent is a bare read of its snapshot + +`tracked_expr_from_alias` and `pinned_expr_from_alias` return +`deferred_read_parquet(snapshot_path(parent))` for a worthy parent, after +making sure the file exists (D5). For a cheap parent they return the parent's +loaded graph, which by D1 contains no cache nodes. `entry_graph_expr` and +`cached_result_expr` become the same function. + +The child's hash covers the literal path of the parent's snapshot, and that +path contains the parent's content hash. A child's identity is therefore a +function of its parent's identity, which is the device ADR-002 already uses for +sources: if you want content identity, put it in the path. + +Measured with `scripts/spike_bare_read_chaining.py` (raw xorq, no cache nodes): + +| Question | Result | +| --- | --- | +| Child hash, snapshot rewritten with other bytes at the same path | unchanged | +| Child hash, same bytes at another path | changes | +| `build_expr` of a child while the parent's snapshot is absent | raises `FileNotFoundError: local path does not exist` | +| `load_expr` of the child's build while the snapshot is absent | succeeds | +| Executing it in that state | raises `ValueError: At least one path is required` | +| Parent re-materialized from its own build, then the child executed | digest unchanged; child rows match the reference (19,777) | +| `classify_build` of a filter + computed column over the snapshot | cheap | +| `classify_build` of the same child with the parent graph inlined | worthy (`ops:Aggregate,Sort,SortKey`) | +| Child `expr.yaml` | carries the literal path; 3,434 bytes against 6,903 inlined | +| Files written under `XORQ_CACHE_DIR` | none | + +So the graph is cut at every worthy entry, a child of an aggregate is cheap, +and the literal path in `expr.yaml` means `make_portable_inplace` handles it +with the existing `${TALLYMAN_PROJECT_ROOT}` placeholder. + +*Rejected:* keep inlining the parent's graph without its cache node. Every read +of every descendant would re-run the parent's expensive subgraph. + +### D4. One writer, used by the build and by every heal + +`materialize(project, content_hash)` loads the entry's build, executes it as a +record-batch stream, and writes the snapshot itself: + +- a unique temp name in the destination directory, then `os.replace`; +- under a per-hash cross-process `flock`, with the existence check repeated + inside the lock, so a second process waits and then finds the file; +- it numbers the rows as it writes them, in a last column named `__row_order` + (decision D2 of `plans/ADR-008-row-order-of-reads.md`); +- it returns the digest of what it wrote. + +The build's execute-once step and every heal call this function, so the +contract's I2 ("result bytes are manufactured exactly once; a self-heal is +reproduce-and-verify") holds because there is one routine that manufactures +bytes. The file format and the digest definition are ADR-009's. + +*Rejected:* keep `ParquetStorage` behind a tallyman lock. That fixes the race +and keeps items 3 and 4. + +### D5. One entry point makes files exist: `ensure_materialized` + +`ensure_materialized(project, content_hash)` guarantees that every snapshot an +entry's plan reads is on disk before anything executes: + +1. If the entry is worthy and its snapshot exists, return. No build is loaded. +2. Otherwise load the entry's build and collect the snapshot paths its `Read` + nodes point at (any read under `compute_cache/result_cache/`). The list is + kept with the loaded plan in the existing LRU. +3. For each of those that is missing, recurse on the hash in its file name. +4. If the entry is worthy, `materialize` it. + +Every file it writes is verified against the manifest's `result_digest` before +it is served. A mismatch takes the existing path: a durable `unfaithful_heal` +record and the SSE event (ADR-006 decision D7, loud verification), the +stat-cache wipe and session eviction (ADR-006 decision D10), and the pin and +badge (ADR-006 decision D12). + +Callers: the canonical read (`cached_result_expr`, on every call, where step 1 +or a handful of `stat` calls is the whole cost, and which covers diff +composition), chaining at mint time (D3), `load_session` (D6), and the verify +sweep. + +ADR-006 decision D4 rejected bare-read chaining because "builds stay +non-self-contained and the pre-heal choreography stays load-bearing forever". +What that decision bought was a build that repairs its own ancestors when +something other than tallyman executes it cold. Buckaroo is the only such +thing, and the comment at `buckaroo_lifecycle.py:570-575` says the design +relied on it: "Buckaroo's replay of the build regenerates any evicted snapshot +through ordinary cache mechanics on first query." Under the governing rule +Buckaroo never does that, so the property has no user and nothing is given up. +This was question 1 of the grilling session, resolved 2026-09-20. + +The old pre-heal was also weaker than this function. It was a +`cached_result_expr` call with a discarded result at one call site, and +ancestors were healed only because reads then re-executed recipes. Here the +requirement is a precondition of the one canonical read that every in-process +consumer already uses (contract invariant I3, "one read semantics"), and it is +computed from the build's own reads, so it cannot drift from what execution +opens. + +*Rejected:* derive the required snapshots from `manifest.parents`. It needs only +JSON reads, but it depends on the recorded edges being complete and on each +parent's recorded worthiness matching what chaining did when the child was +minted. Those edges are recalc policy and are not complete today: a promoted +diff's recipe calls `build_diff_expr`, which reads both sides through +`cached_result_expr` and records no parent edge at all. The build's reads are +exactly what the engine will open. + +### D6. Buckaroo is handed something that already exists + +This is the governing rule applied to entry grids. + +For a worthy entry `load_session` calls `ensure_materialized`, then posts a +view build of the snapshot, written once to a stable per-entry directory +(Buckaroo's stat-cache keys include the build directory's path, which is why +the expanded build already lives at a stable path). Buckaroo never executes an +aggregate, join or sort on tallyman's behalf and never writes a snapshot, and +tallyman stops depending on buckaroo#972, whose premise (teach Buckaroo where +tallyman's cache is) the rule contradicts. The grid and `/api/data` read the +same file (contract invariant I5, "one question, one path"). + +For a cheap entry it calls `ensure_materialized` and posts the entry's own +expanded build. A cheap build is a view in the database sense: a stored +definition (filter these rows, keep these columns) over files that exist. +Buckaroo's stats, sorts and pages run through it. Tallyman has already executed +that plan once, in full, at build time, so an error in it has already surfaced +in tallyman. + +Paddy confirmed this reading on 2026-09-20 (question 4 of the grilling +session). The words that decide it are "materialize the parquet if necessary": +a file is written when the entry is created and only for a worthy entry, and +nothing is written when an entry is viewed. The alternative, writing a file the +first time a cheap entry is opened so that Buckaroo only ever reads one file, +was set aside. It costs a wait on first open and a full copy per viewed +revision; the audit measured 19 GB of cache against 779 MB of data when every +CSV revision wrote a copy. + +The timing half of the rule already holds for creation. The build executes the +entry once before it writes the manifest, a worthy entry's snapshot is written +in that step (D4), and an entry with no manifest is treated as absent, so +Buckaroo cannot be asked to display an entry whose query is still running. The +same ordering now covers a snapshot that was deleted later: `load_session` +waits for `ensure_materialized` before it posts anything. + +Deleting a snapshot (the Cache page, a reset prune, a future budget eviction) +ends every live Buckaroo session whose plan reads it first, using the +session-eviction hook that ADR-006 decision D10 introduced. The next +`/api/session` re-materializes and opens a new session. Without this, a page +request against a deleted file would fail inside Buckaroo with the +`At least one path is required` error above, which is exactly the kind of +failure the rule says belongs to tallyman. + +This resembles what #104 removed: #102's viewer build over +`/result.parquet`. #104's objection was two materialized copies per +entry and two read paths. Here there is one copy and the view build only points +at it. + +*Rejected:* post the entry's own build for worthy entries too. With no cache +node in it, Buckaroo would re-run the full computation for every page and stat +query. + +### D7. The cold state is an empty `compute_cache` + +The contract's cold seam today is the `cache_dir` argument, and the standing +tests exercise it by deleting `compute_cache/` and reading again +(`tests/test_lineage_faithful_reads.py`). That remains the test: with +`compute_cache/` removed, the canonical read must reproduce every snapshot the +entry needs, each with its recorded digest. The `cache_dir` parameter leaves +the contract, since nothing in a build resolves through it any more. + +### D8. A sentinel test keeps xorq's cache out of the path + +With `XORQ_CACHE_DIR` pointing at an empty sentinel directory, a build, a +chained child build, a view, an eviction and a heal must leave the sentinel +empty. The test fails on `main` today (finding 1), so it belongs in the +failing-tests commit, and it outlives this change as the check that no xorq +cache node has crept back in. + +### D9. One change, one rebuild + +Removing the cache node changes the hash of every worthy entry, and bare-read +chaining changes every child's. This lands together with ADR-008's change to +`tallyman_read_csv` and ADR-009's digest definition, behind a single corpus +rebuild, with the failing tests committed and seen red first. After the +rebuild, `~/.cache/xorq/result_cache` (14 GB) and the older leaks under +`~/.cache/xorq/parquet/` (2.6 GB) can be deleted by hand. + +### D10. Every diff is built as an entry before it is displayed + +Live and promoted diffs already build the same expression +(`build_compare_expr`). The live path posts it to Buckaroo unmaterialized +(`_build_compare_expr`, `app.py:418-441`), so the outer join runs again for +every page, sort and stat query in the diff grid, and a failure of the join +surfaces inside Buckaroo. The promote path writes a recipe that calls +`build_diff_expr(a_hash, b_hash, keys)` and runs the normal build. + +Every diff now takes the promote path. When a diff view is opened, tallyman +builds the diff entry and waits for the build to finish. A diff contains a +join, so it is worthy and is materialized, and Buckaroo is then handed a view +build of the finished file (D6) with the diff's display configuration. The page +shows a "building diff" state while it waits. Promoting a diff is reduced to +putting an alias on an entry that already exists. + +- **An unnamed diff entry is ephemeral.** The checkpoint zips and commits every + complete directory under `entries/`, aliased or not (`zip_pending_entries`, + `catalog.py:160-180`), so an unnamed diff stored there would put every diff + ever viewed into the catalog's git history. Ephemeral entries live under + `compute_cache/ephemeral_entries//`, where everything is + already defined as deletable at any time, and they can be rebuilt from the + two hashes and the keys. Promote moves the directory into `entries/`, sets + the alias and checkpoints. The content hash is computed from the graph, so + the move does not change it, and the snapshot path stays the same. +- **Parents.** A worthy side is read from its snapshot. A cheap side is not + copied first: because the diff is itself materialized, each side is read + exactly once, while the diff is written. +- **Sessions.** The same entry can be opened as a diff, with the diff display + classes, or as a plain entry, so the Buckaroo session key includes the view + kind and is no longer the content hash alone. +- **Retired with this:** `_build_compare_expr` and its temp build directory, + the separate diff-session bookkeeping (`diff_session_is_loaded`, + `mark_diff_session_loaded`), and `diff_stat_cache/`, since a diff's Buckaroo + stats become its entry's own. The audit finding that every recalc wipes all + of `diff_stat_cache/` goes with it. + +*Rejected:* keep posting the join and materialize nothing. It shows a first +page sooner, and it breaks the governing rule in both directions: Buckaroo runs +tallyman's join, repeatedly, and tallyman never learns whether it succeeded. + +## Consequences + +- **Retired:** the `.cache()` call and the source-read injection in + `rewrite_for_build`; `rewrite_cache_dirs`; `_cached_node_path`; + `manifest.snapshot_key` and `_assert_recorded_snapshot_key`; the `baked` / + `recompute` plan split keyed on a `CachedNode`; the thread-only `_heal_lock` + and the `(FileNotFoundError, ValueError)` retry around xorq's shared temp + file; `entry_graph_expr` as a separate function. +- **ADR-006:** its D4 (inlined chaining) and D8 (snapshot-key tripwire) are + superseded. Its D2's "snapshot path derived from the loaded expression" + becomes `snapshot_path`. Its D3 (rebind composition onto the default backend) + is still needed for cheap parents and for diff composition. Its D5 (canonical + sort) and D6 (a missing build is a hard error) are unchanged. Its D7, D10 and + D12 (loud verification, the Buckaroo-state wipe, the pin and badge) attach to + `ensure_materialized`. +- **`docs/system-contract.md`** needs rewriting in Part 1 ยง4 (xorq caching + becomes background, not mechanism), "Content hash" (no cache injection in the + hashed expression; parents appear as snapshot paths), "Manifest" (drop + `snapshot_key`), "Worthiness", write path steps 2 and 5, the read path, + "Chaining", and the `[^preheal]` footnote, which currently describes + bare-read chaining as a retired workaround. +- **`plans/remove-ondemand-result-parquet.md`:** its premise that xorq's + snapshot is the single materialized copy is replaced. Tallyman's snapshot is + the single copy. +- **ADR-003:** chained descendants of an expensive entry are cheap, so its + motivating lineage stops filling the cache when the iterations chain off the + join. A revision that restates the join in its own recipe is still worthy + and still writes a copy, so the budget and eviction half of ADR-003 remains + and should be rewritten against this design. +- **CSV lineages:** a child of a `tallyman_read_csv` entry reads the parent's + snapshot and no longer inherits its Sort, so revisions stop baking one full + sorted copy each. The root entry's own copy is ADR-008's subject. +- **Buckaroo's stat keys** for a worthy entry become a function of one read of + one content-addressed path. +- **Source identity `salt` mode:** `rewrite_for_build` returns early under + `salt` because xorq's path-only snapshot keys would collide. A snapshot named + by the entry's content hash has no such collision, so the early return should + become unnecessary. Not tested. +- **Cost accepted:** a worthy parent's snapshot must exist before a child can + be built. A build also no longer repairs its own ancestors when something + outside tallyman executes it, and under the governing rule nothing does. + +## Open questions + +1. **Deep cheap chains.** Nothing cuts the graph between cheap entries. Run + `tests/test_perf_chain_depth.py` against this design and decide whether a + node-count or `compile_seconds` threshold should make an otherwise cheap + entry worthy. +2. **An unfaithful parent's descendants.** Descendants of an entry whose heal + failed verification were computed from bytes that no longer exist. Nothing + flags them today, and nothing here does either. +3. **Eviction policy.** D6 says what eviction must do to live sessions. Which + snapshots to evict, and when, stays with the ADR-003 rewrite. +4. **Garbage collection of ephemeral entries.** D10 says where they live and + that they are deletable. When to delete them belongs with the eviction + policy of open question 3. diff --git a/plans/ADR-008-row-order-of-reads.md b/plans/ADR-008-row-order-of-reads.md new file mode 100644 index 0000000..a7e0eea --- /dev/null +++ b/plans/ADR-008-row-order-of-reads.md @@ -0,0 +1,382 @@ +# ADR: Row order of reads (every file carries `__row_order`, every page sorts by it) + +- **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling + session). The first draft pinned row order with an engine setting. Paddy + proposed baking a row-order column into every file tallyman writes and + sorting every page by it. The measurements below favour that, so it is now + the decision and the engine setting is the rejected alternative under D5. + Amends `plans/ADR-005-intelligent-csv-import.md` INV-1 (the name and position + of the row-order column) and INV-2 (the trailing `order_by`). Corrects a + threshold quoted in `plans/ADR-006-read-path-loads-builds.md` decision D5 + (the canonical sort) and three other places. +- **Context:** the 2026-09-18 cache audit (tallyman @ `a748ea6`, buckaroo + 0.15.4, xorq 0.3.26, xorq-datafusion 0.2.7). No ticket filed yet. +- **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`, + `_tie_break_order`), `src/tallyman_xorq/io.py` (`read_project_file`, + `tallyman_read_csv`, `io.py:627`), `src/tallyman_xorq/result_cache.py` + (`_EXPENSIVE_OPS`, `classify_build`), `src/tallyman_companion/app.py` + (`api_data`, `app.py:930`, and the chart data it feeds), + `src/tallyman_xorq/primary_key.py` (candidate selection, + `primary_key.py:219`), `src/tallyman_companion/diff.py` + (`build_compare_expr`), and the `materialize` writer introduced by + `plans/ADR-007-tallyman-owned-materialization.md` decision D4. Buckaroo's + paging is Buckaroo's code and is covered by D8. +- **Related ADRs:** `plans/ADR-004-result-digest-canonical-ordering.md` (why a + canonical stored order exists), `plans/ADR-007-tallyman-owned-materialization.md` + (what a snapshot is, and the corpus rebuild this shares), + `plans/ADR-009-digest-stability.md` (the file format, which D5 adds a + requirement to). +- **Evidence:** `scripts/spike_row_order_paging.py` (the decisions) and + `scripts/spike_window_read_order.py` (the problem, and the rejected + engine-setting approach). All figures are from those scripts on a 14-core + machine. + +## Terms + +- **Entry:** one catalog computation, stored under its content hash. +- **Materialize:** run an entry's computation once and write the result to a + parquet file. That file is the entry's **snapshot**. +- **Worthy entry:** an entry tallyman materializes. **Cheap entry:** one it + does not, whose small plan re-runs on every read. D4 draws the line. +- **Page request:** a request for `limit` rows starting at `offset`. `/api/data`, + charts and Buckaroo's grid all issue them. The first draft of this ADR called + it a "window". +- **Row-preserving:** each output row comes from exactly one input row, and no + input row produces more than one output row. Filters, column selections, + computed columns, renames and casts qualify. Aggregates, joins, unions, + distincts and unnests do not. +- **Tie:** two or more rows with equal values in every sort key. +- **Exchange operator:** a DataFusion physical-plan step (`RepartitionExec`, + `CoalescePartitionsExec`) that moves rows between parallel partitions. After + one, rows arrive in whatever order the partitions finish. + +## Problem + +Decision D5 of ADR-006 (the canonical sort) made the write deterministic: a +worthy entry's snapshot is written in a fixed total order, so the file is the +same on every rebuild. Nothing made the read of that file deterministic. +`/api/data` serves a page as +`cached_result_expr(project, hash).limit(limit, offset=offset).execute()` +(`app.py:930`), charts pull `limit=100000` through the same endpoint, and +Buckaroo pages the grid the same way in its own process. A `LIMIT/OFFSET` with +no `ORDER BY` takes rows in whatever order the plan delivers them. + +Eight identical requests for 50 rows from a 91 MB parquet file whose rows are +physically sorted by `id` (`scripts/spike_window_read_order.py`): + +| Plan | Offset | Distinct pages out of 8 | First id seen (file order would give) | +| --- | --- | --- | --- | +| read, limit | 0 | 5 | 0, 434176, 1294336 (0) | +| read, limit | 1,000,000 | 8 | 1360448, 1368640, 1565248 (1000000) | +| read, filter, computed column, limit | 0 | 8 | 1, 221185, 647168 (1) | +| read, filter, computed column, limit | 1,000,000 | 8 | 746336, 754526, 967522 (1500001) | + +A sort does not help when its key has ties. Sorting 3,000,000 rows by a column +with 200 distinct values and asking for the page at offset 100,000 returned 6 +different pages for 6 identical requests (`scripts/spike_row_order_paging.py`). +Sorting the grid by a category or a date is exactly that case. + +Paging through a large entry therefore repeats some rows and never shows +others, a chart over more than 100,000 rows changes each time it mounts, and an +author's own `order_by` is not honoured on screen: the file is sorted and the +page taken from it is not. + +**The governing variable** for the unsorted case is whether the physical plan +has an exchange operator between the scan and the limit. There are three ways +to get one: + +- DataFusion splits a file scan into byte ranges, one per partition, when the + file is larger than `datafusion.optimizer.repartition_file_min_size`. In this + engine that is 10,485,760 bytes (`SHOW ...` on a fresh `xo.connect()`), not + the 1 MiB the repo states. +- Above a single-partition scan the planner still inserts + `RepartitionExec: RoundRobinBatch(14)` under a filter or projection. +- A hash aggregate or join repartitions by key. This, and not file splitting, + is what the #171 probe observed: its fixture is a 2.07 MB file, below the + split threshold, and its plan is an aggregate. + +**What INV-2 of ADR-005 was for.** INV-2 keeps a trailing +`order_by("original_row_order")` on every `tallyman_read_csv` expression so +that the entry is worthy and its order is canonical. Its costs, measured in the +audit: + +- Every entry in a CSV lineage is worthy for that Sort alone, so every revision + writes a full sorted copy. A 9.3 MB CSV with four trivial revisions produced + 37 MB of snapshots. One real project holds 779 MB of data and 19 GB of + `compute_cache`, with ten snapshots of 0.45 to 3.7 GB that are all + `why=ops:Sort,SortKey`. +- Primary-key inheritance is gated on `not cache_worthy` + (`primary_key.py:204`), so it never applies in a CSV lineage and every + revision pays a full-table distinct scan. +- Above 10 MB it does not deliver a stable order on screen, for the reason + above. + +## Decisions + +### D1. The contract: a page is a function of `(content_hash, sort, offset, limit)` + +The same page request returns the same rows in the same order, in any process +and any cache state, with or without a user sort. With no user sort the rows +come in `__row_order` order (D2). This is the system contract's invariant I1 +("a content hash names a fixed result") applied to a page. + +### D2. Every file tallyman reads carries `__row_order` + +`__row_order` is an `int64` column holding `0..N-1` in the file's physical row +order. It is the last column, and it is visible: Buckaroo shows it as the final +column of the table. + +Two writers produce it: + +- **`materialize`** (ADR-007 decision D4, the one writer of snapshots) numbers + the rows of the canonically sorted stream as it writes them. If the stream + already has a `__row_order` inherited from a parent, the writer replaces that + one column. Each materialization therefore overwrites `__row_order` with + positions in its own file. +- **Ingest.** A source file enters tallyman through an ordered copy: polars + scans it, `with_row_index` numbers the rows in file order, and the copy is + written with the column last. CSVs already work this way (the intermediate + parquet under `csv_ordered/`). Parquet sources gain the same step, keyed by + the source's digest, beside the content-addressed clone that stays the + immutable input. A source that already has a `__row_order` column has it + overwritten, which is the right outcome for a file tallyman exported. + +The canonical sort's tie-break (`_tie_break_order`) puts an inherited +`__row_order` where `original_row_order` is today: after the author's own +`order_by` keys and before the remaining columns. A worthy entry that keeps its +parent's rows, such as one adding a window function, therefore keeps the +parent's order. + +*Rejected:* `row_number()` inside the entry's graph. It needs the same global +sort, adds a window function to every worthy build, and leaves contiguity to +the engine. A counter in the writer is contiguous and physical by construction, +which D5's range requests depend on. +*Rejected:* a row number kept only as file metadata. DataFusion exposes no +parquet row number that a query can sort or filter by, so it has to be a column. + +### D3. A cheap entry that drops `__row_order` is a build error + +`__row_order` is metadata that happens to be a column, and it is not to be +deleted. A column selection is an allow-list: `t.select("g", "n")` drops every +column it does not name, so an author who is not thinking about row order +drops it without meaning to. When a cheap entry's output lacks `__row_order`, +the build fails, and the error names the parent entry, says that a +row-preserving view must keep `__row_order` so that paging stays repeatable, +and shows the fix (`t.select("g", "n", "__row_order")`). The MCP tool +descriptions say the same thing up front. This is the feedback channel +`tallyman_read_csv` already uses when a CSV has a column named +`original_row_order`. + +A worthy entry is exempt, because the writer numbers its rows (D2). An author +changes `__row_order` by asking for an order: `order_by` makes the entry worthy, +and the writer numbers the rows in the requested order. Assigning to the column +directly stays an error (D6). + +Tallyman makes one alteration of its own, at the top of the expression only: it +moves `__row_order` to the last position, since a computed column added after +it would otherwise push it into the middle of the table. + +The column can be copied for debugging. +`foo_v1.mutate(__row_order_v1=foo_v1["__row_order"])` gives the new entry both +columns. Once the new entry is materialized, `__row_order` holds its own +positions and `__row_order_v1` still says where each row sat in the parent. + +*Rejected:* carry the column automatically. `rewrite_for_build` can rewrite +every column selection in a cheap graph to keep it, and the spike shows that +working: `t.filter(t.g < 100).select("g", "v0").mutate(z=t.v0 * 2)` comes out +with columns `['g', 'v0', 'z', '__row_order']` and pages repeatably. It was +rejected because the recipe text and the entry's columns would then disagree, +because it is surgery inside an expression an LLM wrote, and because a mistake +the author can fix in one line is better reported than silently repaired. The +cost accepted is a failed build whenever a select list forgets the column. +*Rejected:* carry the column and hide it from the grid. Paddy's call: it is +shown, as the final column. + +### D4. Cheap means row-preserving over one file; everything else is materialized + +A cheap entry inherits its row order, so it must be a row-preserving plan over +exactly one file. The classifier changes from a deny-list to an allow-list: an +entry is cheap only if every relation operation in its graph is known to be +row-preserving (a file read, a filter, a column selection, a computed column, a +rename, a cast, a column drop). Anything else is worthy, including operations +nobody has thought about yet. Today's `_EXPENSIVE_OPS` deny-list classes +`Union`, `Distinct` and `Unnest` as cheap, and none of them can carry one +parent's row order. + +A new or unknown operation now costs a copy (safe) instead of unstable paging +(unsafe). `classify_build` (which reads the serialized build) and +`_is_worthy_expr` (which reads the live expression) flip together, as they must +today. + +Supporting measurement from the first draft: a union of two files returned 2 +different pages for 8 identical requests even on a single-partition +connection, because `UnionExec` emits one partition per input and an exchange +operator merges them. + +### D5. Every page request orders by `__row_order` + +- No user sort: `ORDER BY __row_order`. +- User sort: the user's keys, then `__row_order` ascending as the last key, + which breaks every tie. + +That is the whole rule, for every entry and both processes. Two faster paths +exist and are deliberately not part of this decision (Paddy, 2026-09-20: a +cohesive system that works reliably comes first, and speed problems are handled +as they come up): + +- **Declared file order.** Telling the engine the file is already sorted by + `__row_order` removes the sort from the plan. +- **Range request.** For the unfiltered, unsorted view of a materialized file a + row's position equals its `__row_order`, so a page can be fetched as + `__row_order >= offset AND __row_order < offset + limit` instead of `OFFSET`. + +Both are measured below so the numbers are on hand when they are wanted. + +Measured on 3,000,000 rows by 14 columns (287 MB), on the default parallel +connection with no engine settings. Every row of the table returned the correct +page 6 times out of 6: + +| Page request | Offset 0 | Offset 1,000,000 | Offset 2,900,000 | +| --- | --- | --- | --- | +| `ORDER BY __row_order LIMIT 50 OFFSET k` | 100 ms | 332 ms | 374 ms | +| The same, with the file's order declared to the engine (`WITH ORDER`) | 25 ms | 102 ms | 242 ms | +| Range request, row-group statistics only | 90 ms | 90 ms | 79 ms | +| Range request, file written with a parquet page index | 19 ms | 24 ms | 23 ms | +| First draft's approach: bare `LIMIT/OFFSET`, single-partition connection | 21 ms | 90 ms | 249 ms | + +And the sorted case, at offset 100,000: `ORDER BY g` gave 6 distinct pages in 6 +requests (214 ms); `ORDER BY g, __row_order` gave 1 (302 ms). + +One consequence for the file format, recorded in ADR-009 decision D3: the +writer puts `__row_order` last. It also emits a parquet page index, which costs +nothing now and is what takes a later range request from 90 ms to 20 ms. + +Declaring the file's order to the engine removes the sort from the plan +(`GlobalLimitExec <- SortPreservingMergeExec <- DataSourceExec`, no +`SortExec`). The spike registers the file through +`CREATE EXTERNAL TABLE ... WITH ORDER`. Whether the declaration can travel +inside a xorq build is untested (open question 3), so it is an optimization +here and not part of the decision. + +*Rejected:* the first draft's decision, a second connection with +`target_partitions = 1` for page requests. It makes unsorted pages repeatable +(bottom row of the table) at the same cost as a declared order. It was +rejected because: + +- it depends on the engine's planner never introducing an exchange operator, + and `repartition_file_scans = false`, which looked sufficient in an earlier + experiment, returned the same wrong page 8 times out of 8 for a filtered plan + on a file with 100,000-row groups; +- it has to be reproduced inside Buckaroo's process; +- it does nothing for a user sort with ties; +- it fails for a union. + +An `ORDER BY` on a column with no ties is repeatable by the query's own +semantics, in any engine and any process. + +### D6. The exact name `__row_order` is reserved + +- A recipe may read the column and may copy it under another name (D3). A + recipe that assigns to `__row_order` is a build error, because arbitrary + values could contain ties or gaps, and D5 depends on `0..N-1` with neither. +- Only the exact name is special. `__row_order_v1`, or any other name an author + picks for a copy, is ordinary data and survives materialization. +- A join of two entries leaves the right side's copy behind under ibis's + collision name, `__row_order_right`, and a three-way join silently keeps only + the first two. That column is ordinary data too: it says where the row sat in + the right-hand parent. The writer replaces only `__row_order` itself. +- The primary-key search skips it. Nothing excludes `original_row_order` from + the candidates today (`primary_key.py:219`), and a column that is unique in + every table would win the search for any table without a string or id key. + Row positions shift between versions, so a diff keyed on it would be + meaningless. +- `build_compare_expr` drops it from both sides before joining. The diff is an + entry (ADR-007 decision D10) and gets its own when it is materialized. + +### D7. `tallyman_read_csv` loses its trailing `order_by`, and its column becomes `__row_order` + +Amends ADR-005. INV-2: `io.py:627` returns a plain read of the intermediate +parquet with no `order_by`. INV-1: the row-index column is named `__row_order` +and written last, so a CSV root has one row-order column and not two with +identical values. The root entry becomes a cheap read of the intermediate, and +D5 gives file-order pages with no Sort and no second copy. + +What INV-2 provided, and what replaces it: + +| INV-2 gave | Replacement | +| --- | --- | +| A canonical display order | D5. INV-2 did not deliver this above 10 MB. | +| A parquet boundary for chained children | A cheap root's graph is one read node. | +| A `result_digest` on the root, so a re-parse that produced different rows would be caught | Lost as it stands: cheap entries record no digest (ADR-006 decision D9, "no cheap-entry digests"). See open question 5. | + +Every hash in every CSV lineage changes, so this rides the corpus rebuild of +ADR-007 decision D9 ("one change, one rebuild"). + +### D8. Buckaroo's half is one hint and one Buckaroo issue + +Buckaroo pages in its own process, so the grid needs the same rule and tallyman +cannot apply it from outside. Tallyman passes the column's name in the +`/load_expr` payload as a hint. The Buckaroo issue asks that, given the hint, +Buckaroo sorts by it when the user has chosen no sort, appends it as the last +key of any user sort, and may use range requests for the unfiltered, unsorted +view. Without the hint Buckaroo behaves as it does now. Filed as +buckaroo-data/buckaroo#974. + +The issue reproduces the defect through Buckaroo's own page builder. +`_window_to_parquet` (`buckaroo/xorq_buckaroo.py`, lines 302-330 in 0.15.6) +sorts on a single key, `expr.order_by(expr[sort_col].asc())`, and applies no +`order_by` at all when the user has chosen no sort. Six identical calls for the +same 50 rows of a 27 MB file gave 5 different results unsorted and 6 sorted by +a column with 200 distinct values. + +### D9. Correct the threshold + +`repartition_file_min_size` is 10,485,760 in this engine. Four places say 1 MiB +and are corrected with this change: `plans/ADR-006-read-path-loads-builds.md:98`, +`plans/datafusion-scan-order-findings.md:64`, +`tests/test_parquet_digest_order_probe.py:5` (whose `> 1_048_576` size guard +does not establish a split scan and is not what makes that test meaningful; +its aggregate is), and `src/tallyman_xorq/source_cache.py:98`. +`tests/test_tallyman_read_csv.py:159` already says about 10 MB. + +## Consequences + +- Pages are repeatable for unsorted and sorted requests, in tallyman and in + Buckaroo, with no engine settings and no second connection. +- Every table shows one more column, at the end. A join result also shows + `__row_order_right` unless the recipe drops it. +- A recipe whose select list forgets `__row_order` fails to build until the + author adds it. +- CSV lineages stop writing sorted copies. With ADR-007, revisions of a CSV + entry are cheap reads over one intermediate file, and primary-key inheritance + applies to them. +- Unions, distincts and unnests are materialized, which is the price of having + a defined row order. +- Each parquet source costs one ordered copy, about the size of the source. +- An unsorted page costs a sort of one column unless the file's order is + declared (100 to 374 ms against 25 to 242 ms in the spike). A range request + costs about 20 ms at any depth. +- The grid stays unstable until Buckaroo's half (buckaroo-data/buckaroo#974) + lands: above 10 MB when unsorted, and at any size when sorted by a column + with ties. + +## Open questions + +1. **Ordered copies of parquet sources.** D2 adds a copy per parquet source. + Adopted as the uniform rule under Paddy's "cohesive first" priority, and not + yet confirmed by him in so many words. It also assumes polars numbers a + parquet scan's rows in file order, as ADR-004 measured for CSV, which needs + checking. +2. **Renaming `original_row_order`.** D7 replaces it with `__row_order`. + Adopted on the same basis, and also not yet confirmed. The alternative keeps it as a data column meaning "line of + the source file", at the cost of two identical columns on every CSV root. +3. **Declaring a file's order inside a xorq build.** It works through DDL on a + connection. If it can ride in a build's read node, Buckaroo's unsorted pages + get the cheaper plan too. +4. **Deep offsets on a cheap entry.** Positions in a filtered view have gaps, + so it pages with `OFFSET`, whose cost grows with depth. +5. **A digest for an ordered copy.** An ordered copy is the record of a parse + and is re-created from the clone if deleted. Recording its digest in the root + entry's manifest, and verifying it on re-creation, would restore what D7 + gives up. It wants ADR-009's digest definition, and it touches where the + copies live: `csv_ordered` is global, is never collected, and is not packed. diff --git a/plans/ADR-009-digest-stability.md b/plans/ADR-009-digest-stability.md new file mode 100644 index 0000000..5f9763c --- /dev/null +++ b/plans/ADR-009-digest-stability.md @@ -0,0 +1,268 @@ +# ADR: Digest stability (a heal is flagged only when the result changed) + +- **Status:** Proposed (2026-09-18, revised 2026-09-20: D3 gains two format + requirements from `plans/ADR-008-row-order-of-reads.md`, and D1 lost its + speed gate). Amends + `plans/ADR-004-result-digest-canonical-ordering.md` (Option A's "hash the + snapshot bytes") and decision D5 of + `plans/ADR-006-read-path-loads-builds.md` (the canonical sort), which said + "`result_digest` keeps its file-hash definition". The canonical sort itself + is unchanged and is still required. +- **Reading decision labels:** a bare label such as "D2" in this document + always means this ADR's own decision. Another ADR's decision is always + written with its ADR number and a few words saying what it decides. +- **Context:** the 2026-09-18 cache audit (tallyman @ `a748ea6`, xorq 0.3.26, + xorq-datafusion 0.2.7, pyarrow 21.0.0). No ticket filed yet. +- **Affected code:** `src/tallyman_xorq/result_cache.py` + (`snapshot_file_digest`, `verify_result_faithful`, `_verify_self_heal`), + `src/tallyman_xorq/build.py` (the execute-once step), + `src/tallyman_xorq/backend.py` (a single-partition connection), + `src/tallyman_core/manifest.py` (`result_digest`), and the `materialize` + writer that decision D4 of `plans/ADR-007-tallyman-owned-materialization.md` + introduces (one writer for snapshots, used by the build and by every heal). + D2 and D3 assume that writer. If ADR-007 were rejected, D1 would stand as + written and D2 would need restating against xorq's writer. +- **Related ADRs:** `plans/ADR-008-row-order-of-reads.md` (uses the same + single-partition setting for a different job, and has an open question this + digest would answer). +- **Evidence:** `scripts/spike_float_aggregate_digest.py`, + `scripts/spike_logical_digest.py`. + +## Terms + +- **Materialize:** run an entry's computation once and write the result to a + parquet file. That file is the entry's **snapshot**. +- **Worthy entry:** an entry tallyman materializes. A cheap entry is one it + does not, and it records no digest. +- **`result_digest`:** the value recorded in a worthy entry's manifest when its + snapshot is first written, against which every later rewrite is checked. +- **Heal:** re-create a snapshot that is missing from disk by re-running the + entry's build. +- **Unfaithful heal:** a heal whose digest does not match the recorded one. +- **Canonical sort:** the fixed total order in which a snapshot's rows are + written (the author's `order_by` keys, then an inherited `__row_order`, then + the remaining columns), from ADR-006 decision D5. +- **Partition:** one of the parallel streams DataFusion splits a query into. + `target_partitions = 1` runs a query as a single stream. + +## Problem + +`result_digest` is the SHA-256 of the snapshot file's bytes. The contract gives +it one job: witnessing that a later rematerialization reproduced the original. +When a heal's digest does not match, tallyman writes a durable +`unfaithful_heal` record, pushes an SSE event, wipes the entry's Buckaroo stat +cache, evicts its session, and pins and badges the entry (ADR-006 decisions D7, +loud verification; D10, the Buckaroo-state wipe; and D12, the pin and badge). +That response is right for a recipe that calls `sample()`. It is +expensive, sticky and misleading when nothing about the result changed, and +today two things trigger it without a change in the result. + +**1. A float aggregate is not bit-reproducible under parallel execution.** +DataFusion sums each partition separately and merges the partial sums in +arrival order. Float addition is not associative, so the low bits move from +run to run. A group-by over 3,000,000 rows with a float `SUM` and `AVG`, +canonically ordered, six runs per configuration: + +| Aggregate | Partitions | Distinct digests in 6 runs | Median | +| --- | --- | --- | --- | +| float | default (14) | 6 | 6 ms | +| float | `target_partitions = 1` | 1 | 20 ms | +| integer | default (14) | 1 | 7 ms | +| integer | `target_partitions = 1` | 1 | 31 ms | + +The largest relative difference between two float runs was 6.4e-16. At the +tallyman level the audit healed a float-aggregate entry six times: six +different files, six `unfaithful_heal` records, six stat-cache wipes. An +integer-only aggregate healed byte-identical six times. The canonical sort +cannot help, since the rows are in the same order and it is the values that +differ. Aggregate is the usual reason an entry is worthy, so most expensive +entries with a float measure are flagged after any eviction. The message +blames "execution (#83), a fixed graph that runs differently each execute", +which sends the author to a recipe that is deterministic. The default partition +count is also the machine's core count, so the same build merges differently +on a different machine. + +**2. File bytes depend on things that are not the result.** + +- How the stream was batched. The same rows handed to a parquet writer as + 100,000-row batches and as 8,192-row batches gave different files unless each + row group was first combined into contiguous arrays. +- The writer's version. The footer carries + `created_by = 'parquet-cpp-arrow version 21.0.0'`, so after a pyarrow upgrade + every heal would mismatch. +- Under xorq's writer today, whether the cache node was the root of the + executed expression (1,234 bytes against 858 for the same rows). + +ADR-004 accepted "a library upgrade changes the hash; rebuild is fine". That +was written before a mismatch became loud. With those three ADR-006 decisions +in place, an +upgrade would pin and badge every entry that gets evicted until the corpus is +rebuilt. + +## Decisions + +### D1. Materialization runs single-partition + +`materialize` (ADR-007 decision D4, the one writer of snapshots) executes on a +connection configured with +`SET datafusion.execution.target_partitions = 1`. The build and every heal use +it, so both run one plan with one merge order, on any machine. It is a +different connection object from ADR-008's window connection, with the same +setting, because a long materialization must not share a context with page +reads. + +Cost: about 3x on the spike's aggregate. ADR-004 measured a 3.1M-group +aggregate at 0.5 s parallel against 3.4 to 3.9 s single-partition, and a full +43-column read at 2.5 s against 6.9 s, on the 11.8M-row parking file. A +materialization runs once per entry and once per heal, never on a read. + +ADR-004 rejected pinning scan order through session config as the *ingest* +lever, because it serializes everything downstream. This decision uses the same +setting for a different job and accepts that cost knowingly: reproducible +arithmetic is the point, and no setting gives both. + +Paddy's call in the grilling session (2026-09-20): "I want a cohesive system +that works reliably, then we can worry about speed problems as they come up." +So every materialization runs single-partition, and this decision carries no +speed gate. If the cost becomes a problem, the known variant is to run +single-partition only for a plan with a floating-point reduction or window, +decided once at build from the expression and recorded in the manifest so that +a heal never re-derives it. + +*Rejected:* round floats before hashing. A value next to a rounding boundary +flips under one unit of noise in the last place, and among millions of values +some are. +*Rejected:* compare with a tolerance. When a heal runs the original values are +gone and only the digest is left. +*Rejected:* treat a mismatch as advisory when the plan has a float aggregate. +That removes verification from the entries most likely to be expensive. +*Rejected:* drop the canonical sort, since single-partition output is ordered +anyway. The sort is what puts the author's keys first in the order the rows +are numbered (`__row_order`, ADR-008 decision D2), and it keeps the result independent of the hash +table's emission order, which an engine upgrade can change. + +### D2. `result_digest` is a digest of the snapshot's content, computed from the file as read back + +The digest is a SHA-256 over the snapshot's ordered Arrow data: + +- one hash stream per column for validity (one byte per row), one for lengths + (variable-width types) and one for values, with null slots zeroed or emptied, + since a null slot may hold anything; +- each stream seeded with the column name and its logical type, where `string`, + `large_string` and `string_view` are one type; +- the streams combined in schema order together with the row count; +- stored with an algorithm prefix (`arrow-sha256:`) so a future definition can + never be compared against this one by accident. + +Separate streams matter. The first version of the spike fed validity and +values into one hasher per column, and the digest then depended on where batch +boundaries fell. + +One function computes it, from the written file read back, for both the build +and verify. This is the "one derivation route" rule behind ADR-006 decision D8 +(the snapshot-key tripwire: the build and the read share one derivation). The +spike shows why it cannot be computed from the stream handed to the writer: the +writer coerced a `timestamp[s]` column to `timestamp[ms]`, and the two digests +differed. + +Measured on 3,000,000 rows with nulls, NaNs, strings, booleans and timestamps: + +| Check | Result | +| --- | --- | +| Same rows as 8,192-row, 100,000-row and single batches | 1 digest | +| Same file read back in 1,000-row batches | same | +| Same rows written Snappy with 8,192-row groups instead of zstd with 1,048,576 | same | +| Same file read through DataFusion with `order_by(id)` | same | +| One value changed | differs | +| One null replaced by `0.0` | differs | +| First two rows swapped | differs | +| Read back and hash a 57 MB file (127 MB of Arrow data) | 0.12 s, against 0.02 s for a file-bytes hash | + +The digest stays order-sensitive, so the canonical sort is still what makes it +reproducible. `__row_order` is a column of the file like any other, so the +digest covers it. + +This reverses part of the reasoning in ADR-006 decision D5 (the canonical +sort). That decision rejected the multiset digest +partly because "verify must read every row and the digest stops being a hash +of the artifact". Both are true of this digest as well. They are accepted here +because verify runs only on a heal and in the sweep, at roughly 1 GB/s of Arrow +data, and because being a hash of the artifact is exactly what turns a writer +upgrade into a false alarm. + +*Rejected:* keep file bytes and pin the writer's settings. It is reproducible +today (the spike gets one digest across batch sizes once each row group is +combined) and six times cheaper to verify. It also freezes the codec and +row-group size for the life of the corpus, and it fails on the first pyarrow +upgrade. +*Rejected:* polars `hash_rows`. Its documentation does not guarantee stable +results across polars versions. + +### D3. The snapshot's format + +Tallyman's writer (ADR-007 decision D4) regroups the record-batch stream into +row groups of 1,048,576 rows, combines each row group into contiguous arrays, +and writes zstd level 3, parquet format 2.6, statistics on. For the spike's rows that is +57.1 MB, 3 row groups and a 2.7 KB footer, against 100.3 MB, 367 row groups +and a 228.5 KB footer in the shape xorq writes (one Snappy row group per +8,192-row batch). Memory is bounded by one row group. + +ADR-008 adds two requirements. The writer numbers the rows in a last column +named `__row_order` (ADR-008 decision D2). And it writes a parquet page index, +which is what lets a page be fetched as a range of `__row_order` values without +decoding a whole row group: 19 to 24 ms at any depth with the index against 79 +to 90 ms without it, on a 287 MB file (`scripts/spike_row_order_paging.py` on +the ADR-008 branch). + +Combining each row group keeps the file bytes reproducible as well. Nothing +depends on that after D2, and it means two writes of the same entry can still +be compared with `cmp` when debugging. Because of D2 these settings can change later without +touching any digest. + +### D4. A mismatch record names its likely cause + +With D1 and D2 in place the causes left are the recipe's own nondeterminism +(`sample()`, `now()`, an impure UDF), source drift under `off` identity mode, +and an engine upgrade that changed results. The manifest records the xorq, +xorq-datafusion and pyarrow versions at build, and the `unfaithful_heal` record +carries them alongside the versions at heal. When they differ, the message says +so instead of blaming the recipe. The contract's attribution table gains that +row. + +### D5. It lands with the rebuild + +Every worthy entry's digest is recomputed by the corpus rebuild of ADR-007 +decision D9 (one change, one rebuild). +The manifest field keeps its name. Tests that cannot fail first ride with the +fix; the float-aggregate heal test and a batch-boundary digest test fail on +`main` and belong in the failing-tests commit. + +## Consequences + +- A float-aggregate entry heals to the same digest, and a pyarrow upgrade no + longer flags anything. The loud responses of ADR-006 decisions D10 and D12 + are left for the causes they were designed for. +- Materializations and heals are slower, by about 3x on aggregation at spike scale and up + to 7x in ADR-004's parking measurement. Reads are unaffected. +- Verify decodes the file instead of hashing its bytes. It runs on a heal and + in `catalog_scan_staleness(verify_results=True)`, never on a read. +- Snapshots are smaller, and their footers were 85 times smaller in the spike, + which matters to every page request that opens one. +- `docs/system-contract.md` changes in "Result digest" and in the manifest + table ("SHA-256 of the baked result snapshot"), and its verification table + gains the engine-change row. +- The same function can digest the CSV intermediate (ADR-008, open + question 1). + +## Open questions + +1. **Nested types.** The spike covers fixed-width, boolean, string and binary + columns. Lists, structs and maps need a recursive definition. +2. **Types parquet cannot store as given.** A `timestamp[s]` column comes back + as `timestamp[ms]`, so the snapshot's schema differs from the entry's + recorded schema. Either the writer refuses such a column, or the entry's + schema is recorded from the snapshot. `__row_order` pushes toward the second + answer, since the writer adds a column the entry's graph does not have. +3. **Engine upgrades.** Single-partition execution fixes the merge order within + one engine version. Nothing guarantees float results across versions. D4 + attributes that case and the remedy stays a rebuild. diff --git a/scripts/spike_bare_read_chaining.py b/scripts/spike_bare_read_chaining.py new file mode 100644 index 0000000..cf5044e --- /dev/null +++ b/scripts/spike_bare_read_chaining.py @@ -0,0 +1,117 @@ +"""ADR-007 evidence: chaining through a bare read of the parent's snapshot, with no xorq cache node anywhere. + +Raw xorq plus tallyman's ``classify_build``; no tallyman build machinery. A worthy parent (an aggregate, canonically +sorted) is built with no cache node and materialized by a plain parquet write to ``/.parquet``. +A child reads that path with ``deferred_read_parquet`` and adds a filter and a computed column. + +Questions, in the order printed: + +1. What does the child's build hash depend on: the snapshot's path string, or its bytes/mtime? +2. Can a child be built while the parent's snapshot is absent? +3. Does ``load_expr`` of the child's build need the snapshot? What does executing without it raise? +4. After the parent is re-materialized from its own build, does the child execute to the right answer? +5. Is the child classified cheap? (Under today's inlined chaining the same child is worthy.) +6. Does the child's ``expr.yaml`` carry the literal path, so the portable-path placeholder applies to it? +7. Did anything get written under xorq's global cache directory? + + uv run python scripts/spike_bare_read_chaining.py +""" + +from __future__ import annotations + +import hashlib +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_bare_read_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import numpy as np # noqa: E402 +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 +from xorq.ibis_yaml.compiler import build_expr, load_expr # noqa: E402 + +from tallyman_xorq.result_cache import classify_build # noqa: E402 + +N = 400_000 +BUILDS = HOME / "builds" +SNAPSHOTS = HOME / "compute_cache" / "result_cache" + + +def materialize(expr, dest: Path) -> str: + tmp = dest.with_name(f".{dest.name}.{os.getpid()}.tmp") + pq.write_table(expr.to_pyarrow(), tmp, compression="zstd") + os.replace(tmp, dest) + return hashlib.sha256(dest.read_bytes()).hexdigest()[:16] + + +def child_of(snapshot: Path, schema=None): + parent = xo.deferred_read_parquet(str(snapshot), schema=schema) + return parent.filter(parent.n > 10).mutate(r=parent.s / parent.n) + + +def attempt(fn) -> str: + try: + return str(fn()) + except Exception as exc: # noqa: BLE001 - the spike reports whatever is raised + return f"raises {type(exc).__name__}: {str(exc)[:90]}" + + +def main() -> None: + SNAPSHOTS.mkdir(parents=True) + source = HOME / "cas" / "deadbeef.parquet" + source.parent.mkdir() + rng = np.random.default_rng(7) + pq.write_table( + pa.table({"id": np.arange(N), "g": rng.integers(0, 20_000, N), "v": rng.integers(0, 1000, N)}), source + ) + + t = xo.deferred_read_parquet(str(source)) + parent = t.group_by("g").agg(n=t.count(), s=t.v.sum()).order_by(["g", "n", "s"]) + parent_build = Path(build_expr(parent, builds_dir=BUILDS)) + snapshot = SNAPSHOTS / f"{parent_build.name}.parquet" + first_digest = materialize(load_expr(parent_build), snapshot) + print(f"parent {parent_build.name}: {classify_build(parent_build)}, snapshot digest {first_digest}\n") + + same_path = Path(build_expr(child_of(snapshot), builds_dir=BUILDS)).name + original = snapshot.read_bytes() + pq.write_table(pa.table({"g": [1, 2], "n": [99, 98], "s": [5, 6]}), snapshot) # other rows, other size and mtime + other_bytes = Path(build_expr(child_of(snapshot), builds_dir=BUILDS)).name + elsewhere = SNAPSHOTS / "0123456789ab.parquet" + elsewhere.write_bytes(original) + other_path = Path(build_expr(child_of(elsewhere), builds_dir=BUILDS)).name + print(f"1. child hash: {same_path}") + print(f" same path with other bytes: {other_bytes}; same bytes at another path: {other_path}") + + snapshot.unlink() + print(f"2. compose with snapshot absent, no schema: {attempt(lambda: child_of(snapshot).schema().names)}") + with_schema = attempt(lambda: build_expr(child_of(snapshot, parent.schema()), builds_dir=BUILDS)) + print(f" build with snapshot absent, schema given: {with_schema}") + + child_build = BUILDS / same_path + print(f"3. load_expr(child) with snapshot absent: {attempt(lambda: type(load_expr(child_build)).__name__)}") + print(f" execute with snapshot absent: {attempt(lambda: load_expr(child_build).count().execute())}") + + again = materialize(load_expr(parent_build), snapshot) + rows = int(load_expr(child_build).count().execute()) + want = int(parent.filter(parent.n > 10).count().execute()) + print(f"4. parent re-materialized from its build: digest unchanged={again == first_digest}") + print(f" child rows {rows}, expected {want}") + + inlined = Path(build_expr(parent.filter(parent.n > 10).mutate(r=parent.s / parent.n), builds_dir=BUILDS)) + print(f"5. classify_build(bare-read child) = {classify_build(child_build)}") + print(f" classify_build(same child, parent graph inlined) = {classify_build(inlined)}") + + yaml_text = (child_build / "expr.yaml").read_text() + inlined_size = len((inlined / "expr.yaml").read_text()) + print(f"6. literal snapshot path in child expr.yaml: {str(snapshot) in yaml_text}") + print(f" child expr.yaml is {len(yaml_text):,} bytes; with the parent graph inlined it is {inlined_size:,}") + + leaked = sorted(str(f) for f in (HOME / "_global_xorq").rglob("*.parquet")) + print(f"7. parquet files under XORQ_CACHE_DIR: {leaked or 'none'}") + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_float_aggregate_digest.py b/scripts/spike_float_aggregate_digest.py new file mode 100644 index 0000000..55f2c67 --- /dev/null +++ b/scripts/spike_float_aggregate_digest.py @@ -0,0 +1,77 @@ +"""ADR-009 evidence: a float aggregate is not bit-reproducible under DataFusion's parallel partial aggregation. + +Runs the same canonically ordered group-by six times per configuration and hashes the result's Arrow buffers. +A float ``SUM``/``AVG`` comes back with different low bits from run to run under the default partition count, and +with identical bits under ``target_partitions = 1``. An integer-only aggregate is the control: it is identical in +both configurations. Median wall time is printed so the cost of single-partition execution is visible. + + uv run python scripts/spike_float_aggregate_digest.py +""" + +from __future__ import annotations + +import hashlib +import statistics +import tempfile +import time +from pathlib import Path + +import numpy as np +import pyarrow as pa +import pyarrow.parquet as pq +import xorq.api as xo + +N = 3_000_000 +RUNS = 6 + + +def value_digest(table: pa.Table) -> str: + h = hashlib.sha256() + for col in table.columns: + for chunk in col.chunks: + for buf in chunk.buffers(): + if buf is not None: + h.update(buf) + return h.hexdigest()[:12] + + +def run(path: Path, partitions: int | None, kind: str) -> tuple[str, float, pa.Table]: + con = xo.connect() + if partitions is not None: + con.raw_sql(f"SET datafusion.execution.target_partitions = {partitions}") + t = con.read_parquet(str(path)) + if kind == "float": + expr = t.group_by("g").agg(n=t.count(), s=t.v.sum(), m=t.v.mean()).order_by("g") + else: + expr = t.group_by("g").agg(n=t.count(), s=t.k.sum(), hi=t.k.max()).order_by("g") + t0 = time.perf_counter() + table = expr.to_pyarrow() + return value_digest(table), time.perf_counter() - t0, table + + +def main() -> None: + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "t.parquet" + rng = np.random.default_rng(5) + source = pa.table( + {"g": rng.integers(0, 1_000, N), "v": rng.random(N) * 1e6, "k": rng.integers(0, 1_000_000, N)} + ) + pq.write_table(source, path, row_group_size=100_000) + groups = pq.ParquetFile(path).metadata.num_row_groups + print(f"source: {N:,} rows, {groups} row groups, {path.stat().st_size / 1e6:.1f} MB\n") + for kind in ("float", "int"): + for label, partitions in (("default partitions", None), ("target_partitions=1", 1)): + results = [run(path, partitions, kind) for _ in range(RUNS)] + digests = [d for d, _, _ in results] + median_ms = statistics.median(s for _, s, _ in results) * 1000 + distinct = len(set(digests)) + line = f"{kind:5s} {label:20s} distinct digests over {RUNS} runs: {distinct}, median {median_ms:.0f} ms" + if kind == "float" and len(set(digests)) > 1: + a, b = results[0][2].column("s").to_numpy(), results[1][2].column("s").to_numpy() + rel = np.max(np.abs(a - b) / np.abs(a)) + line += f", max relative difference between two runs {rel:.1e}" + print(line) + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_logical_digest.py b/scripts/spike_logical_digest.py new file mode 100644 index 0000000..7a695d1 --- /dev/null +++ b/scripts/spike_logical_digest.py @@ -0,0 +1,229 @@ +"""ADR-009 evidence: two candidate definitions of ``result_digest`` for a tallyman-written snapshot. + +**File bytes** (today's definition). Reproducible only if the writer's input and settings are pinned: the same rows +arriving in differently sized record batches must be regrouped into fixed row groups, and each row group combined +into contiguous arrays before it is written. The parquet footer also embeds the writer's version string, so the +bytes move on every pyarrow upgrade. + +**Logical content**. A SHA-256 over the ordered Arrow data, one hasher per column, fed a normalized form that does +not depend on batch boundaries, on the physical string type, or on whatever sits in null slots. It is independent +of codec, row-group size and writer version, and costs a read-back to verify. + +The script checks both for the invariances each needs, compares the file shape of a tallyman-side writer with +the shape xorq's ``ParquetStorage`` writes, then times the two digests. + + uv run python scripts/spike_logical_digest.py +""" + +from __future__ import annotations + +import hashlib +import tempfile +import time +from pathlib import Path + +import numpy as np +import pyarrow as pa +import pyarrow.compute as pc +import pyarrow.parquet as pq +import xorq.api as xo + +N = 3_000_000 +ROW_GROUP = 1_048_576 + + +def _logical_type(dtype: pa.DataType) -> str: + if pa.types.is_string(dtype) or pa.types.is_large_string(dtype) or pa.types.is_string_view(dtype): + return "string" + if pa.types.is_binary(dtype) or pa.types.is_large_binary(dtype) or pa.types.is_binary_view(dtype): + return "binary" + return str(dtype) + + +class LogicalDigest: + """Order-sensitive digest of a record-batch stream, invariant to chunking and physical encoding.""" + + # Each column keeps one hasher per byte stream (validity, lengths, values). A single hasher per column would + # interleave the streams chunk by chunk, and the digest would then depend on where the batch boundaries fall. + STREAMS = ("validity", "lengths", "values") + + def __init__(self, schema: pa.Schema): + self.schema = schema + self.hashers = [ + {s: hashlib.sha256(f"{f.name}\x00{_logical_type(f.type)}\x00{s}".encode()) for s in self.STREAMS} + for f in schema + ] + self.rows = 0 + + def update(self, batch: pa.RecordBatch | pa.Table) -> None: + for hashers, column in zip(self.hashers, batch.columns): + chunks = column.chunks if isinstance(column, pa.ChunkedArray) else [column] + for arr in chunks: + self._update_array(hashers, arr) + self.rows += batch.num_rows + + @staticmethod + def _update_array(hashers: dict, arr: pa.Array) -> None: + if pa.types.is_dictionary(arr.type): + arr = arr.dictionary_decode() + nulls = arr.is_null().to_numpy(zero_copy_only=False) + hashers["validity"].update(nulls.astype(np.uint8).tobytes()) # one byte per row, null or not + dtype = arr.type + if _logical_type(dtype) in ("string", "binary"): + target = pa.large_string() if _logical_type(dtype) == "string" else pa.large_binary() + filled = pc.fill_null(arr.cast(target), "" if target == pa.large_string() else b"") + hashers["lengths"].update( + pc.binary_length(filled).to_numpy(zero_copy_only=False).astype(np.int64).tobytes() + ) + offsets = np.frombuffer(filled.buffers()[1], dtype=np.int64)[ + filled.offset : filled.offset + len(filled) + 1 + ] + if len(filled): + hashers["values"].update(memoryview(filled.buffers()[2])[int(offsets[0]) : int(offsets[-1])]) + elif pa.types.is_boolean(dtype): + hashers["values"].update(pc.fill_null(arr, False).to_numpy(zero_copy_only=False).astype(np.uint8).tobytes()) + elif pa.types.is_primitive(dtype) or pa.types.is_decimal(dtype) or pa.types.is_fixed_size_binary(dtype): + width = dtype.bit_width // 8 + raw = np.frombuffer(arr.buffers()[1], dtype=np.uint8)[arr.offset * width : (arr.offset + len(arr)) * width] + if arr.null_count: + raw = raw.reshape(len(arr), width).copy() + raw[nulls] = 0 # null slots may hold anything; zero them + hashers["values"].update(raw.tobytes() if arr.null_count else memoryview(raw)) + else: + raise NotImplementedError(f"nested/other type not covered by the spike: {dtype}") + + def hexdigest(self) -> str: + top = hashlib.sha256(str(self.rows).encode()) + for hashers in self.hashers: + for stream in self.STREAMS: + top.update(hashers[stream].digest()) + return top.hexdigest()[:16] + + +def logical_digest(batches, schema: pa.Schema) -> str: + d = LogicalDigest(schema) + for batch in batches: + d.update(batch) + return d.hexdigest() + + +def write_pinned(batches, schema: pa.Schema, dest: Path, combine: bool = True, **settings) -> str: + """Regroup a batch stream into fixed row groups and write it; return the digest of the file's bytes.""" + options = {"compression": "zstd", "compression_level": 3, "version": "2.6", "data_page_version": "1.0"} | settings + with pq.ParquetWriter(dest, schema, **options) as writer: + pending, rows = [], 0 + for batch in batches: + pending.append(batch) + rows += batch.num_rows + while rows >= ROW_GROUP: + table = pa.Table.from_batches(pending) + head, tail = table.slice(0, ROW_GROUP), table.slice(ROW_GROUP) + writer.write_table(head.combine_chunks() if combine else head, row_group_size=ROW_GROUP) + pending, rows = tail.to_batches(), tail.num_rows + if rows: + table = pa.Table.from_batches(pending) + writer.write_table(table.combine_chunks() if combine else table, row_group_size=ROW_GROUP) + return hashlib.sha256(dest.read_bytes()).hexdigest()[:16] + + +def make_table() -> pa.Table: + rng = np.random.default_rng(11) + floats = rng.random(N) + floats[rng.integers(0, N, 1000)] = np.nan + ints = rng.integers(0, 1_000_000, N) + return pa.table( + { + "id": pa.array(np.arange(N)), + "g": pa.array(ints), + "f": pa.array(floats, mask=rng.random(N) < 0.01), + "s": pa.array([f"k{v % 5000:05d}" for v in ints], mask=rng.random(N) < 0.01), + "b": pa.array(ints % 2 == 0), + "ts": pa.array(ints.astype("datetime64[s]")), + } + ) + + +def main() -> None: + table = make_table() + schema = table.schema + print(f"table: {N:,} rows, {table.nbytes / 1e6:.0f} MB of Arrow data, pyarrow {pa.__version__}\n") + with tempfile.TemporaryDirectory() as tmp: + tmp = Path(tmp) + + print("file bytes, the same rows arriving in different batch sizes:") + for combine in (True, False): + digests = { + size: write_pinned( + table.to_batches(max_chunksize=size), schema, tmp / f"c{int(combine)}_{size}.parquet", combine + ) + for size in (8_192, 100_000, N) + } + distinct = sorted(set(digests.values())) + print(f" combine_chunks per row group={combine!s:5}: {len(distinct)} distinct digest(s) {distinct}") + created_by = pq.ParquetFile(tmp / "c1_8192.parquet").metadata.created_by + print(f" footer created_by = {created_by!r} (moves with every writer upgrade)\n") + + print("file shape, the same rows:") + xorq_shape = tmp / "xorq_shape.parquet" + with pq.ParquetWriter(xorq_shape, schema, compression="snappy") as writer: + for batch in table.to_batches(max_chunksize=8_192): + writer.write_batch(batch) # one row group per batch, as xorq's ParquetStorage writes them + for label, path in (("zstd, 1,048,576-row groups", tmp / "c1_8192.parquet"), ("xorq's shape", xorq_shape)): + md = pq.ParquetFile(path).metadata + size = path.stat().st_size / 1e6 + groups, footer = md.num_row_groups, md.serialized_size / 1e3 + print(f" {label:27s}: {size:5.1f} MB, {groups:3d} row groups, {footer:6.1f} KB footer") + print() + + def of_file(path: Path, batch_size: int = 65_536) -> str: + pf = pq.ParquetFile(path) + return logical_digest(pf.iter_batches(batch_size=batch_size), pf.schema_arrow) + + print("logical content:") + by_batch = {size: logical_digest(table.to_batches(max_chunksize=size), schema) for size in (8_192, 100_000, N)} + distinct = len(set(by_batch.values())) + print(f" in-memory stream, batch sizes 8,192 / 100,000 / one table: {distinct} distinct digest(s)") + + zstd_path, snappy_path = tmp / "c1_8192.parquet", tmp / "snappy_small_groups.parquet" + pq.write_table(table, snappy_path, compression="snappy", row_group_size=8_192) + reference = of_file(zstd_path) + stored = pq.ParquetFile(zstd_path).schema_arrow + coerced = [f"{a.name}: {a.type} -> {b.type}" for a, b in zip(schema, stored) if a.type != b.type] + print(f" stream handed to the writer vs the file read back: same={by_batch[8_192] == reference}") + print(f" (the writer coerced {coerced})") + print(f" same file, read in 1,000-row batches: same={of_file(zstd_path, 1_000) == reference}") + print(f" same rows written snappy with 8,192-row groups: same={of_file(snappy_path) == reference}") + engine = xo.connect().read_parquet(str(zstd_path)).order_by("id").to_pyarrow_batches() + print(f" same file through DataFusion (read, order_by id): same={logical_digest(engine, stored) == reference}") + + def rewritten(changed: pa.Table) -> str: + path = tmp / "changed.parquet" + pq.write_table(changed, path, compression="zstd", row_group_size=ROW_GROUP) + return of_file(path) + + flipped = table.set_column(1, "g", pc.add(table["g"], pa.array(np.eye(1, N, 12345, dtype=np.int64)[0]))) + print(f" one value changed: differs={rewritten(flipped) != reference}") + f = table["f"].combine_chunks() + first_null = int(np.flatnonzero(f.is_null().to_numpy(zero_copy_only=False))[0]) + as_zero = pa.concat_arrays([f.slice(0, first_null), pa.array([0.0]), f.slice(first_null + 1)]) + print(f" one null replaced by 0.0: differs={rewritten(table.set_column(2, 'f', as_zero)) != reference}") + swapped = table.take(pa.array(np.r_[1, 0, np.arange(2, N)])) + print(f" first two rows swapped: differs={rewritten(swapped) != reference}\n") + + print("cost:") + t0 = time.perf_counter() + logical_digest(table.to_batches(max_chunksize=8_192), schema) + secs = time.perf_counter() - t0 + print(f" logical digest of an in-memory stream: {secs:.2f} s ({table.nbytes / 1e6 / secs:.0f} MB/s)") + t0 = time.perf_counter() + of_file(zstd_path) + secs = time.perf_counter() - t0 + print(f" logical digest of the file (read back + hash; the build and the verify path): {secs:.2f} s") + t0 = time.perf_counter() + hashlib.sha256(zstd_path.read_bytes()).hexdigest() + secs = time.perf_counter() - t0 + print(f" file-bytes digest to verify ({zstd_path.stat().st_size / 1e6:.0f} MB file): {secs:.2f} s") + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_row_order_paging.py b/scripts/spike_row_order_paging.py new file mode 100644 index 0000000..115e28c --- /dev/null +++ b/scripts/spike_row_order_paging.py @@ -0,0 +1,159 @@ +"""ADR-008 evidence: paging by a baked ``__row_order`` column. + +``__row_order`` holds 0..N-1 in the physical row order of a file tallyman wrote. The script answers, on the +DEFAULT parallel connection with no engine settings: + +1. Is ``ORDER BY __row_order LIMIT n OFFSET k`` repeatable and correct, and what does it cost? +2. Does declaring the file's order to the engine (``WITH ORDER``) remove the sort? +3. Does a user sort on a column with ties page repeatably, without and with ``__row_order`` as the last key? +4. What does a range request (``__row_order >= k AND __row_order < k + n``) cost at depth, without and with a + parquet page index in the file? +5. Could tallyman alter a cheap recipe so the column survives a ``select`` that does not name it? A ``select`` is + an allow-list, so an unnamed column is dropped whether or not anyone can see it. ``carry_row_order`` below is + the rewrite. It works, and ADR-008 D3 rejects it in favour of a build error. +6. What do joins and a debugging copy do with the column's name? + +For reference it also times the first draft's approach, a bare LIMIT/OFFSET on a single-partition connection. + + uv run python scripts/spike_row_order_paging.py +""" + +from __future__ import annotations + +import hashlib +import statistics +import tempfile +import time +from pathlib import Path + +import numpy as np +import pandas as pd +import pyarrow as pa +import pyarrow.parquet as pq +import xorq.api as xo +import xorq.vendor.ibis.expr.operations as ops +from xorq.common.utils.graph_utils import replace_nodes + +ROW = "__row_order" +N = 3_000_000 +REQUESTS = 6 +OFFSETS = (0, 1_000_000, 2_900_000) + + +def carry_row_order(expr): + """Rewrite a row-preserving expression so ``__row_order`` reaches the output as the last column.""" + + def replacer(node, kwargs): + if kwargs: + node = node.__recreate__(kwargs) + if isinstance(node, ops.Project) and ROW in node.parent.schema and ROW not in node.values: + return ops.Project(node.parent, {**node.values, ROW: ops.Field(node.parent, ROW)}) + if isinstance(node, ops.DropColumns) and ROW in node.columns_to_drop: + keep = frozenset(c for c in node.columns_to_drop if c != ROW) + return ops.DropColumns(node.parent, keep) if keep else node.parent + return node + + out = replace_nodes(replacer, expr).to_expr() + if ROW in out.columns and out.columns[-1] != ROW: + out = out.select([c for c in out.columns if c != ROW] + [ROW]) + return out + + +def measure(expr, first_col: str = ROW) -> tuple[int, list[int], float]: + pages, firsts, secs = set(), set(), [] + for _ in range(REQUESTS): + t0 = time.perf_counter() + df = expr.execute() + secs.append(time.perf_counter() - t0) + pages.add(hashlib.md5(df.to_csv(index=False).encode()).hexdigest()) + firsts.add(int(df[first_col].iloc[0])) + return len(pages), sorted(firsts)[:3], statistics.median(secs) * 1000 + + +def report(label: str, expr, want: int) -> None: + n, firsts, ms = measure(expr) + print(f" {label}: {n} distinct pages / {REQUESTS}, first {ROW} {firsts} (want {want}), median {ms:.0f} ms") + + +def operators(con, expr) -> str: + plan = con.raw_sql("EXPLAIN " + xo.to_sql(expr)).to_pandas() + physical = plan[plan.iloc[:, 0] == "physical_plan"].iloc[0, 1] + return " <- ".join(line.strip().split(":")[0] for line in physical.splitlines() if line.strip()) + + +def main() -> None: + rng = np.random.default_rng(9) + cols = {"g": rng.integers(0, 200, N)} + cols |= {f"v{i}": rng.random(N) for i in range(12)} + cols[ROW] = np.arange(N) + table = pa.table(cols) + + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "snapshot.parquet" + pq.write_table(table, path, compression="zstd", row_group_size=1_048_576) + size = path.stat().st_size / 1e6 + print(f"file: {size:.0f} MB, {N:,} rows x {table.num_columns} columns, written in {ROW} order\n") + + con = xo.connect() + t = con.read_parquet(str(path)) + + print(f"1. default connection, ORDER BY {ROW} LIMIT 50 OFFSET k") + for k in OFFSETS: + report(f"offset {k:>9,}", t.order_by(ROW).limit(50, offset=k), k) + print(" operators:", operators(con, t.order_by(ROW).limit(50, offset=1_000_000))) + + print("\n2. the same, with the file's order declared to the engine") + ordered = xo.connect() + ordered.raw_sql( + f"CREATE EXTERNAL TABLE snapshot_ordered STORED AS PARQUET LOCATION '{path}' WITH ORDER ({ROW} ASC)" + ) + o = ordered.table("snapshot_ordered") + for k in OFFSETS: + report(f"offset {k:>9,}", o.order_by(ROW).limit(50, offset=k), k) + print(" operators:", operators(ordered, o.order_by(ROW).limit(50, offset=1_000_000))) + + print("\n3. a user sort on g (200 distinct values, so ties of about 15,000 rows), offset 100,000") + for label, keys in (("ORDER BY g", ["g"]), (f"ORDER BY g, {ROW}", ["g", ROW])): + n, _, ms = measure(t.order_by(keys).limit(50, offset=100_000)) + print(f" {label:26s}: {n} distinct pages / {REQUESTS}, median {ms:.0f} ms") + + print(f"\n4. a range request, {ROW} >= k AND {ROW} < k + 50") + indexed = Path(tmp) / "snapshot_page_index.parquet" + pq.write_table(table, indexed, compression="zstd", row_group_size=1_048_576, write_page_index=True) + for label, file in (("row-group statistics only", path), ("with a parquet page index", indexed)): + r = xo.connect().read_parquet(str(file)) + print(f" {label}:") + for k in OFFSETS: + report(f" rows from {k:>9,}", r.filter((r[ROW] >= k) & (r[ROW] < k + 50)).order_by(ROW), k) + + print("\n5. a cheap recipe that does not name the column") + recipe = t.filter(t.g < 100).select("g", "v0").mutate(z=t.v0 * 2) + print(f" as written: columns {list(recipe.columns)}") + carried = carry_row_order(recipe) + print(f" as altered: columns {list(carried.columns)}") + want = int(np.flatnonzero(cols["g"] < 100)[100_000]) + report("page at offset 100,000", carried.order_by(ROW).limit(50, offset=100_000), want) + dropped = carry_row_order(t.drop(ROW, "v11")) + last, kept = dropped.columns[-1], "v11" in dropped.columns + print(f" t.drop({ROW!r}, 'v11') as altered: last column {last!r}, v11 kept: {kept}") + + print("\n6. names") + mem = xo.connect() + a, b, c = ( + mem.create_table(name, pd.DataFrame({"k": [1, 2, 3], col: [10, 20, 30], ROW: [0, 1, 2]})) + for name, col in (("a", "x"), ("b", "y"), ("c", "z")) + ) + print(f" two-way join: {list(a.join(b, 'k').columns)}") + print(f" three-way join: {list(a.join(b, 'k').join(c, 'k').columns)}") + print(f" debugging copy: {list(a.mutate(__row_order_v1=a[ROW]).columns)}") + + print("\nreference: first draft's approach, bare LIMIT/OFFSET on a single-partition connection") + single = xo.connect() + single.raw_sql("SET datafusion.execution.target_partitions = 1") + s = single.read_parquet(str(path)) + for k in OFFSETS: + report(f"offset {k:>9,}", s.limit(50, offset=k), k) + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_window_read_order.py b/scripts/spike_window_read_order.py new file mode 100644 index 0000000..2b4584d --- /dev/null +++ b/scripts/spike_window_read_order.py @@ -0,0 +1,148 @@ +"""ADR-008 evidence: which DataFusion settings make unsorted LIMIT/OFFSET windows repeatable and in file order? + +Writes parquet files above ``datafusion.optimizer.repartition_file_min_size`` whose rows are physically sorted by +``id``, then issues the same window request several times per configuration, for two plan shapes: + +* ``bare`` โ€” ``read_parquet -> limit/offset`` (a worthy entry's snapshot read) +* ``shaped`` โ€” ``read_parquet -> filter -> mutate -> limit/offset`` (a cheap entry's plan over a snapshot) + +For each it reports how many distinct pages came back, the first ids seen against the file-order answer, the +exchange operators in the physical plan (the governing variable: a window is in file order exactly when no +exchange operator sits between the scan and the limit), and the median latency. Two row-group sizes are used +because ``repartition_file_scans = false`` alone returns the file-order page for one and not for the other. + +It then times a full-scan aggregate under each configuration, which is the cost of applying an order-pinning +setting to a connection that also runs aggregates. + +A last section checks two ops that ``classify_build`` treats as cheap and that have more than a scan under them: +``union`` (``UnionExec`` emits one partition per input, so an exchange operator survives ``target_partitions = 1``) +and ``distinct`` (a hash aggregate). + + uv run python scripts/spike_window_read_order.py +""" + +from __future__ import annotations + +import hashlib +import statistics +import tempfile +import time +from pathlib import Path + +import numpy as np +import pyarrow as pa +import pyarrow.parquet as pq +import xorq.api as xo + +N = 3_000_000 +REQUESTS = 8 +EXCHANGES = ("RepartitionExec", "CoalescePartitionsExec", "SortPreservingMergeExec") +CONFIGS = { + "default": [], + "repartition_file_scans=false": ["SET datafusion.optimizer.repartition_file_scans = false"], + "rfs=false + round_robin=false": [ + "SET datafusion.optimizer.repartition_file_scans = false", + "SET datafusion.optimizer.enable_round_robin_repartition = false", + ], + "target_partitions=1": ["SET datafusion.execution.target_partitions = 1"], +} + + +def connect(stmts: list[str]): + con = xo.connect() + for stmt in stmts: + con.raw_sql(stmt) + return con + + +def shapes(t): + return {"bare": t, "shaped": t.filter(t.id % 3 != 0).mutate(z=t.id * 2)} + + +def file_order_first_id(shape: str, offset: int) -> int: + if shape == "bare": + return offset + # kept ids are 1, 2, 4, 5, 7, 8, ...: the k-th (0-based) is 3 * (k // 2) + 1 + (k % 2) + return 3 * (offset // 2) + 1 + (offset % 2) + + +def plan_summary(con, expr) -> str: + plan = con.raw_sql("EXPLAIN " + xo.to_sql(expr)).to_pandas() + physical = plan[plan.iloc[:, 0] == "physical_plan"].iloc[0, 1] + found = [name for name in EXCHANGES if name in physical] + groups = physical.split("file_groups={")[1].split(" group")[0] + return f"{groups} file group(s), exchanges: {', '.join(found) or 'none'}" + + +def cheap_ops_beyond_a_scan(tmp: Path) -> None: + rng = np.random.default_rng(1) + half = N // 2 + paths = [] + for i in range(2): + path = tmp / f"part{i}.parquet" + part = pa.table({"id": np.arange(half) + i * half, "g": rng.integers(0, 5_000, half), "v": rng.random(half)}) + pq.write_table(part, path, row_group_size=100_000) + paths.append(path) + print("\n=== cheap ops with more than a scan under them (two files, ids 0..N/2-1 and N/2..N-1)") + for label in ("default", "target_partitions=1"): + con = connect(CONFIGS[label]) + a, b = (con.read_parquet(str(path)) for path in paths) + for name, expr, offset in ( + ("union", a.union(b), 1_400_000), + ("distinct", a.select("g").distinct(), 2_000), + ): + window = expr.limit(50, offset=offset) + pages = {hashlib.md5(window.execute().to_csv(index=False).encode()).hexdigest() for _ in range(REQUESTS)} + physical = con.raw_sql("EXPLAIN " + xo.to_sql(window)).to_pandas() + physical = physical[physical.iloc[:, 0] == "physical_plan"].iloc[0, 1] + found = [op for op in (*EXCHANGES, "UnionExec") if op in physical] + print( + f"{label:30s} {name:8s} offset={offset:>9,}: {len(pages)} distinct pages / {REQUESTS} " + f"| operators: {', '.join(found) or 'none'}" + ) + + +def main() -> None: + rng = np.random.default_rng(3) + table = pa.table({"id": np.arange(N), "g": rng.integers(0, 50_000, N), "v": rng.random(N), "w": rng.random(N)}) + probe = xo.connect() + threshold = probe.raw_sql("SHOW datafusion.optimizer.repartition_file_min_size").to_pandas().iloc[0, 1] + partitions = probe.raw_sql("SHOW datafusion.execution.target_partitions").to_pandas().iloc[0, 1] + print(f"engine: repartition_file_min_size={threshold} target_partitions={partitions}") + + with tempfile.TemporaryDirectory() as tmp: + for row_group_size in (8_192, 100_000): + path = Path(tmp) / f"sorted_by_id_rg{row_group_size}.parquet" + pq.write_table(table, path, row_group_size=row_group_size) + print(f"\n=== {path.stat().st_size / 1e6:.1f} MB file, rows sorted by id, row groups of {row_group_size:,}") + for label, stmts in CONFIGS.items(): + con = connect(stmts) + t = con.read_parquet(str(path)) + for shape, expr in shapes(t).items(): + for offset in (0, 1_000_000): + window = expr.limit(50, offset=offset) + pages, firsts, secs = set(), set(), [] + for _ in range(REQUESTS): + t0 = time.perf_counter() + df = window.execute() + secs.append(time.perf_counter() - t0) + pages.add(hashlib.md5(df.to_csv(index=False).encode()).hexdigest()) + firsts.add(int(df["id"].iloc[0])) + want = file_order_first_id(shape, offset) + print( + f"{label:30s} {shape:6s} offset={offset:>9,}: {len(pages)} distinct pages / {REQUESTS}, " + f"first ids {sorted(firsts)[:3]} (file order: {want}), " + f"median {statistics.median(secs) * 1000:.0f} ms | {plan_summary(con, window)}" + ) + agg = t.group_by("g").agg(n=t.count(), s=t.v.sum()) + secs = [] + for _ in range(3): + t0 = time.perf_counter() + agg.execute() + secs.append(time.perf_counter() - t0) + print(f"{label:30s} full-scan aggregate (50k groups): median {statistics.median(secs) * 1000:.0f} ms") + cheap_ops_beyond_a_scan(Path(tmp)) + + +if __name__ == "__main__": + main() From 077e30810623bf5ea9e1bd62a9e3edab436da59c Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Sun, 20 Sep 2026 10:31:43 -0400 Subject: [PATCH 2/4] docs(plans): fold design-session answers 7-10 into the cache redesign ADRs - ADR-009 D6 (new): create runs the query twice and compares digests, so a non-reproducible recipe is known from birth and its file is pinned; a cheap entry gets the same check and is materialized if it fails. Adds a Testing section. The entry's schema is read from the file. - ADR-007 D11 (new): one write at a time per project, by taking the existing project file lock around every write; replaces the per-entry lock. Two tallyman servers on one project is unsupported (#183). - ADR-007 D12 (new): files are deleted only by an explicit user action; the startup warm-up stops rewriting deleted files; no disk budget yet. - ADR-007 D6: tallyman stops remembering Buckaroo sessions and re-posts every time, with a session id derived from project, hash and view kind. - ADR-007 D9: the agreed order of work. Nothing starts before review. - ADR-007 open question 1: whether two kinds of entry survive. Unanswered. - Testing sections for ADR-007 and ADR-008, marking which tests are red on main today. Co-Authored-By: Claude Fable 5.1 --- .../ADR-007-tallyman-owned-materialization.md | 137 +++++++++++++++--- plans/ADR-008-row-order-of-reads.md | 29 +++- plans/ADR-009-digest-stability.md | 94 ++++++++++-- 3 files changed, 222 insertions(+), 38 deletions(-) diff --git a/plans/ADR-007-tallyman-owned-materialization.md b/plans/ADR-007-tallyman-owned-materialization.md index 404cd5d..5880e1f 100644 --- a/plans/ADR-007-tallyman-owned-materialization.md +++ b/plans/ADR-007-tallyman-owned-materialization.md @@ -1,8 +1,9 @@ # ADR: Tallyman owns result materialization (no xorq cache nodes in builds) - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling - session, which added the governing rule, decision D10, and the resolution - recorded under D5). Supersedes two decisions of + session, which added the governing rule, decisions D10 to D12, and the + resolution recorded under D5). Awaiting Paddy's review; nothing here is + implemented. Supersedes two decisions of `plans/ADR-006-read-path-loads-builds.md`: its D4 (chaining inlines the parent's cache node) and its D8 (the manifest records the snapshot key and reads assert it). Five other ADR-006 decisions keep their intent: D5 (the @@ -257,8 +258,8 @@ of every descendant would re-run the parent's expensive subgraph. record-batch stream, and writes the snapshot itself: - a unique temp name in the destination directory, then `os.replace`; -- under a per-hash cross-process `flock`, with the existence check repeated - inside the lock, so a second process waits and then finds the file; +- under the project's write lock (D11), with the existence check repeated + inside the lock, so a second writer waits and then finds the file; - it numbers the rows as it writes them, in a last column named `__row_order` (decision D2 of `plans/ADR-008-row-order-of-reads.md`); - it returns the digest of what it wrote. @@ -356,9 +357,16 @@ Buckaroo cannot be asked to display an entry whose query is still running. The same ordering now covers a snapshot that was deleted later: `load_session` waits for `ensure_materialized` before it posts anything. -Deleting a snapshot (the Cache page, a reset prune, a future budget eviction) -ends every live Buckaroo session whose plan reads it first, using the -session-eviction hook that ADR-006 decision D10 introduced. The next +The same rule covers the session itself. Buckaroo drops a session that has had +no browser attached for an hour, and tallyman's session map assumes a session +lives as long as the Buckaroo process, so an entry reopened after an idle hour +is handed an id Buckaroo no longer knows and the grid never loads. Tallyman +stops remembering sessions. The session id is derived from the project, the +content hash and the view kind (D10), and `load_session` posts `/load_expr` +every time, which Buckaroo already treats as a no-op for a session it has. + +Deleting a snapshot (D12) ends every live Buckaroo session whose plan reads it +first, using the session-eviction hook that ADR-006 decision D10 introduced. The next `/api/session` re-materializes and opens a new session. Without this, a page request against a deleted file would fail inside Buckaroo with the `At least one path is required` error above, which is exactly the kind of @@ -395,9 +403,19 @@ cache node has crept back in. Removing the cache node changes the hash of every worthy entry, and bare-read chaining changes every child's. This lands together with ADR-008's change to `tallyman_read_csv` and ADR-009's digest definition, behind a single corpus -rebuild, with the failing tests committed and seen red first. After the -rebuild, `~/.cache/xorq/result_cache` (14 GB) and the older leaks under -`~/.cache/xorq/parquet/` (2.6 GB) can be deleted by hand. +rebuild. After the rebuild, `~/.cache/xorq/result_cache` (14 GB) and the older +leaks under `~/.cache/xorq/parquet/` (2.6 GB) can be deleted by hand. + +Order of work, agreed 2026-09-20. Nothing starts until Paddy has reviewed all +three ADRs. + +1. One commit of failing tests covering everything the three ADRs change, plus + the audit's independent bugs, pushed and seen red on CI. +2. The redesign as one change, then the corpus rebuild. +3. The independent bugs that remain, which change no hash: the chart error + loop, eager notebook sessions, the staleness scan re-hashing every source + per entry, `tallyman pack` shipping the cache, the primary-key search that + never converges, and the over-broad stat-cache wipes. ### D10. Every diff is built as an entry before it is displayed @@ -440,6 +458,70 @@ putting an alias on an entry that already exists. page sooner, and it breaks the governing rule in both directions: Buckaroo runs tallyman's join, repeatedly, and tallyman never learns whether it succeeded. +### D11. One write at a time per project + +Every write takes the project's existing lock: a build, a materialization, a +promote and a recalc, as well as the checkpoint that takes it today. +`_project_lock` (`catalog_state.py:232-242`) is an OS file lock on +`.checkpoint.lock`, so it holds between the two processes of a normal tallyman, +the MCP server and the companion, which both build. It becomes re-entrant +within a process, since a promote builds and then checkpoints. + +This replaces the per-entry lock of this ADR's first draft and closes the audit +finding that two builds of one entry can end with the failing one deleting the +winner's directory (`build.py:447-468`, `618-627`). FastMCP runs tool calls on +a thread pool, so parallel tool calls did build at once. They now queue. + +Two tallyman servers pointed at one project is unsupported. Paddy: "you have +done something diabolical and deserve the results." The file lock would still +serialize their writes on one machine, and nothing else about them is safe, +because each holds in-process state the other never sees. +buckaroo-data/tallyman#183 tracks detecting that case and refusing to start. + +### D12. Files are deleted only by an explicit user action + +Nothing deletes a materialized file on its own, and nothing rewrites one except +opening an entry that needs it (D5). There is no disk budget yet. That is the +rewrite of `plans/ADR-003-result-cache-cost-rubric.md`, deferred. + +- The startup warm-up stops calling `cached_result_expr`. Today it rewrites + every snapshot that was deleted, which undoes the Cache page's delete button + and can block startup on one large file. +- An explicit delete skips an entry marked not reproducible (decision D6 of + `plans/ADR-009-digest-stability.md`), whose file cannot be recreated. +- Ephemeral diff entries (D10) follow the same rule and are not collected + automatically. + +## Testing + +Tests marked *red* fail on `main` today and belong in the failing-tests commit. +The others cannot fail before the code exists and ride with the change. + +- **Sentinel** (*red*, D8). With `XORQ_CACHE_DIR` pointing at an empty + directory, a build, a chained child build, a view, a delete and a reopen + leave that directory empty. +- **Concurrent builds** (*red*, D11). Two threads building the same entry both + return it, and the entry's directory is intact afterwards. +- **Forgotten session** (*red*, D6). After Buckaroo has dropped a session, + reopening the entry yields a grid that loads. +- **Warm-up leaves deleted files alone** (*red*, D12). A snapshot deleted before + startup is still absent after startup with no requests made. +- **Identity** (D3). A child's hash changes when its parent's snapshot path + changes and not when the file's bytes do, and a filter over an aggregate's + snapshot is classed cheap. +- **Files exist before anything runs** (D5). With an ancestor's snapshot + deleted, opening a descendant rewrites the ancestor first, verifies it, and + never executes a plan whose file is missing. +- **Cold equals warm** (D7). With `compute_cache/` removed, every snapshot an + entry needs is reproduced with its recorded digest. +- **Handoff** (D6). A worthy entry's grid is posted a view build of its + snapshot, and Buckaroo writes nothing under `compute_cache/result_cache/`. +- **Diffs** (D10). Opening a diff builds an entry under + `compute_cache/ephemeral_entries/` before Buckaroo is called, the checkpoint + does not zip it, and promoting it keeps the same content hash. +- **Explicit delete** (D12). Deleting a snapshot ends the sessions that read it, + and the next open rewrites it. + ## Consequences - **Retired:** the `.cache()` call and the source-read injection in @@ -484,15 +566,26 @@ tallyman's join, repeatedly, and tallyman never learns whether it succeeded. ## Open questions -1. **Deep cheap chains.** Nothing cuts the graph between cheap entries. Run - `tests/test_perf_chain_depth.py` against this design and decide whether a - node-count or `compile_seconds` threshold should make an otherwise cheap - entry worthy. -2. **An unfaithful parent's descendants.** Descendants of an entry whose heal - failed verification were computed from bytes that no longer exist. Nothing - flags them today, and nothing here does either. -3. **Eviction policy.** D6 says what eviction must do to live sessions. Which - snapshots to evict, and when, stays with the ADR-003 rewrite. -4. **Garbage collection of ephemeral entries.** D10 says where they live and - that they are deletable. When to delete them belongs with the eviction - policy of open question 3. +1. **Do two kinds of entry survive?** This is the largest open question of the + set and Paddy has not answered it. Materializing every entry when it is + created would remove the cheap and worthy classifier, the build error of + ADR-008 decision D3, the allow-list of ADR-008 decision D4, the view case in + D6, and open question 2 below, and it would give every entry a digest. An + entry built on another would always read the parent's file, so no graph + would be more than one entry deep, which is the parquet boundary Paddy wanted + in June. `plans/ADR-003-result-cache-cost-rubric.md` already proposes + admitting every result and evicting by budget. The cost is one file per + entry: one project measured 19 GB of cache for 779 MB of data while every + CSV revision wrote a file. His answer to question 4 ("materialize the + parquet if necessary") stands until he says otherwise. +2. **Deep cheap chains.** Nothing cuts the graph between cheap entries. Moot if + open question 1 is answered with one kind of entry. +3. **Where ordered copies of sources live.** ADR-008 decision D2 adds one per + parquet source. `csv_ordered/` is global today, is never collected, and is + not included by `tallyman pack`. If every entry is materialized, a root + entry's own file could serve as the ordered copy. + +Closed in the grilling session: an unfaithful parent's descendants (ADR-009 +decision D6 finds a non-reproducible entry when it is created and pins its +file, so it is never rewritten underneath its descendants); eviction policy +and the collection of ephemeral entries (D12). diff --git a/plans/ADR-008-row-order-of-reads.md b/plans/ADR-008-row-order-of-reads.md index a7e0eea..3ab6b25 100644 --- a/plans/ADR-008-row-order-of-reads.md +++ b/plans/ADR-008-row-order-of-reads.md @@ -1,7 +1,7 @@ # ADR: Row order of reads (every file carries `__row_order`, every page sorts by it) - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling - session). The first draft pinned row order with an engine setting. Paddy + session). Awaiting Paddy's review; nothing here is implemented. The first draft pinned row order with an engine setting. Paddy proposed baking a row-order column into every file tallyman writes and sorting every page by it. The measurements below favour that, so it is now the decision and the engine setting is the rejected alternative under D5. @@ -339,6 +339,33 @@ does not establish a split scan and is not what makes that test meaningful; its aggregate is), and `src/tallyman_xorq/source_cache.py:98`. `tests/test_tallyman_read_csv.py:159` already says about 10 MB. +## Testing + +Tests marked *red* fail on `main` today and belong in the failing-tests commit. +The others cannot fail before the code exists and ride with the change. + +- **Repeatable pages** (*red*, D5). Eight identical `/api/data` requests at a + deep offset into an entry whose file is larger than 10,485,760 bytes return + identical rows, for a worthy entry and for a cheap one. The sorted case is + Buckaroo's code and is tested there (buckaroo-data/buckaroo#974). +- **The column** (D2). Every file tallyman writes ends in `__row_order`, holding + `0..N-1` with no gaps, and a materialization replaces an inherited one. +- **Dropping it fails the build** (D3). A cheap recipe whose select list omits + the column raises a build error whose message contains the corrected select. + A worthy recipe that omits it builds. +- **Asking for an order renumbers** (D3). An `order_by` recipe's file is + numbered in the requested order, and assigning to the column is a build + error. +- **A debugging copy survives** (D6). `__row_order_v1` is still present, with + the parent's positions, after the child is materialized. +- **Classification** (D4). A union, a distinct, an unnest and an operation the + allow-list has never seen are all classed worthy. +- **Reserved** (D6). The primary-key search never returns `__row_order`, and a + diff has no `__row_order_v2` column. +- **CSV roots** (D7). A `tallyman_read_csv` entry has no Sort in its build, is + classed cheap, and has exactly one row-order column. +- **The hint** (D8). The `/load_expr` payload names `__row_order`. + ## Consequences - Pages are repeatable for unsorted and sorted requests, in tallyman and in diff --git a/plans/ADR-009-digest-stability.md b/plans/ADR-009-digest-stability.md index 5f9763c..0b34f87 100644 --- a/plans/ADR-009-digest-stability.md +++ b/plans/ADR-009-digest-stability.md @@ -1,8 +1,9 @@ # ADR: Digest stability (a heal is flagged only when the result changed) -- **Status:** Proposed (2026-09-18, revised 2026-09-20: D3 gains two format - requirements from `plans/ADR-008-row-order-of-reads.md`, and D1 lost its - speed gate). Amends +- **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling + session: D3 gains two format requirements from + `plans/ADR-008-row-order-of-reads.md`, D1 lost its speed gate, and D6 is + new). Awaiting Paddy's review; nothing here is implemented. Amends `plans/ADR-004-result-digest-canonical-ordering.md` (Option A's "hash the snapshot bytes") and decision D5 of `plans/ADR-006-read-path-loads-builds.md` (the canonical sort), which said @@ -214,6 +215,11 @@ decoding a whole row group: 19 to 24 ms at any depth with the index against 79 to 90 ms without it, on a 287 MB file (`scripts/spike_row_order_paging.py` on the ADR-008 branch). +The entry's recorded schema (`schema.json`) is read from the written file and +not from the expression. The file is what every consumer reads, the writer +adds a column the graph does not have (`__row_order`), and parquet changes some +types on the way in: a `timestamp[s]` column comes back as `timestamp[ms]`. + Combining each row group keeps the file bytes reproducible as well. Nothing depends on that after D2, and it means two writes of the same entry can still be compared with `cmp` when debugging. Because of D2 these settings can change later without @@ -232,18 +238,81 @@ row. ### D5. It lands with the rebuild Every worthy entry's digest is recomputed by the corpus rebuild of ADR-007 -decision D9 (one change, one rebuild). -The manifest field keeps its name. Tests that cannot fail first ride with the -fix; the float-aggregate heal test and a batch-boundary digest test fail on -`main` and belong in the failing-tests commit. +decision D9 (one change, one rebuild). The manifest field keeps its name. + +### D6. Create runs the query twice and compares + +Decided by Paddy in the grilling session (2026-09-20): "call the same query +twice... put some tests around this." + +Today tallyman learns that an entry is not reproducible only when a deleted +file is rewritten and its digest differs. By then the original rows are gone, +and everything built on them disagrees with the new file without anyone +knowing. So the check moves to the moment the entry is created: + +- `materialize` runs the entry's query twice through the same writer, on the + same single-partition connection (D1), and compares the two content digests + (D2). Both runs go through the writer because D2's digest is defined on the + file as read back. The second file is then discarded. +- When they match, the entry is recorded as reproducible, with its digest. +- When they differ, the build still succeeds, because a recipe that calls + `sample()` is legitimate. The entry is recorded as not reproducible, its file + is pinned (never deleted by tallyman, ADR-007 decision D12), it is badged in + the UI, and the build result tells the author so, with the columns whose + digests differed. This is the state ADR-006 decision D12 (unfaithful entries + are pinned and badged) reaches after the damage is done. D6 reaches it first. +- A cheap entry gets the same check. Its two runs are streamed and digested + without writing a file. If they differ, the entry is materialized and pinned + like any other non-reproducible entry, because re-running it on every read + would show different values each time. A computed column that calls + `random()` is row-preserving, so it is classed cheap today, and nothing + detects it: ADR-006 decision D9 ("no cheap-entry digests") assumed a frozen + graph over pinned inputs always gives the same rows. + +The cost is a second execution of every create. It is accepted under the +priority recorded in ADR-007 ("a cohesive system that works reliably" first). + +A heal is still verified against the recorded digest, as now. After D6 a +mismatch there means something changed underneath a reproducible entry, which +D4 attributes. + +## Testing + +Tests marked *red* fail on `main` today and belong in the failing-tests commit. +The others cannot fail before the code exists and ride with the change. + +- **Float aggregate reproduces** (*red*). An entry with a float `SUM` and `AVG` + over a source large enough to run in parallel is created, its file deleted, + and the entry reopened, three times. Every rewrite must match the recorded + digest, and no `unfaithful_heal` record may be written. +- **Digest ignores batching and format.** The same rows delivered as 8,192-row + batches, 100,000-row batches and one table give one digest, and so do the + same rows written Snappy with small row groups. +- **Digest sees what it must.** One changed value, one null replaced by `0.0`, + and two swapped rows each change the digest. +- **Create detects a non-reproducible recipe.** A recipe whose UDF returns a + different value on every call builds successfully, is recorded as not + reproducible, names the offending column, and has a pinned file. +- **Create passes a reproducible recipe.** A deterministic recipe is recorded + as reproducible, and the query is observed to run exactly twice. +- **A non-reproducible cheap recipe is materialized.** A computed column that + calls `random()` ends up with a pinned file instead of re-running per read. +- **A pinned file survives an explicit delete.** The Cache page's delete skips + it and says why. +- **The schema comes from the file.** An entry with a `timestamp[s]` column + records `timestamp[ms]`, and every entry's recorded schema ends in + `__row_order`. ## Consequences - A float-aggregate entry heals to the same digest, and a pyarrow upgrade no longer flags anything. The loud responses of ADR-006 decisions D10 and D12 are left for the causes they were designed for. -- Materializations and heals are slower, by about 3x on aggregation at spike scale and up - to 7x in ADR-004's parking measurement. Reads are unaffected. +- Materializations and heals are slower, by about 3x on aggregation at spike + scale and up to 7x in ADR-004's parking measurement, and a create runs its + query twice (D6). Reads are unaffected. +- A recipe that is not reproducible is known to be so from the moment it is + created, and its file is never rewritten underneath the entries built on it. - Verify decodes the file instead of hashing its bytes. It runs on a heal and in `catalog_scan_staleness(verify_results=True)`, never on a read. - Snapshots are smaller, and their footers were 85 times smaller in the spike, @@ -258,11 +327,6 @@ fix; the float-aggregate heal test and a batch-boundary digest test fail on 1. **Nested types.** The spike covers fixed-width, boolean, string and binary columns. Lists, structs and maps need a recursive definition. -2. **Types parquet cannot store as given.** A `timestamp[s]` column comes back - as `timestamp[ms]`, so the snapshot's schema differs from the entry's - recorded schema. Either the writer refuses such a column, or the entry's - schema is recorded from the snapshot. `__row_order` pushes toward the second - answer, since the writer adds a column the entry's graph does not have. -3. **Engine upgrades.** Single-partition execution fixes the merge order within +2. **Engine upgrades.** Single-partition execution fixes the merge order within one engine version. Nothing guarantees float results across versions. D4 attributes that case and the remedy stays a rebuild. From 20d330de9f9a99a6eddacead6b27147d6c1b3aeb Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Sun, 20 Sep 2026 11:44:17 -0400 Subject: [PATCH 3/4] docs(plans): fold the PR #184 review into the cache redesign ADRs An adversarial review of the three draft ADRs, and Paddy's decisions on its findings (2026-09-20). Still Proposed; nothing is implemented. - ADR-007 D10 (every diff is built as an entry) moved out to #188, so the set stays about the core structure of the cache. D10 is kept as a stub so that references to D11 and D12 stay valid. The live diff is recorded as the one known exception to the governing rule. - ADR-007 D5: the verify sweep is not a caller of ensure_materialized; it reads and never writes. The ordered-copy gap is recorded as accepted. - ADR-007 D6: the session id is derived from project and content hash (closes #172); the exact condition under which Buckaroo skips a repeat /load_expr; deleting a snapshot ends no session; an unfaithful heal posts force_reload, since evict_session worked by forgetting a record that D6 removes. - ADR-007 D11: three limits of the project lock (per-thread re-entrancy, recalc granularity left open, reads not covered, #118). #186 tracks the waiting it causes. - ADR-007 D12: a file is written only because something is about to read it. The warm-up sentence now states the precise condition. - ADR-007 D2: memoizing the read stops tables piling up, not footer reads. - ADR-008 D2, D7: CSVs do not already work this way. The intermediate is keyed by path and overwritten in place (#168), so D7 cannot land before that fix. CSVs go through source identity and the ordered copy is built from the content-addressed clone. - ADR-008: "repeatable pages" asserts the exact rows; a new test that an edited CSV forks the hash; memory at depth recorded under Consequences as a performance matter, deferred. - ADR-009 D1, D3: single-partition execution does not make an ungrouped float total a function of the rows alone; it depends on the parent file's row-group layout (#187). Row-group size and batch_size are pinned and the manifest records a snapshot format version. - ADR-009 D6: the cheap-entry half moved to #185; the limits of running twice are stated, and the #88 lint is mentioned. - ADR-009: four stale cross-references to ADR-008 fixed. - All three Testing sections: normal TDD. Every test goes in the failing-tests commit; a test of a missing function fails on import. New evidence scripts, each runnable from a clean temp dir: scripts/spike_csv_source_identity.py (ADR-008 D2, D7), scripts/spike_float_layout_digest.py (ADR-009 D1, D3), scripts/spike_deep_page_memory.py (ADR-008 Consequences). Co-Authored-By: Claude Fable 5.1 --- .../ADR-007-tallyman-owned-materialization.md | 257 ++++++++++++------ plans/ADR-008-row-order-of-reads.md | 106 ++++++-- plans/ADR-009-digest-stability.md | 147 +++++++--- scripts/spike_csv_source_identity.py | 77 ++++++ scripts/spike_deep_page_memory.py | 77 ++++++ scripts/spike_float_layout_digest.py | 83 ++++++ 6 files changed, 594 insertions(+), 153 deletions(-) create mode 100644 scripts/spike_csv_source_identity.py create mode 100644 scripts/spike_deep_page_memory.py create mode 100644 scripts/spike_float_layout_digest.py diff --git a/plans/ADR-007-tallyman-owned-materialization.md b/plans/ADR-007-tallyman-owned-materialization.md index 5880e1f..701518e 100644 --- a/plans/ADR-007-tallyman-owned-materialization.md +++ b/plans/ADR-007-tallyman-owned-materialization.md @@ -2,8 +2,10 @@ - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling session, which added the governing rule, decisions D10 to D12, and the - resolution recorded under D5). Awaiting Paddy's review; nothing here is - implemented. Supersedes two decisions of + resolution recorded under D5, and again the same day after a review of + PR #184: D10 moved out to #188, the verify sweep left D5's callers, D6's + session-ending clause was dropped, and D12's rule was restated). Awaiting + Paddy's review; nothing here is implemented. Supersedes two decisions of `plans/ADR-006-read-path-loads-builds.md`: its D4 (chaining inlines the parent's cache node) and its D8 (the manifest records the snapshot key and reads assert it). Five other ADR-006 decisions keep their intent: D5 (the @@ -19,6 +21,10 @@ buckaroo-data/buckaroo#972 (`/load_expr` has no `cache_dir`). The direction was set by Paddy the same day: "I want to depend on xorq as little as possible for caching." +- **Tickets:** #188 (diffs, moved out of this ADR), #186 (the waiting that + D11's lock causes), #185 (non-pure recipes), #183 (two servers on one + project), #168 (CSV source identity, which the shared rebuild of D9 needs), + #118 (concurrent reads on the shared backend, which D11 does not cover). - **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`), `src/tallyman_xorq/result_cache.py` (`_resolve_result_plan`, `cached_result_expr`, `entry_graph_expr`, `baked_snapshot_path`, @@ -154,6 +160,10 @@ computation, writes result files, or repairs tallyman's cache. Besides being simpler, this puts every failure of a computation in tallyman's process, where it can be logged and reported, and none inside a grid query in Buckaroo. +One exception is known and accepted for now. The live diff still hands +Buckaroo an unmaterialized join, because the decision that fixed it (D10) was +moved out of this set to #188. + ## Decisions ### D1. Builds carry no cache nodes @@ -201,9 +211,12 @@ snapshot comes from the manifest's `cache_worthy`, as it does now. the facade"). For a worthy entry it returns one bare read of `snapshot_path`, memoized per `(project, content_hash)` for the life of the process, so repeated reads stop -registering tables and stop re-opening the footer. A worthy entry whose file -exists is served without loading its build at all. For a cheap entry it returns -the loaded graph, as now. +piling up tables in the shared backend: one read has one table name. It does +not stop the footer being opened per query, because xorq registers a deferred +read's table again on every execute (`xorq/expr/api.py`, +`_transform_deferred_reads`). With the format of ADR-009 decision D3 that is a +2.7 KB read. A worthy entry whose file exists is served without loading its +build at all. For a cheap entry it returns the loaded graph, as now. Deleted: `manifest.snapshot_key`, `_assert_recorded_snapshot_key`, `_cached_node_path`, `rewrite_cache_dirs`, and the `cache_dir` argument. With @@ -292,8 +305,26 @@ badge (ADR-006 decision D12). Callers: the canonical read (`cached_result_expr`, on every call, where step 1 or a handful of `stat` calls is the whole cost, and which covers diff -composition), chaining at mint time (D3), `load_session` (D6), and the verify -sweep. +composition), chaining at mint time (D3), and `load_session` (D6). + +The verify sweep (`verify_sweep`, `staleness.py:118`, reached through +`catalog_scan_staleness(verify_results=True)`) is not a caller. It checks the +files that exist, reports each entry that recorded a digest as faithful, +unfaithful or absent, and writes nothing, which is what it does today. A sweep +that called this function would rewrite every deleted snapshot in the project, +which D12 forbids. Nothing is lost by leaving absent files alone: every file +this function writes is verified before it is served, so an absent file is +checked at the moment it next exists. A file that exists with the wrong digest +is reported through the same loud path and left in place, since deleting it is +the user's action (D12). + +One gap is known and accepted. Step 2 collects only reads under +`compute_cache/result_cache/`. A root entry also reads an ordered copy of its +source (decision D2 of `plans/ADR-008-row-order-of-reads.md`), which lives +elsewhere and which this function does not re-create. If one is missing, +Buckaroo fails with `At least one path is required`. Paddy, 2026-09-20: +Buckaroo erroring when tallyman has not provided a prerequisite is acceptable +for now, and follow-on work closes it. ADR-006 decision D4 rejected bare-read chaining because "builds stay non-self-contained and the pre-heal choreography stays load-bearing forever". @@ -361,16 +392,37 @@ The same rule covers the session itself. Buckaroo drops a session that has had no browser attached for an hour, and tallyman's session map assumes a session lives as long as the Buckaroo process, so an entry reopened after an idle hour is handed an id Buckaroo no longer knows and the grid never loads. Tallyman -stops remembering sessions. The session id is derived from the project, the -content hash and the view kind (D10), and `load_session` posts `/load_expr` -every time, which Buckaroo already treats as a no-op for a session it has. - -Deleting a snapshot (D12) ends every live Buckaroo session whose plan reads it -first, using the session-eviction hook that ADR-006 decision D10 introduced. The next -`/api/session` re-materializes and opens a new session. Without this, a page -request against a deleted file would fail inside Buckaroo with the -`At least one path is required` error above, which is exactly the kind of -failure the rule says belongs to tallyman. +stops remembering sessions. The session id is derived from the project and the +content hash, and `load_session` posts `/load_expr` with that id every time. +Buckaroo 0.15.6 skips the work when it already has a session with that id and +the same build directory, provided the post carries none of +`component_config`, `column_config_overrides`, `extra_grid_config`, `init_sd` +and `skip_stat_columns` (`buckaroo/server/handlers.py`, lines 429-454). +Tallyman sends none of them for an ordinary entry, so the repeat post is a +no-op there. A promoted diff entry sends `column_config_overrides` and so +reloads on every open, which #188 covers. Putting the project in the id also +closes #172 (one project's session served to another on a hash collision). + +Deleting a snapshot (D12) ends no session. A tab that already has the entry +open fails on its next query, inside Buckaroo, with the +`At least one path is required` error above. That is accepted: the user +deleted the file on purpose, and reopening the entry fixes it, because every +open goes through `ensure_materialized` before it posts. The session Buckaroo +still holds then works again. xorq registers the path afresh on every query, +and a faithful rewrite has the same content, so the session's stats are still +right. An earlier draft ended every session whose plan read the deleted file. +It was dropped in review: it needs an index from each snapshot to every entry +that reads it, and Buckaroo has no route that ends a session. + +An unfaithful heal is the one event that leaves a session wrong, since the +path now holds different rows and the session holds stats computed from the +old ones. ADR-006 decision D10 handles it today through `evict_session`, which +works by dropping tallyman's own record of the session so that the next load +mints a new one. With no record to drop, the companion's unfaithful-heal hook +wipes the entry's stat cache, as now, and posts `/load_expr` for the entry's +id with `force_reload: true`, which Buckaroo already accepts and which re-runs +its pipeline for that session. Sessions of entries built on the healed file +are stale too, as they are today; that is part of #185. This resembles what #104 removed: #102's viewer build over `/result.parquet`. #104's objection was two materialized copies per @@ -402,9 +454,11 @@ cache node has crept back in. Removing the cache node changes the hash of every worthy entry, and bare-read chaining changes every child's. This lands together with ADR-008's change to -`tallyman_read_csv` and ADR-009's digest definition, behind a single corpus -rebuild. After the rebuild, `~/.cache/xorq/result_cache` (14 GB) and the older -leaks under `~/.cache/xorq/parquet/` (2.6 GB) can be deleted by hand. +`tallyman_read_csv`, the fix for #168 that the change depends on (CSV sources +go through source identity, ADR-008 decision D2), and ADR-009's digest +definition, behind a single corpus rebuild. After the rebuild, +`~/.cache/xorq/result_cache` (14 GB) and the older leaks under +`~/.cache/xorq/parquet/` (2.6 GB) can be deleted by hand. Order of work, agreed 2026-09-20. Nothing starts until Paddy has reviewed all three ADRs. @@ -417,46 +471,28 @@ three ADRs. per entry, `tallyman pack` shipping the cache, the primary-key search that never converges, and the over-broad stat-cache wipes. -### D10. Every diff is built as an entry before it is displayed - -Live and promoted diffs already build the same expression -(`build_compare_expr`). The live path posts it to Buckaroo unmaterialized -(`_build_compare_expr`, `app.py:418-441`), so the outer join runs again for -every page, sort and stat query in the diff grid, and a failure of the join -surfaces inside Buckaroo. The promote path writes a recipe that calls -`build_diff_expr(a_hash, b_hash, keys)` and runs the normal build. - -Every diff now takes the promote path. When a diff view is opened, tallyman -builds the diff entry and waits for the build to finish. A diff contains a -join, so it is worthy and is materialized, and Buckaroo is then handed a view -build of the finished file (D6) with the diff's display configuration. The page -shows a "building diff" state while it waits. Promoting a diff is reduced to -putting an alias on an entry that already exists. - -- **An unnamed diff entry is ephemeral.** The checkpoint zips and commits every - complete directory under `entries/`, aliased or not (`zip_pending_entries`, - `catalog.py:160-180`), so an unnamed diff stored there would put every diff - ever viewed into the catalog's git history. Ephemeral entries live under - `compute_cache/ephemeral_entries//`, where everything is - already defined as deletable at any time, and they can be rebuilt from the - two hashes and the keys. Promote moves the directory into `entries/`, sets - the alias and checkpoints. The content hash is computed from the graph, so - the move does not change it, and the snapshot path stays the same. -- **Parents.** A worthy side is read from its snapshot. A cheap side is not - copied first: because the diff is itself materialized, each side is read - exactly once, while the diff is written. -- **Sessions.** The same entry can be opened as a diff, with the diff display - classes, or as a plain entry, so the Buckaroo session key includes the view - kind and is no longer the content hash alone. -- **Retired with this:** `_build_compare_expr` and its temp build directory, - the separate diff-session bookkeeping (`diff_session_is_loaded`, - `mark_diff_session_loaded`), and `diff_stat_cache/`, since a diff's Buckaroo - stats become its entry's own. The audit finding that every recalc wipes all - of `diff_stat_cache/` goes with it. - -*Rejected:* keep posting the join and materialize nothing. It shows a first -page sooner, and it breaks the governing rule in both directions: Buckaroo runs -tallyman's join, repeatedly, and tallyman never learns whether it succeeded. +### D10. Diffs: moved to a follow-on (#188) + +This decision said that every diff is built as an entry before it is +displayed, with an unnamed diff stored as an ephemeral entry under +`compute_cache/`. Paddy moved it out of this set on 2026-09-20, so that the set +stays about the core structure of the cache. Its text, and what the review +found about it, are in #188. The number is kept so that references to D11 and +D12 stay valid. + +What this set still does for diffs: + +- `build_compare_expr` drops `__row_order` from both sides before joining + (decision D6 of `plans/ADR-008-row-order-of-reads.md`). +- Both sides are read through `cached_result_expr`, so both files exist before + the join is composed (D5). A diff is the one consumer that needs two files + at once, which makes it the natural test of D5. +- A promoted diff is an ordinary worthy entry. It contains a join, so it is + materialized by D4 like any other. + +Until #188 lands the live diff works as it does today. It posts an +unmaterialized join to Buckaroo (`_build_compare_expr`, `app.py:418-441`), +which is the known exception recorded under the governing rule. ### D11. One write at a time per project @@ -472,6 +508,24 @@ finding that two builds of one entry can end with the failing one deleting the winner's directory (`build.py:447-468`, `618-627`). FastMCP runs tool calls on a thread pool, so parallel tool calls did build at once. They now queue. +Three limits, recorded here so that the implementation does not have to +discover them: + +- Re-entrant has to mean per thread. `_project_lock` takes `flock` on a fresh + file descriptor, so a nested acquire in one process blocks forever. The + companion also moves work between threads with `run_in_threadpool`, so the + lock cannot be held across an `await`. +- Whether a recalc takes the lock once per build or once for the whole walk is + left to the implementation. Either is correct. +- The lock covers writes only. Concurrent reads on the shared default backend + fail with `RuntimeError: Already borrowed`. That is #118, and this ADR does + not change it. + +The lock is blocking and has no timeout, and the work it now covers is long: a +materialization runs single-partition and twice (ADR-009 decisions D1 and D6). +A page request that needs a heal therefore waits behind any build in the other +process. Paddy, 2026-09-20: correct first. #186 tracks the waiting. + Two tallyman servers pointed at one project is unsupported. Paddy: "you have done something diabolical and deserve the results." The file lock would still serialize their writes on one machine, and nothing else about them is safe, @@ -480,31 +534,39 @@ buckaroo-data/tallyman#183 tracks detecting that case and refusing to start. ### D12. Files are deleted only by an explicit user action -Nothing deletes a materialized file on its own, and nothing rewrites one except -opening an entry that needs it (D5). There is no disk budget yet. That is the -rewrite of `plans/ADR-003-result-cache-cost-rubric.md`, deferred. - -- The startup warm-up stops calling `cached_result_expr`. Today it rewrites - every snapshot that was deleted, which undoes the Cache page's delete button - and can block startup on one large file. +Nothing deletes a materialized file on its own, and nothing writes one +speculatively. A file is written only because something is about to read it: a +read of the entry, or a build or a read of an entry whose plan reads its file +(a page request, a chart, chaining at mint time, a recalc). Those are D5's +callers. There is no disk budget yet. That is the rewrite of +`plans/ADR-003-result-cache-cost-rubric.md`, deferred. + +- The startup warm-up stops calling `cached_result_expr`. Today it heals + deleted snapshots until a 3 s budget is spent, and the budget is checked only + between entries (`app.py:807-824`). That undoes the Cache page's delete + button, and one large heal blocks startup for as long as it takes. +- The verify sweep reads and never writes (D5). - An explicit delete skips an entry marked not reproducible (decision D6 of - `plans/ADR-009-digest-stability.md`), whose file cannot be recreated. -- Ephemeral diff entries (D10) follow the same rule and are not collected - automatically. + `plans/ADR-009-digest-stability.md`), whose file cannot be recreated. The + skip protects the file from the Cache page only. `compute_cache/` as a whole + is still deletable by definition (D7), and where such a file should live is + part of #185. ## Testing -Tests marked *red* fail on `main` today and belong in the failing-tests commit. -The others cannot fail before the code exists and ride with the change. +Every test below goes in the failing-tests commit and is seen red on CI before +the change lands (D9, step 1). A test of a function that does not exist yet +fails on import, and that counts as red. An earlier draft let such tests ride +with the change. Paddy, 2026-09-20: do normal TDD. -- **Sentinel** (*red*, D8). With `XORQ_CACHE_DIR` pointing at an empty +- **Sentinel** (D8). With `XORQ_CACHE_DIR` pointing at an empty directory, a build, a chained child build, a view, a delete and a reopen leave that directory empty. -- **Concurrent builds** (*red*, D11). Two threads building the same entry both +- **Concurrent builds** (D11). Two threads building the same entry both return it, and the entry's directory is intact afterwards. -- **Forgotten session** (*red*, D6). After Buckaroo has dropped a session, +- **Forgotten session** (D6). After Buckaroo has dropped a session, reopening the entry yields a grid that loads. -- **Warm-up leaves deleted files alone** (*red*, D12). A snapshot deleted before +- **Warm-up leaves deleted files alone** (D12). A snapshot deleted before startup is still absent after startup with no requests made. - **Identity** (D3). A child's hash changes when its parent's snapshot path changes and not when the file's bytes do, and a filter over an aggregate's @@ -516,11 +578,15 @@ The others cannot fail before the code exists and ride with the change. entry needs is reproduced with its recorded digest. - **Handoff** (D6). A worthy entry's grid is posted a view build of its snapshot, and Buckaroo writes nothing under `compute_cache/result_cache/`. -- **Diffs** (D10). Opening a diff builds an entry under - `compute_cache/ephemeral_entries/` before Buckaroo is called, the checkpoint - does not zip it, and promoting it keeps the same content hash. -- **Explicit delete** (D12). Deleting a snapshot ends the sessions that read it, - and the next open rewrites it. +- **Two files at once** (D5, D10). With both sides' snapshots deleted, + composing a diff rewrites and verifies both before the join is built, and the + diff carries no row-order column from either side. +- **Explicit delete** (D12). After a snapshot is deleted, the next open + rewrites it and verifies it before Buckaroo is called. +- **The sweep writes nothing** (D5, D12). With a snapshot deleted, a verify + sweep reports the entry as absent, and the file is still absent afterwards. +- **Forced reload** (D6). After an unfaithful heal, Buckaroo receives a + `/load_expr` for that entry's session id with `force_reload` set. ## Consequences @@ -529,7 +595,9 @@ The others cannot fail before the code exists and ride with the change. `manifest.snapshot_key` and `_assert_recorded_snapshot_key`; the `baked` / `recompute` plan split keyed on a `CachedNode`; the thread-only `_heal_lock` and the `(FileNotFoundError, ValueError)` retry around xorq's shared temp - file; `entry_graph_expr` as a separate function. + file; `entry_graph_expr` as a separate function; tallyman's record of + Buckaroo sessions (`_sessions`, `~/.tallyman/buckaroo_sessions.json`) and + `evict_session`, which worked by dropping an entry of it (D6). - **ADR-006:** its D4 (inlined chaining) and D8 (snapshot-key tripwire) are superseded. Its D2's "snapshot path derived from the loaded expression" becomes `snapshot_path`. Its D3 (rebind composition onto the default backend) @@ -562,7 +630,10 @@ The others cannot fail before the code exists and ride with the change. become unnecessary. Not tested. - **Cost accepted:** a worthy parent's snapshot must exist before a child can be built. A build also no longer repairs its own ancestors when something - outside tallyman executes it, and under the governing rule nothing does. + outside tallyman executes it, and under the governing rule nothing does. A + tab open on an entry whose file the user deletes errors until the entry is + reopened (D6). A missing ordered copy of a source surfaces as a Buckaroo + error (D5). ## Open questions @@ -582,10 +653,20 @@ The others cannot fail before the code exists and ride with the change. open question 1 is answered with one kind of entry. 3. **Where ordered copies of sources live.** ADR-008 decision D2 adds one per parquet source. `csv_ordered/` is global today, is never collected, and is - not included by `tallyman pack`. If every entry is materialized, a root - entry's own file could serve as the ordered copy. + not included by `tallyman pack`. Its path is also outside the project root, + so `make_portable_inplace` does not rewrite it and a CSV entry's build is + not portable. The fix for #168 proposes keeping a CSV's ordered copy under + the project, next to the content-addressed clone it is built from, which + would settle this for CSVs. If every entry is materialized, a root entry's + own file could serve as the ordered copy. + +Closed in the grilling session: eviction policy (D12). -Closed in the grilling session: an unfaithful parent's descendants (ADR-009 +Reopened in review and moved out of this set: an unfaithful parent's +descendants. The grilling session closed it on the grounds that ADR-009 decision D6 finds a non-reproducible entry when it is created and pins its -file, so it is never rewritten underneath its descendants); eviction policy -and the collection of ephemeral entries (D12). +file. The pin holds against the Cache page only, the file lives under +`compute_cache/`, which D7 defines as deletable, and an entry built on a +non-reproducible parent is itself recorded as reproducible, because both of +its runs read the same parent file. #185 has it. The collection of ephemeral +diff entries went to #188 with D10. diff --git a/plans/ADR-008-row-order-of-reads.md b/plans/ADR-008-row-order-of-reads.md index 3ab6b25..6cdae2d 100644 --- a/plans/ADR-008-row-order-of-reads.md +++ b/plans/ADR-008-row-order-of-reads.md @@ -1,7 +1,9 @@ # ADR: Row order of reads (every file carries `__row_order`, every page sorts by it) - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling - session). Awaiting Paddy's review; nothing here is implemented. The first draft pinned row order with an engine setting. Paddy + session, and again the same day after a review of PR #184, which made the + fix for #168 a precondition of D2 and D7). Awaiting Paddy's review; nothing + here is implemented. The first draft pinned row order with an engine setting. Paddy proposed baking a row-order column into every file tallyman writes and sorting every page by it. The measurements below favour that, so it is now the decision and the engine setting is the rejected alternative under D5. @@ -10,10 +12,15 @@ threshold quoted in `plans/ADR-006-read-path-loads-builds.md` decision D5 (the canonical sort) and three other places. - **Context:** the 2026-09-18 cache audit (tallyman @ `a748ea6`, buckaroo - 0.15.4, xorq 0.3.26, xorq-datafusion 0.2.7). No ticket filed yet. + 0.15.4, xorq 0.3.26, xorq-datafusion 0.2.7). +- **Tickets:** #168 (CSV sources bypass source identity; D2 and D7 depend on + its fix), buckaroo-data/buckaroo#974 (Buckaroo's half of D5, see D8), #188 + (diffs, moved out of ADR-007). - **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`, `_tie_break_order`), `src/tallyman_xorq/io.py` (`read_project_file`, - `tallyman_read_csv`, `io.py:627`), `src/tallyman_xorq/result_cache.py` + `tallyman_read_csv`, `io.py:627`, and for #168 `_ordered_csv_key` and + `_ordered_csv_parquet`), `src/tallyman_xorq/source_identity.py` (the three + steps a CSV now goes through), `src/tallyman_xorq/result_cache.py` (`_EXPENSIVE_OPS`, `classify_build`), `src/tallyman_companion/app.py` (`api_data`, `app.py:930`, and the chart data it feeds), `src/tallyman_xorq/primary_key.py` (candidate selection, @@ -26,10 +33,12 @@ (what a snapshot is, and the corpus rebuild this shares), `plans/ADR-009-digest-stability.md` (the file format, which D5 adds a requirement to). -- **Evidence:** `scripts/spike_row_order_paging.py` (the decisions) and +- **Evidence:** `scripts/spike_row_order_paging.py` (the decisions), `scripts/spike_window_read_order.py` (the problem, and the rejected - engine-setting approach). All figures are from those scripts on a 14-core - machine. + engine-setting approach), `scripts/spike_csv_source_identity.py` (D2 and D7: + what a CSV edit does to a content hash) and + `scripts/spike_deep_page_memory.py` (the memory figures under Consequences). + All figures are from those scripts on a 14-core machine. ## Terms @@ -135,12 +144,25 @@ Two writers produce it: positions in its own file. - **Ingest.** A source file enters tallyman through an ordered copy: polars scans it, `with_row_index` numbers the rows in file order, and the copy is - written with the column last. CSVs already work this way (the intermediate - parquet under `csv_ordered/`). Parquet sources gain the same step, keyed by - the source's digest, beside the content-addressed clone that stays the + written with the column last. The copy is keyed by the source's content and + written once. Both kinds of source go through source identity first + (`si.digest_for`, then `si.ensure_cas_path`, then `si.note_source`, the three + steps `read_project_file` performs for parquet today, `io.py:78-84`), and the + ordered copy is built from the content-addressed clone, which stays the immutable input. A source that already has a `__row_order` column has it overwritten, which is the right outcome for a file tallyman exported. + CSVs have the ordered-copy step today and not the keying. The first draft of + this decision said they already worked this way, which was wrong. The + intermediate under `csv_ordered/` is keyed by `md5(absolute path | schema | + reader options)` (`io.py:515-527`), it is overwritten in place when the CSV's + mtime changes (`io.py:558-582`), and `tallyman_read_csv` never touches source + identity. That is #168, and D7 cannot land before its fix. Building the copy + from the clone is enough: the key function already hashes the path, and the + clone's path carries the digest, which is the device of + `plans/ADR-002-source-identity-content-hash.md`. The clone never changes, so + the in-place overwrite becomes dead code and is deleted. + The canonical sort's tie-break (`_tie_break_order`) puts an inherited `__row_order` where `original_row_order` is today: after the author's own `order_by` keys and before the remaining columns. A worthy entry that keeps its @@ -290,8 +312,10 @@ semantics, in any engine and any process. every table would win the search for any table without a string or id key. Row positions shift between versions, so a diff keyed on it would be meaningless. -- `build_compare_expr` drops it from both sides before joining. The diff is an - entry (ADR-007 decision D10) and gets its own when it is materialized. +- `build_compare_expr` drops it from both sides before joining. A promoted + diff is a worthy entry, since it contains a join, and gets its own when it is + materialized. The live diff grid has none until #188 lands (diffs built as + entries, moved out of ADR-007), so its paging stays as it is today. ### D7. `tallyman_read_csv` loses its trailing `order_by`, and its column becomes `__row_order` @@ -301,13 +325,26 @@ and written last, so a CSV root has one row-order column and not two with identical values. The root entry becomes a cheap read of the intermediate, and D5 gives file-order pages with no Sort and no second copy. +This depends on the fix for #168 (D2). Today the trailing Sort is the only +thing that keeps a CSV root's rows fixed: it makes the root worthy, so a baked +snapshot freezes them, and a heal that read a changed intermediate would be +flagged. Without the Sort and without the fix, editing a CSV and re-running the +same recipe gives the same content hash, so no new version is created, and the +first version's frozen build returns the edited rows (`[10, 999, 30, 40]` where +it was built from `[10, 20, 30]`). With the ordered copy keyed on the clone, the +edit gives a new hash and the first version keeps its rows +(`scripts/spike_csv_source_identity.py`). The hash does not move today either, +with the Sort in place, so an edited CSV under an unchanged recipe produces no +new version. The fix for #168 corrects that as well. + What INV-2 provided, and what replaces it: | INV-2 gave | Replacement | | --- | --- | | A canonical display order | D5. INV-2 did not deliver this above 10 MB. | | A parquet boundary for chained children | A cheap root's graph is one read node. | -| A `result_digest` on the root, so a re-parse that produced different rows would be caught | Lost as it stands: cheap entries record no digest (ADR-006 decision D9, "no cheap-entry digests"). See open question 5. | +| Fixed rows under the root's hash, through its baked snapshot | The content-keyed ordered copy of D2, written once. Requires the fix for #168. | +| A `result_digest` on the root, so a re-parse that produced different rows would be caught | Lost as it stands: cheap entries record no digest (ADR-006 decision D9, "no cheap-entry digests"). It matters only when an ordered copy is deleted and re-created. See open question 5. | Every hash in every CSV lineage changes, so this rides the corpus rebuild of ADR-007 decision D9 ("one change, one rebuild"). @@ -341,13 +378,21 @@ its aggregate is), and `src/tallyman_xorq/source_cache.py:98`. ## Testing -Tests marked *red* fail on `main` today and belong in the failing-tests commit. -The others cannot fail before the code exists and ride with the change. - -- **Repeatable pages** (*red*, D5). Eight identical `/api/data` requests at a - deep offset into an entry whose file is larger than 10,485,760 bytes return - identical rows, for a worthy entry and for a cheap one. The sorted case is - Buckaroo's code and is tested there (buckaroo-data/buckaroo#974). +Every test below goes in the failing-tests commit and is seen red on CI before +the change lands (ADR-007 decision D9, the order of work). A test of a function +that does not exist yet fails on import, and that counts as red. Paddy, +2026-09-20: do normal TDD. + +- **Repeatable pages** (D5). Eight identical `/api/data` requests at a deep + offset into an entry whose file is larger than 10,485,760 bytes each return + exactly the rows at positions `offset` to `offset + limit - 1` in + `__row_order` order, for a worthy entry and for a cheap one. Asserting only + that the eight agree is not enough: one rejected setting returned the same + wrong page 8 times out of 8 (D5). The sorted case is Buckaroo's code and is + tested there (buckaroo-data/buckaroo#974). +- **An edited CSV forks the hash** (D2, #168). Editing a CSV and re-running the + same recipe gives a new content hash, and the earlier entry still returns the + rows it was built from. - **The column** (D2). Every file tallyman writes ends in `__row_order`, holding `0..N-1` with no gaps, and a materialization replaces an inherited one. - **Dropping it fails the build** (D3). A cheap recipe whose select list omits @@ -379,10 +424,18 @@ The others cannot fail before the code exists and ride with the change. applies to them. - Unions, distincts and unnests are materialized, which is the price of having a defined row order. -- Each parquet source costs one ordered copy, about the size of the source. +- Each source costs one ordered copy, about the size of the source. A CSV + source also gains a content-addressed clone of the CSV, which is + copy-on-write where the filesystem offers it. - An unsorted page costs a sort of one column unless the file's order is declared (100 to 374 ms against 25 to 242 ms in the spike). A range request costs about 20 ms at any depth. +- A sorted page also holds more in memory the deeper it is. On the spike's + file (336 MB as Arrow) peak process memory was 626 MB at offset 0, 1,142 MB + at 1,000,000 and 1,176 MB at 2,900,000, against 300 MB for a bare limit and + 248 MB for a range request (`scripts/spike_deep_page_memory.py`). The corpus + holds a 3.68 GB snapshot. Paddy, 2026-09-20: a performance matter, taken up + after correctness. The range request is the path with bounded memory. - The grid stays unstable until Buckaroo's half (buckaroo-data/buckaroo#974) lands: above 10 MB when unsorted, and at any size when sorted by a column with ties. @@ -403,7 +456,12 @@ The others cannot fail before the code exists and ride with the change. 4. **Deep offsets on a cheap entry.** Positions in a filtered view have gaps, so it pages with `OFFSET`, whose cost grows with depth. 5. **A digest for an ordered copy.** An ordered copy is the record of a parse - and is re-created from the clone if deleted. Recording its digest in the root - entry's manifest, and verifying it on re-creation, would restore what D7 - gives up. It wants ADR-009's digest definition, and it touches where the - copies live: `csv_ordered` is global, is never collected, and is not packed. + and is re-created from the clone if deleted. With the copy keyed by content + (D2) a given path always holds the same parse, so what is left unverified is + a re-creation, for example after a polars upgrade that parses differently. + Recording the copy's digest in the root entry's manifest, and verifying it + on re-creation, would cover that. It wants ADR-009's digest definition, and + it touches where the copies live: `csv_ordered` is global, is never + collected, and is not packed (ADR-007 open question 3). Nothing re-creates a + missing copy automatically yet, which ADR-007 decision D5 (one entry point + makes files exist) records as an accepted gap. diff --git a/plans/ADR-009-digest-stability.md b/plans/ADR-009-digest-stability.md index 0b34f87..1003329 100644 --- a/plans/ADR-009-digest-stability.md +++ b/plans/ADR-009-digest-stability.md @@ -3,7 +3,9 @@ - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling session: D3 gains two format requirements from `plans/ADR-008-row-order-of-reads.md`, D1 lost its speed gate, and D6 is - new). Awaiting Paddy's review; nothing here is implemented. Amends + new; and again the same day after a review of PR #184: D1 and D3 now say + what single-partition execution leaves undetermined, and D6's cheap-entry + half moved to #185). Awaiting Paddy's review; nothing here is implemented. Amends `plans/ADR-004-result-digest-canonical-ordering.md` (Option A's "hash the snapshot bytes") and decision D5 of `plans/ADR-006-read-path-loads-builds.md` (the canonical sort), which said @@ -13,7 +15,9 @@ always means this ADR's own decision. Another ADR's decision is always written with its ADR number and a few words saying what it decides. - **Context:** the 2026-09-18 cache audit (tallyman @ `a748ea6`, xorq 0.3.26, - xorq-datafusion 0.2.7, pyarrow 21.0.0). No ticket filed yet. + xorq-datafusion 0.2.7, pyarrow 21.0.0). +- **Tickets:** #187 (a float total depends on the parent file's layout; D1 and + D3), #185 (non-pure recipes, which D6 only partly covers). - **Affected code:** `src/tallyman_xorq/result_cache.py` (`snapshot_file_digest`, `verify_result_faithful`, `_verify_self_heal`), `src/tallyman_xorq/build.py` (the execute-once step), @@ -23,11 +27,14 @@ introduces (one writer for snapshots, used by the build and by every heal). D2 and D3 assume that writer. If ADR-007 were rejected, D1 would stand as written and D2 would need restating against xorq's writer. -- **Related ADRs:** `plans/ADR-008-row-order-of-reads.md` (uses the same - single-partition setting for a different job, and has an open question this +- **Related ADRs:** `plans/ADR-008-row-order-of-reads.md` (its first draft used + the same single-partition setting for page reads, and it now rejects that; + its open question 5, a digest for an ordered copy of a source, is one this digest would answer). - **Evidence:** `scripts/spike_float_aggregate_digest.py`, - `scripts/spike_logical_digest.py`. + `scripts/spike_logical_digest.py`, and + `scripts/spike_float_layout_digest.py` (D1 and D3: what the layout of the + parent file does to a float total). ## Terms @@ -106,11 +113,12 @@ rebuilt. `materialize` (ADR-007 decision D4, the one writer of snapshots) executes on a connection configured with -`SET datafusion.execution.target_partitions = 1`. The build and every heal use -it, so both run one plan with one merge order, on any machine. It is a -different connection object from ADR-008's window connection, with the same -setting, because a long materialization must not share a context with page -reads. +`SET datafusion.execution.target_partitions = 1` and an explicit +`datafusion.execution.batch_size`. The build and every heal use it, so both run +one plan with one merge order, on any machine, given input files with the same +layout (see "What this does not fix" below). It is a separate connection from +the default backend that serves page reads, because a long materialization +must not share a context with them. Cost: about 3x on the spike's aggregate. ADR-004 measured a 3.1M-group aggregate at 0.5 s parallel against 3.4 to 3.9 s single-partition, and a full @@ -130,6 +138,28 @@ single-partition only for a plan with a floating-point reduction or window, decided once at build from the expression and recorded in the manifest so that a heal never re-derives it. +**What this does not fix.** A single stream fixes the order in which partial +results are merged. It does not make a float total a function of the rows +alone. With one partition and the same rows in the same order, an ungrouped +float `SUM` or `AVG` took three different bit patterns across four copies of +one file that differed only in row-group size (1,048,576, 777,777, 100,000 and +8,192 rows), each stable run to run (`scripts/spike_float_layout_digest.py`). +The variable is association, meaning where the running total is cut into +sub-sums. The likely mechanism, not confirmed in DataFusion's source, is that +the ungrouped accumulator sums each record batch as a block while batch +boundaries follow row-group boundaries. Sorting the input first changes +nothing, because DataFusion removes the sort: the plan is +`AggregateExec <- DataSourceExec` with or without it. A grouped aggregate, +`GROUP BY` a constant key, and the window form `SUM(v) OVER ()` were all +independent of the layout, so an ibis percent-of-total is not affected. Only a +true ungrouped reduction is exposed. + +So the layout of every file an entry reads is part of what makes its digest +reproducible. Tallyman writes all of those files (snapshots through D3, and +sources through the ordered copies of ADR-008 decision D2), which is why +pinning the layout is enough. D3 pins it. #187 tracks the rest: confirming the +mechanism, other reductions, and results across machines. + *Rejected:* round floats before hashing. A value next to a rounding boundary flips under one unit of noise in the last place, and among millions of values some are. @@ -212,8 +242,7 @@ ADR-008 adds two requirements. The writer numbers the rows in a last column named `__row_order` (ADR-008 decision D2). And it writes a parquet page index, which is what lets a page be fetched as a range of `__row_order` values without decoding a whole row group: 19 to 24 ms at any depth with the index against 79 -to 90 ms without it, on a 287 MB file (`scripts/spike_row_order_paging.py` on -the ADR-008 branch). +to 90 ms without it, on a 287 MB file (`scripts/spike_row_order_paging.py`). The entry's recorded schema (`schema.json`) is read from the written file and not from the expression. The file is what every consumer reads, the writer @@ -222,18 +251,28 @@ types on the way in: a `timestamp[s]` column comes back as `timestamp[ms]`. Combining each row group keeps the file bytes reproducible as well. Nothing depends on that after D2, and it means two writes of the same entry can still -be compared with `cmp` when debugging. Because of D2 these settings can change later without -touching any digest. +be compared with `cmp` when debugging. + +Because of D2 the codec, the compression level and the statistics can change +later without touching this entry's digest. The row-group size cannot. It +decides the batch boundaries that an entry built on this file sees, and an +ungrouped float total depends on them (D1). An earlier draft said every +setting here was free to change. The row-group size and the materialization +connection's `batch_size` are therefore part of the reproducibility contract: +the manifest records a snapshot format version that stands for both, and +changing either is a corpus rebuild. ### D4. A mismatch record names its likely cause With D1 and D2 in place the causes left are the recipe's own nondeterminism (`sample()`, `now()`, an impure UDF), source drift under `off` identity mode, and an engine upgrade that changed results. The manifest records the xorq, -xorq-datafusion and pyarrow versions at build, and the `unfaithful_heal` record -carries them alongside the versions at heal. When they differ, the message says -so instead of blaming the recipe. The contract's attribution table gains that -row. +xorq-datafusion and pyarrow versions at build, together with the snapshot +format version (D3), and the `unfaithful_heal` record carries them alongside +the values at heal. When they differ, the message says so instead of blaming +the recipe. The contract's attribution table gains that row. One cause has no +attribution yet: a parent that is not reproducible was rewritten, so every +entry built on it heals to different rows. That belongs to #185. ### D5. It lands with the rebuild @@ -245,10 +284,13 @@ decision D9 (one change, one rebuild). The manifest field keeps its name. Decided by Paddy in the grilling session (2026-09-20): "call the same query twice... put some tests around this." -Today tallyman learns that an entry is not reproducible only when a deleted -file is rewritten and its digest differs. By then the original rows are gone, -and everything built on them disagrees with the new file without anyone -knowing. So the check moves to the moment the entry is created: +Today the build lint from #88 (`_nondeterminism_warnings`, `build.py`) warns +when a recipe uses one of five known non-pure operations, and nothing records +its verdict. Beyond that warning, tallyman learns that an entry is not +reproducible only when a deleted file is rewritten and its digest differs. By +then the original rows are gone, and everything built on them disagrees with +the new file without anyone knowing. So a check moves to the moment a +materialized entry is created: - `materialize` runs the entry's query twice through the same writer, on the same single-partition connection (D1), and compares the two content digests @@ -261,16 +303,27 @@ knowing. So the check moves to the moment the entry is created: the UI, and the build result tells the author so, with the columns whose digests differed. This is the state ADR-006 decision D12 (unfaithful entries are pinned and badged) reaches after the damage is done. D6 reaches it first. -- A cheap entry gets the same check. Its two runs are streamed and digested - without writing a file. If they differ, the entry is materialized and pinned - like any other non-reproducible entry, because re-running it on every read - would show different values each time. A computed column that calls - `random()` is row-preserving, so it is classed cheap today, and nothing - detects it: ADR-006 decision D9 ("no cheap-entry digests") assumed a frozen - graph over pinned inputs always gives the same rows. - -The cost is a second execution of every create. It is accepted under the -priority recorded in ADR-007 ("a cohesive system that works reliably" first). +- A cheap entry is not checked here. An earlier draft ran it twice as well, and + materialized and pinned it when the runs differed. That made a third kind of + entry, a graph the classifier calls cheap with a file the manifest says + exists, and whether an entry has a file stopped being a function of its + graph, which decisions D2, D3 and D5 of + `plans/ADR-007-tallyman-owned-materialization.md` all rely on. Paddy moved + it to #185 on 2026-09-20. Until that is settled a cheap entry that calls + `random()` behaves as it does today: the #88 lint warns, and the entry + re-runs on every read. + +The cost is a second execution of every create of a materialized entry. It is +accepted under the priority recorded in ADR-007 ("a cohesive system that works +reliably" first). + +Running twice has two limits, both part of #185. It cannot see `today()`, +since both runs agree; the #88 lint does. And it cannot see what an entry +inherits: an entry built on a non-reproducible parent reads the same parent +file in both runs and is recorded as reproducible, which holds only while that +file survives. The pin protects the file from the Cache page's delete and from +nothing else, since `compute_cache/` is deletable by definition (ADR-007 +decision D7, the cold state is an empty `compute_cache`). A heal is still verified against the recorded digest, as now. After D6 a mismatch there means something changed underneath a reproducible entry, which @@ -278,10 +331,12 @@ D4 attributes. ## Testing -Tests marked *red* fail on `main` today and belong in the failing-tests commit. -The others cannot fail before the code exists and ride with the change. +Every test below goes in the failing-tests commit and is seen red on CI before +the change lands (ADR-007 decision D9, the order of work). A test of a function +that does not exist yet fails on import, and that counts as red. Paddy, +2026-09-20: do normal TDD. -- **Float aggregate reproduces** (*red*). An entry with a float `SUM` and `AVG` +- **Float aggregate reproduces** (D1). An entry with a float `SUM` and `AVG` over a source large enough to run in parallel is created, its file deleted, and the entry reopened, three times. Every rewrite must match the recorded digest, and no `unfaithful_heal` record may be written. @@ -295,8 +350,9 @@ The others cannot fail before the code exists and ride with the change. reproducible, names the offending column, and has a pinned file. - **Create passes a reproducible recipe.** A deterministic recipe is recorded as reproducible, and the query is observed to run exactly twice. -- **A non-reproducible cheap recipe is materialized.** A computed column that - calls `random()` ends up with a pinned file instead of re-running per read. +- **The layout is pinned** (D1, D3). Every snapshot has row groups of 1,048,576 + rows, the materialization connection reports the pinned `batch_size`, and + the manifest records the snapshot format version. - **A pinned file survives an explicit delete.** The Cache page's delete skips it and says why. - **The schema comes from the file.** An entry with a `timestamp[s]` column @@ -311,8 +367,10 @@ The others cannot fail before the code exists and ride with the change. - Materializations and heals are slower, by about 3x on aggregation at spike scale and up to 7x in ADR-004's parking measurement, and a create runs its query twice (D6). Reads are unaffected. -- A recipe that is not reproducible is known to be so from the moment it is - created, and its file is never rewritten underneath the entries built on it. +- A materialized entry whose recipe is not reproducible is known to be so from + the moment it is created, and the Cache page's delete leaves its file alone. + What that does not yet cover (a cheap entry, an entry that inherits the + problem from its parent, a file lost with `compute_cache/`) is #185. - Verify decodes the file instead of hashing its bytes. It runs on a heal and in `catalog_scan_staleness(verify_results=True)`, never on a read. - Snapshots are smaller, and their footers were 85 times smaller in the spike, @@ -320,8 +378,10 @@ The others cannot fail before the code exists and ride with the change. - `docs/system-contract.md` changes in "Result digest" and in the manifest table ("SHA-256 of the baked result snapshot"), and its verification table gains the engine-change row. -- The same function can digest the CSV intermediate (ADR-008, open - question 1). +- The same function can digest an ordered copy of a source (ADR-008, open + question 5). +- The row-group size and the materialization connection's `batch_size` are + frozen for the life of the corpus, and changing either is a rebuild (D3). ## Open questions @@ -330,3 +390,8 @@ The others cannot fail before the code exists and ride with the change. 2. **Engine upgrades.** Single-partition execution fixes the merge order within one engine version. Nothing guarantees float results across versions. D4 attributes that case and the remedy stays a rebuild. +3. **What else decides a float's low bits.** D1 and D3 pin the two variables + found so far, the merge order and the batch boundaries. #187 tracks the + mechanism, other reductions (variance, correlation, window frames), a filter + between the scan and the aggregate, and results across machines and CPU + architectures. diff --git a/scripts/spike_csv_source_identity.py b/scripts/spike_csv_source_identity.py new file mode 100644 index 0000000..423b65f --- /dev/null +++ b/scripts/spike_csv_source_identity.py @@ -0,0 +1,77 @@ +"""ADR-008 evidence (D2, D7) and #168: is a CSV root's ordered intermediate fixed under a content hash? + +``tallyman_read_csv`` reads a CSV through an ordered parquet intermediate. That file's key is +``md5(absolute path | schema | reader options)``, not content, and it is overwritten in place when the CSV's mtime +changes. The script edits a CSV, re-runs the identical recipe, and reports the build hash before and after, and what +the first version's frozen build returns afterwards, for three shapes of the root expression: + +1. today: a read of the intermediate with a trailing ``order_by("original_row_order")``; +2. ADR-008 D7 without the fix for #168: a plain read of the same intermediate (no sort, so no snapshot, no digest); +3. the fix for #168: digest the CSV, clone it to a content-named path, and key the intermediate on the clone. + +Raw xorq builds, no tallyman build machinery, so case 1 reports the hash only: in a real build the root is worthy +because of its Sort, and its baked snapshot goes on serving the old rows. An unchanged hash also means +``build_and_persist`` returns the existing entry, so no new version is created at all. + + uv run python scripts/spike_csv_source_identity.py +""" + +from __future__ import annotations + +import os +import tempfile +import time +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_csv_identity_")) +os.environ["TALLYMAN_HOME"] = str(HOME) # the ordered intermediates live under it; set before tallyman is imported + +import xorq.api as xo # noqa: E402 +from xorq.ibis_yaml.compiler import build_expr, load_expr # noqa: E402 + +from tallyman_xorq import source_identity as si # noqa: E402 +from tallyman_xorq.io import _ordered_csv_parquet, tallyman_read_csv # noqa: E402 + +BUILDS = HOME / "builds" +CAS = HOME / "cas" +ORIGINAL = "id,amount\n1,10\n2,20\n3,30\n" +EDITED = "id,amount\n1,10\n2,999\n3,30\n4,40\n" + + +def today(csv: Path): + return tallyman_read_csv(str(csv)) + + +def plain_read(csv: Path): + return xo.deferred_read_parquet(str(_ordered_csv_parquet(str(csv), None, {}))) + + +def keyed_on_clone(csv: Path): + clone = CAS / f"{si._digest_file(csv)}{csv.suffix}" + if not clone.exists(): + clone.write_bytes(csv.read_bytes()) # tallyman's ensure_cas_path makes a copy-on-write clone here + return xo.deferred_read_parquet(str(_ordered_csv_parquet(str(clone), None, {}))) + + +def main() -> None: + CAS.mkdir() + csv = HOME / "sales.csv" + cases = ( + ("1. today (trailing order_by kept)", today, False), + ("2. ADR-008 D7 without the #168 fix", plain_read, True), + ("3. keyed on a content-named clone", keyed_on_clone, True), + ) + for label, root, reread in cases: + csv.write_text(ORIGINAL) + v1 = Path(build_expr(root(csv), builds_dir=BUILDS)) + time.sleep(0.05) # so the edit changes the CSV's mtime + csv.write_text(EDITED) + v2 = Path(build_expr(root(csv), builds_dir=BUILDS)) + print(f"{label}: hash before the edit {v1.name}, after {v2.name}, same hash: {v1.name == v2.name}") + if reread: + rows = load_expr(v1).execute().amount.tolist() + print(f" V1's frozen build, re-read after the edit: {rows} (built from {[10, 20, 30]})") + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_deep_page_memory.py b/scripts/spike_deep_page_memory.py new file mode 100644 index 0000000..edb3d6e --- /dev/null +++ b/scripts/spike_deep_page_memory.py @@ -0,0 +1,77 @@ +"""ADR-008 evidence (Consequences): what a page request holds in memory at depth. + +ADR-008 D5 serves every page as ``ORDER BY __row_order LIMIT n OFFSET k``. Its cost table measures latency. This +script measures peak process memory for the same requests, on the same file shape as ``spike_row_order_paging.py``, +and for a range request (``__row_order >= k AND __row_order < k + n``), which ADR-008 notes as a later optimization. + +Each case runs in a fresh process, so its peak is its own; the first case imports xorq and does nothing, as the floor. + + uv run python scripts/spike_deep_page_memory.py +""" + +from __future__ import annotations + +import resource +import subprocess +import sys +import tempfile +from pathlib import Path + +ROW = "__row_order" +N = 3_000_000 +CASES = ( + "import only", + "bare limit 50", + "sorted 0", + "sorted 1000000", + "sorted 2900000", + "range 2900000", + "chart 100000", +) + + +def run_case(path: str, case: str) -> None: + import xorq.api as xo + + kind, _, arg = case.partition(" ") + if kind != "import": + t = xo.connect().read_parquet(path) + if kind == "bare": + t.limit(50).execute() + elif kind == "sorted": + t.order_by(ROW).limit(50, offset=int(arg)).execute() + elif kind == "range": + k = int(arg) + t.filter((t[ROW] >= k) & (t[ROW] < k + 50)).order_by(ROW).execute() + elif kind == "chart": + t.order_by(ROW).limit(int(arg)).execute() # the chart pull through /api/data + peak = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + peak_mb = peak / 1e6 if sys.platform == "darwin" else peak / 1e3 # bytes on macOS, kilobytes on Linux + print(f" {case:16s} peak process memory {peak_mb:7.0f} MB") + + +def main() -> None: + import numpy as np + import pyarrow as pa + import pyarrow.parquet as pq + + rng = np.random.default_rng(9) + cols = {"g": rng.integers(0, 200, N)} + cols |= {f"v{i}": rng.random(N) for i in range(12)} + cols[ROW] = np.arange(N) + table = pa.table(cols) + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "snapshot.parquet" + pq.write_table(table, path, compression="zstd", row_group_size=1_048_576, write_page_index=True) + on_disk, as_arrow = path.stat().st_size / 1e6, table.nbytes / 1e6 + print(f"file: {on_disk:.0f} MB on disk, {as_arrow:.0f} MB as Arrow, {N:,} rows x {table.num_columns} columns") + for case in CASES: + out = subprocess.run([sys.executable, __file__, str(path), case], capture_output=True, text=True) + print(out.stdout.rstrip() or out.stderr.strip()[-300:]) + + +if __name__ == "__main__": + if len(sys.argv) == 3: + run_case(sys.argv[1], sys.argv[2]) + else: + main() diff --git a/scripts/spike_float_layout_digest.py b/scripts/spike_float_layout_digest.py new file mode 100644 index 0000000..21b0bc3 --- /dev/null +++ b/scripts/spike_float_layout_digest.py @@ -0,0 +1,83 @@ +"""ADR-009 evidence (D1, D3) and #187: what single-partition execution leaves undetermined in a float aggregate. + +``target_partitions = 1`` removes the run-to-run drift of a float aggregate. This script asks whether the result is +then a function of the rows alone. It writes the same rows, in the same order, as four parquet files that differ only +in row-group size, and bit-compares several query shapes across them on a single-partition connection: + +* ``A`` an ungrouped ``SUM`` / ``AVG``; +* ``B`` the same over a subquery sorted by row position, to test whether ordering the addends helps; +* ``C`` ``GROUP BY`` a literal key; +* ``D`` ``GROUP BY`` a single-valued key the optimizer cannot fold (``ro - ro``); +* ``E`` the window form ``SUM(v) OVER ()``, which is what an ibis percent-of-total compiles to. + +Every shape is stable run to run on one file. Shape A, and B with it, differs between files. The physical plan +printed for B shows why sorting changes nothing: DataFusion removes the sort, since ``SUM`` needs no ordered input. +The variable is association (where the running total is cut into sub-sums), not the order of the addends. + + uv run python scripts/spike_float_layout_digest.py +""" + +from __future__ import annotations + +import struct +import tempfile +from pathlib import Path + +import numpy as np +import pyarrow as pa +import pyarrow.parquet as pq +import xorq.api as xo + +N = 3_000_000 +ROW_GROUP_SIZES = (1_048_576, 777_777, 100_000, 8_192) +SHAPES = { + "A ungrouped": "SELECT SUM(v) s, AVG(v) m FROM {t}", + "B sorted first": "SELECT SUM(v) s, AVG(v) m FROM (SELECT * FROM {t} ORDER BY ro)", + "C group by literal": "SELECT SUM(v) s, AVG(v) m FROM (SELECT v, 1 AS k FROM {t}) GROUP BY k", + "D group by ro - ro": "SELECT SUM(v) s, AVG(v) m FROM (SELECT v, ro - ro AS k FROM {t}) GROUP BY k", + "E window SUM() OVER ()": "SELECT MIN(tot) s, MAX(tot) m FROM (SELECT SUM(v) OVER () AS tot FROM {t})", +} + + +def bits(value) -> str: + return struct.pack(" str: + plan = con.raw_sql("EXPLAIN " + sql).to_pandas() + physical = plan[plan.iloc[:, 0] == "physical_plan"].iloc[0, 1] + return " <- ".join(line.strip().split(":")[0] for line in physical.splitlines() if line.strip()) + + +def main() -> None: + rng = np.random.default_rng(5) + table = pa.table({"ro": np.arange(N), "v": rng.random(N) * 1e6}) + results: dict[str, dict[int, tuple[str, str]]] = {shape: {} for shape in SHAPES} + plans: dict[str, str] = {} + with tempfile.TemporaryDirectory() as tmp: + for size in ROW_GROUP_SIZES: + path = Path(tmp) / f"rows_rg{size}.parquet" + pq.write_table(table, path, row_group_size=size, compression="zstd") + con = xo.connect() + con.raw_sql("SET datafusion.execution.target_partitions = 1") + name = f"t_{size}" + con.read_parquet(str(path), table_name=name) + for shape, sql in SHAPES.items(): + runs = set() + for _ in range(2): + row = con.raw_sql(sql.format(t=name)).to_pandas().iloc[0] + runs.add((bits(row.s), bits(row.m))) + assert len(runs) == 1, f"{shape} is not stable run to run on row groups of {size:,}" + results[shape][size] = runs.pop() + plans.setdefault(shape, operators(con, sql.format(t=name))) + + print(f"{N:,} rows, the same order in every file; target_partitions = 1; row-group sizes {ROW_GROUP_SIZES}\n") + for shape, by_size in results.items(): + distinct = len(set(by_size.values())) + low_bytes = [by_size[size][0][:4] for size in ROW_GROUP_SIZES] + print(f"{shape:24s} distinct results across the four files: {distinct} SUM, lowest two bytes: {low_bytes}") + print(f"{'':24s} plan: {plans[shape]}") + + +if __name__ == "__main__": + main() From f777d067ba3693bc16a68acb2814f71f249906da Mon Sep 17 00:00:00 2001 From: Paddy Mullen Date: Sun, 20 Sep 2026 23:29:35 -0400 Subject: [PATCH 4/4] docs(plans): fold the second PR #184 review into the cache redesign ADRs Keep two kinds of entry (ADR-007 open question 1). ADR-007 gains D13 (a file is cache only if ensure_materialized can re-create it) and D14 (a reset leaves compute_cache alone), closes the ordered-copy gap in D5, and adds the klass hot-reload fix to D6. ADR-008 rewrites D4 as a three-part test for cheap, corrects D6's three-way join claim, and adds D10 (natural order on every order_by), D11 (hoist a non-final sort) and D12 (a raw parquet read is a build error). ADR-009 says how a loaded build gets onto the single-partition connection and extends the format version to ordered copies. Adds seven evidence scripts under scripts/. Items not yet confirmed by Paddy are marked as such in the ADR text. Co-Authored-By: Claude Sonnet 5 --- .../ADR-007-tallyman-owned-materialization.md | 303 ++++++++++++++--- plans/ADR-008-row-order-of-reads.md | 320 ++++++++++++++++-- plans/ADR-009-digest-stability.md | 74 +++- scripts/spike_cheap_classifier.py | 108 ++++++ scripts/spike_ordered_copy_layout.py | 100 ++++++ scripts/spike_reset_roundtrip.py | 93 +++++ scripts/spike_row_order_joins.py | 93 +++++ .../spike_single_partition_loaded_build.py | 98 ++++++ scripts/spike_sort_grafting.py | 139 ++++++++ scripts/spike_stream_order.py | 82 +++++ 10 files changed, 1324 insertions(+), 86 deletions(-) create mode 100644 scripts/spike_cheap_classifier.py create mode 100644 scripts/spike_ordered_copy_layout.py create mode 100644 scripts/spike_reset_roundtrip.py create mode 100644 scripts/spike_row_order_joins.py create mode 100644 scripts/spike_single_partition_loaded_build.py create mode 100644 scripts/spike_sort_grafting.py create mode 100644 scripts/spike_stream_order.py diff --git a/plans/ADR-007-tallyman-owned-materialization.md b/plans/ADR-007-tallyman-owned-materialization.md index 701518e..464a312 100644 --- a/plans/ADR-007-tallyman-owned-materialization.md +++ b/plans/ADR-007-tallyman-owned-materialization.md @@ -4,7 +4,10 @@ session, which added the governing rule, decisions D10 to D12, and the resolution recorded under D5, and again the same day after a review of PR #184: D10 moved out to #188, the verify sweep left D5's callers, D6's - session-ending clause was dropped, and D12's rule was restated). Awaiting + session-ending clause was dropped, and D12's rule was restated, and a third + time that day after a second review of PR #184: two kinds of entry were + confirmed (open question 1), D5 lost its accepted gap, D6 gained the klass + reload, and D13 and D14 are new). Awaiting Paddy's review; nothing here is implemented. Supersedes two decisions of `plans/ADR-006-read-path-loads-builds.md`: its D4 (chaining inlines the parent's cache node) and its D8 (the manifest records the snapshot key and @@ -24,7 +27,11 @@ - **Tickets:** #188 (diffs, moved out of this ADR), #186 (the waiting that D11's lock causes), #185 (non-pure recipes), #183 (two servers on one project), #168 (CSV source identity, which the shared rebuild of D9 needs), - #118 (concurrent reads on the shared backend, which D11 does not cover). + #118 (concurrent reads on the shared backend, which D11 does not cover), + #77 (an empty grid on a clone of the project at another path, which D2 + closes), #76 (closed: how bare-read chaining failed the last time it was + tried, which D5 answers), #22 (the checkpoint's cost grows with the cache, + which D14 removes). - **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`), `src/tallyman_xorq/result_cache.py` (`_resolve_result_plan`, `cached_result_expr`, `entry_graph_expr`, `baked_snapshot_path`, @@ -33,7 +40,11 @@ `src/tallyman_xorq/io.py` (`tracked_expr_from_alias`, `pinned_expr_from_alias`), `src/tallyman_xorq/portable.py` (`rewrite_cache_dirs`), `src/tallyman_core/manifest.py` (`snapshot_key`), - `src/tallyman_companion/buckaroo_lifecycle.py` (`load_session`), + `src/tallyman_companion/buckaroo_lifecycle.py` (`load_session`, + `reload_project_sessions`), `src/tallyman_core/catalog_state.py` + (`reset_to`, `prune_compute_cache`, `restore_from_bullpen`, + `capture_tallyman_state`, `_gc_cas_clones`), + `src/tallyman_xorq/source_identity.py` (`gc_cas`, `recon_cas_path`), `docs/system-contract.md`. - **Related ADRs:** `plans/ADR-002-source-identity-content-hash.md` (content in the path; D3 reuses the device), `plans/ADR-003-result-cache-cost-rubric.md` @@ -41,8 +52,11 @@ `plans/ADR-008-row-order-of-reads.md` and `plans/ADR-009-digest-stability.md` (the other two hash- or digest-changing decisions that share this ADR's corpus rebuild). -- **Evidence:** `scripts/spike_bare_read_chaining.py` (results under D3), and - the audit measurements quoted in the Problem section. +- **Evidence:** `scripts/spike_bare_read_chaining.py` (results under D3), + `scripts/spike_reset_roundtrip.py` (D13 and D14: a reset back and forward, + played through on today's code), `scripts/spike_stream_order.py` (open + question 1, the alternative that was not taken), and the audit measurements + quoted in the Problem section. ## Terms @@ -72,6 +86,17 @@ **promoted diff** is one that has been saved as a catalog entry. - **Checkpoint:** tallyman's step that zips new entries and makes one git commit in the catalog repository. +- **Clone:** the copy of a source file under `data/.cas/`, named by the digest + of the source's bytes, which a build reads in place of the live file + (`plans/ADR-002-source-identity-content-hash.md`). +- **Ordered copy:** the parquet copy of a source that carries `__row_order`, + which a root entry reads (decision D2 of + `plans/ADR-008-row-order-of-reads.md`). +- **Reset:** `reset_to`, which returns the catalog to an earlier step. The + **bullpen** is the directory a reset moves retired files into, so that a + later reset forward can bring them back. +- **Klass:** a summary stat, post-processing or display class written for the + project, which Buckaroo loads into each grid. ## Problem @@ -271,8 +296,12 @@ of every descendant would re-run the parent's expensive subgraph. record-batch stream, and writes the snapshot itself: - a unique temp name in the destination directory, then `os.replace`; -- under the project's write lock (D11), with the existence check repeated - inside the lock, so a second writer waits and then finds the file; +- under the project's write lock (D11). A heal repeats the existence check + inside the lock, so a second reader waits and then finds the file. A create + does not look: it always runs the query and replaces whatever is at the + path. That keeps an entry that is added again after a reset honest (D14), + and decision D6 of `plans/ADR-009-digest-stability.md` (create runs the + query twice) needs it; - it numbers the rows as it writes them, in a last column named `__row_order` (decision D2 of `plans/ADR-008-row-order-of-reads.md`); - it returns the digest of what it wrote. @@ -291,10 +320,12 @@ and keeps items 3 and 4. entry's plan reads is on disk before anything executes: 1. If the entry is worthy and its snapshot exists, return. No build is loaded. -2. Otherwise load the entry's build and collect the snapshot paths its `Read` - nodes point at (any read under `compute_cache/result_cache/`). The list is - kept with the loaded plan in the existing LRU. -3. For each of those that is missing, recurse on the hash in its file name. +2. Otherwise load the entry's build and collect every file its `Read` nodes + point at. The list is kept with the loaded plan in the existing LRU. +3. Re-create each of those that is missing, by the rule for its class (D13): a + snapshot by recursing on the hash in its file name, an ordered copy of a + source from its clone, a clone from the live source while the bytes still + match. 4. If the entry is worthy, `materialize` it. Every file it writes is verified against the manifest's `result_digest` before @@ -318,13 +349,19 @@ checked at the moment it next exists. A file that exists with the wrong digest is reported through the same loud path and left in place, since deleting it is the user's action (D12). -One gap is known and accepted. Step 2 collects only reads under -`compute_cache/result_cache/`. A root entry also reads an ordered copy of its -source (decision D2 of `plans/ADR-008-row-order-of-reads.md`), which lives -elsewhere and which this function does not re-create. If one is missing, -Buckaroo fails with `At least one path is required`. Paddy, 2026-09-20: -Buckaroo erroring when tallyman has not provided a prerequisite is acceptable -for now, and follow-on work closes it. +An earlier draft stopped at snapshots. Step 2 collected only reads under +`compute_cache/result_cache/`, and a missing ordered copy of a source +(decision D2 of `plans/ADR-008-row-order-of-reads.md`) surfaced inside +Buckaroo as `At least one path is required`. Paddy accepted that on +2026-09-20 as the case of someone deleting a directory by hand. The second +review played a reset through and found the same state reachable by an +ordinary reset back and forward, on today's code +(`scripts/spike_reset_roundtrip.py`, results under D13). After decision D7 of +`plans/ADR-008-row-order-of-reads.md` (a CSV root becomes a cheap read) every +page of a CSV-rooted cheap entry reads that copy, so the gap is closed here +and not left to follow-on work. When nothing can re-create a file, because the +clone is gone and the live source has changed, the error names the source +file. ADR-006 decision D4 rejected bare-read chaining because "builds stay non-self-contained and the pre-heal choreography stays load-bearing forever". @@ -336,6 +373,14 @@ through ordinary cache mechanics on first query." Under the governing rule Buckaroo never does that, so the property has no user and nothing is given up. This was question 1 of the grilling session, resolved 2026-09-20. +Bare-read chaining has failed here once before. #76 (closed) is a child +frozen on its parent's snapshot path and read after a reset had pruned that +snapshot, or on a clone of the project that never had it: the read died with +`At least one path is required` and nothing recomputed the parent. ADR-006 +decision D4 (chaining inlines the parent's cache node) was the answer then. +This function is the answer now, and the +"files exist before anything runs" test below is #76's reproduction. + The old pre-heal was also weaker than this function. It was a `cached_result_expr` call with a discarded result at one call site, and ancestors were healed only because reads then re-executed recipes. Here the @@ -403,6 +448,17 @@ no-op there. A promoted diff entry sends `column_config_overrides` and so reloads on every open, which #188 covers. Putting the project in the id also closes #172 (one project's session served to another on a hash collision). +One caller still needs to know which grids are open. When a klass is added or +changed, `reload_project_sessions` (`buckaroo_lifecycle.py:386`, four call +sites in `app.py`) posts `/reload_expr/` to every session of the +project, and it finds them in the record this decision deletes. Buckaroo +0.15.6 has no route that lists sessions. With derived ids none is needed: +tallyman posts `/reload_expr/` for each entry of the project and treats +the 404 that Buckaroo returns for an unknown session as "not open", a case +the function already handles. That is one request per entry per klass change. +Found in the second review; the first draft of this decision did not mention +the reload. + Deleting a snapshot (D12) ends no session. A tab that already has the entry open fails on its next query, inside Buckaroo, with the `At least one path is required` error above. That is accepted: the user @@ -450,6 +506,12 @@ empty. The test fails on `main` today (finding 1), so it belongs in the failing-tests commit, and it outlives this change as the check that no xorq cache node has crept back in. +xorq reads `XORQ_CACHE_DIR` once, when it is first imported, and +`tests/conftest.py` points the whole test session at one directory for that +reason. So the sentinel cannot be a fresh directory per test. The test either +asserts that no file appears in the session's directory while it runs, or runs +its steps in a child process with a value of its own. + ### D9. One change, one rebuild Removing the cache node changes the hash of every worthy entry, and bare-read @@ -522,7 +584,9 @@ discover them: not change it. The lock is blocking and has no timeout, and the work it now covers is long: a -materialization runs single-partition and twice (ADR-009 decisions D1 and D6). +materialization runs single-partition, and a create runs its query twice +(ADR-009 decisions D1 and D6). A heal runs it once, since the recorded digest +is what it is compared against. A page request that needs a heal therefore waits behind any build in the other process. Paddy, 2026-09-20: correct first. #186 tracks the waiting. @@ -546,12 +610,123 @@ callers. There is no disk budget yet. That is the rewrite of between entries (`app.py:807-824`). That undoes the Cache page's delete button, and one large heal blocks startup for as long as it takes. - The verify sweep reads and never writes (D5). +- A reset does not touch `compute_cache/` (D14). - An explicit delete skips an entry marked not reproducible (decision D6 of `plans/ADR-009-digest-stability.md`), whose file cannot be recreated. The skip protects the file from the Cache page only. `compute_cache/` as a whole is still deletable by definition (D7), and where such a file should live is part of #185. +### D13. A file is cache only if `ensure_materialized` can re-create it + +Three classes of file sit behind an entry, and until now only the first had a +whole lifecycle: + +| File | Written by | If it is missing | So it is | +| --- | --- | --- | --- | +| Snapshot, `compute_cache/result_cache/.parquet` | `materialize` (D4) | re-run the entry's build and verify the digest (D5) | cache | +| Ordered copy of a source (decision D2 of `plans/ADR-008-row-order-of-reads.md`) | ingest, through polars | re-run ingest on the clone, with the reader options the manifest records | cache | +| Clone, `data/.cas/` | `ensure_cas_path` | copy the live source again, but only while its bytes still hash to the digest | data | + +The rule: a file is cache only if `ensure_materialized` can re-create it from +files that are not cache, and check what it made. Everything under +`compute_cache/` has to satisfy that, and then anything may delete it (D7, +D12). A file that is not cache is never deleted by machinery. + +What follows from the rule: + +- **Ordered copies are cache, so they live under `compute_cache/`**, inside + the project, where the portable-path placeholder covers them and the cold + test of D7 deletes them. `csv_ordered/` under `TALLYMAN_HOME` is retired. + For `ensure_materialized` to re-run ingest, the manifest's `sources` records + each source's reader options (the schema spec and the `scan_csv` options) + next to its digest. It holds only `{path: digest}` today. A re-created copy + is checked against a digest recorded when the copy was first written, which + is the rule D5 applies to snapshots. Where the copies live and the recorded + digest both follow from the rule and are not yet confirmed by Paddy in so + many words. They settle open question 3 here and open question 5 of + `plans/ADR-008-row-order-of-reads.md`. +- **Clones are data.** A clone is the only frozen copy of the bytes an entry + was built from, and it cannot be made again once the live file has been + edited. `gc_cas` deletes one today as soon as no surviving entry refers to + it, which D14 changes. `recon_cas_path` already re-clones a missing clone + from the live source when the live bytes still hash to the digest, but it is + reached only by re-running a recipe, which reads stopped doing in ADR-006. + That check moves into step 3 of D5. +- **The snapshot of an entry that is not reproducible** (decision D6 of + `plans/ADR-009-digest-stability.md`, create runs the query twice and + compares) is data that sits in the cache directory. Where it should live is + part of #185, as D12 already says. + +Measured on today's code (`scripts/spike_reset_roundtrip.py`). Step s1 holds +an entry over `orders.parquet`. Step s2 adds a cheap entry and a worthy entry +over `extra.parquet`, which is never touched: + +| After | Cheap entry | Worthy entry | +| --- | --- | --- | +| building s2 | reads | reads | +| a reset to s1, then a reset forward to s2 | `ValueError: At least one path is required` | reads, from the snapshot the bullpen gave back | +| the same, then `compute_cache/` emptied | the same error | the same error, from the heal | + +The reset to s1 deleted the clone of `extra.parquet`, the reset forward +restored the entries that read it, and nothing on the read path makes a clone +again. This is a defect in today's code and does not depend on this ADR. + +*Rejected:* keep ordered copies next to the clone under `data/.cas/`, as the +fix for #168 first proposed. `gc_cas` deletes every file in that directory +whose stem is not a live source digest, and an ordered copy is named by a key +and not by a digest, so every reset would delete every ordered copy. + +### D14. A reset leaves `compute_cache/` alone + +`reset_to` (`catalog_state.py:327`) returns the catalog to an earlier step +with `git reset --hard`, and then reconciles the files git does not track. For +`compute_cache/` it keeps a git-tracked list, `compute_cache.jsonl`, of every +file that was under the directory at each checkpoint +(`capture_tallyman_state`). A reset moves each file that is not on the target +step's list into the bullpen (`prune_compute_cache`), copies back each listed +file that is missing (`restore_from_bullpen`, a bare `shutil.copy2` onto the +final path), and then deletes every clone that no surviving entry refers to +(`gc_cas`). None of the three ADRs of this set mentioned it before the second +review. + +Against this ADR that machinery is a second owner of the cache: + +- the copy back is a second writer of snapshot files, outside D4. It is not + atomic, and since D5 treats "the file exists" as a hit, a page request can + open a half-copied file; +- the prune moves files that D12 says only the user deletes, including the + file of an entry that is not reproducible, which cannot be rebuilt; +- the list records whatever is under the directory, so it would pick up the + writer's temp files after a crash; +- capturing the list is a cost every checkpoint pays, and it grows with the + cache (#22). + +Decision: a reset stops managing `compute_cache/`. Snapshots are named by +content hash, so a file left behind by a retired entry cannot be served for +another entry. It is unreferenced disk until that entry comes back or the user +deletes it, and a snapshot that is missing after a reset is healed and +verified like any other (D5). `compute_cache.jsonl`, `prune_compute_cache` and +the `compute_cache/` half of `restore_from_bullpen` are deleted. + +The prune existed for one property: after a reset, an expression that is added +again should compute cold, so that a rehearsed demo shows real work. A create +always runs the query (D4), so that property holds without the prune. + +Clones are data (D13), so a reset stops deleting them. It moves a clone that +no surviving entry refers to into the bullpen, and a reset forward brings it +back, which is how a reset already treats entry directories. + +Three options were played through in the second review. Keeping today's +machinery needs an atomic, verified copy back and an exception for files that +cannot be rebuilt. Pruning without restoring keeps one writer and still needs +that exception. Leaving the directory alone needs neither. Paddy, 2026-09-20: +"I guess your suggestions sound good." + +Cost accepted: the snapshot of a retired entry stays on disk until the user +deletes it. The Cache page lists files by entry, so it needs a row for files +whose entry is not in the catalog. + ## Testing Every test below goes in the failing-tests commit and is seen red on CI before @@ -559,9 +734,8 @@ the change lands (D9, step 1). A test of a function that does not exist yet fails on import, and that counts as red. An earlier draft let such tests ride with the change. Paddy, 2026-09-20: do normal TDD. -- **Sentinel** (D8). With `XORQ_CACHE_DIR` pointing at an empty - directory, a build, a chained child build, a view, a delete and a reopen - leave that directory empty. +- **Sentinel** (D8). A build, a chained child build, a view, a delete and a + reopen write nothing under xorq's cache directory. - **Concurrent builds** (D11). Two threads building the same entry both return it, and the entry's directory is intact afterwards. - **Forgotten session** (D6). After Buckaroo has dropped a session, @@ -587,6 +761,19 @@ with the change. Paddy, 2026-09-20: do normal TDD. sweep reports the entry as absent, and the file is still absent afterwards. - **Forced reload** (D6). After an unfaithful heal, Buckaroo receives a `/load_expr` for that entry's session id with `force_reload` set. +- **Klass reload without a session record** (D6). After a klass is added, a + grid that is open shows it, and an entry that was never opened has no + session afterwards. +- **A create always runs** (D4, D14). Building an entry whose snapshot file is + already on disk runs the query and replaces the file. +- **Every class of file is re-created** (D13). With an ordered copy deleted, + opening a root entry makes it again from the clone and checks it. With a + clone deleted and the live source unchanged, the clone is made again. With + the live source changed as well, the error names the source file. +- **A reset back and forward breaks nothing** (D13, D14). After a reset to an + earlier step and a reset forward, every entry of the restored step reads, + cheap and worthy, and still reads after `compute_cache/` is emptied. The + reset moved and copied nothing under `compute_cache/`. ## Consequences @@ -597,7 +784,18 @@ with the change. Paddy, 2026-09-20: do normal TDD. and the `(FileNotFoundError, ValueError)` retry around xorq's shared temp file; `entry_graph_expr` as a separate function; tallyman's record of Buckaroo sessions (`_sessions`, `~/.tallyman/buckaroo_sessions.json`) and - `evict_session`, which worked by dropping an entry of it (D6). + `evict_session`, which worked by dropping an entry of it (D6); + `compute_cache.jsonl`, `prune_compute_cache` and the `compute_cache/` half + of `restore_from_bullpen` (D14); `csv_ordered/` under `TALLYMAN_HOME` (D13). +- **#77:** closed by D2, though not tested on a second machine. #77 is an + empty grid on a clone of the project at another path. The snapshot's name + came from xorq's tokenization of a graph that contains the build-time path, + so a heal on the new machine wrote its file under the key that machine + computed, while the child's frozen build read the key computed at build + time. Under D2 the name is the entry's recorded content hash, which is its + directory name and is the same on every machine, and the literal path in a + child's build goes through the `${TALLYMAN_PROJECT_ROOT}` placeholder. D5 + then heals the file where the build reads it. - **ADR-006:** its D4 (inlined chaining) and D8 (snapshot-key tripwire) are superseded. Its D2's "snapshot path derived from the loaded expression" becomes `snapshot_path`. Its D3 (rebind composition onto the default backend) @@ -627,38 +825,55 @@ with the change. Paddy, 2026-09-20: do normal TDD. - **Source identity `salt` mode:** `rewrite_for_build` returns early under `salt` because xorq's path-only snapshot keys would collide. A snapshot named by the entry's content hash has no such collision, so the early return should - become unnecessary. Not tested. + become unnecessary. Not tested. The early return also skips the canonical + sort, so while it stays a worthy entry built under `salt` is written in the + engine's order. - **Cost accepted:** a worthy parent's snapshot must exist before a child can be built. A build also no longer repairs its own ancestors when something outside tallyman executes it, and under the governing rule nothing does. A tab open on an entry whose file the user deletes errors until the entry is - reopened (D6). A missing ordered copy of a source surfaces as a Buckaroo - error (D5). + reopened (D6). The snapshot of an entry that a reset retired stays on disk + until the user deletes it (D14). ## Open questions -1. **Do two kinds of entry survive?** This is the largest open question of the - set and Paddy has not answered it. Materializing every entry when it is - created would remove the cheap and worthy classifier, the build error of - ADR-008 decision D3, the allow-list of ADR-008 decision D4, the view case in - D6, and open question 2 below, and it would give every entry a digest. An - entry built on another would always read the parent's file, so no graph - would be more than one entry deep, which is the parquet boundary Paddy wanted - in June. `plans/ADR-003-result-cache-cost-rubric.md` already proposes - admitting every result and evicting by budget. The cost is one file per - entry: one project measured 19 GB of cache for 779 MB of data while every - CSV revision wrote a file. His answer to question 4 ("materialize the - parquet if necessary") stands until he says otherwise. -2. **Deep cheap chains.** Nothing cuts the graph between cheap entries. Moot if - open question 1 is answered with one kind of entry. +1. **Do two kinds of entry survive?** Closed on 2026-09-20. Paddy: "yep keep + two kinds". A worthy entry is materialized when it is created, and a cheap + entry is a stored plan over files that exist (D6). The number is kept so + that references to open questions 2 and 3 stay valid. + + The alternative was to materialize every entry when it is created. It + would have removed the cheap and worthy classifier, the build error of + ADR-008 decision D3, the view case in D6 and open question 2, given every + entry a digest, and cut every graph at one entry deep, which is the parquet + boundary Paddy wanted in June. Its cost is one file per entry: one project + measured 19 GB of cache for 779 MB of data while every CSV revision wrote a + file. The second review measured that it was workable. On the + single-partition connection `materialize` uses, every row-preserving plan + streamed its rows in the parent file's order, three runs out of three, + against none on the default connection (`scripts/spike_stream_order.py`), + so the writer could have numbered rows in parent order with no column + carried through the recipe. + + What keeping two kinds costs is that the test for "cheap" becomes a + guarantee. A cheap entry has no file of its own and pages by its parent's + `__row_order`, so the test has to ensure that the column is still present + and still unique at the top of the plan. Four decisions of + `plans/ADR-008-row-order-of-reads.md` carry that, and each was tightened or + added in the second review: D3 (dropping the column is a build error), D4 + (the test itself), D6 (joins) and D12 (a raw parquet read is a build + error). +2. **Deep cheap chains.** Nothing cuts the graph between cheap entries. Still + open, now that open question 1 is closed with two kinds. 3. **Where ordered copies of sources live.** ADR-008 decision D2 adds one per parquet source. `csv_ordered/` is global today, is never collected, and is not included by `tallyman pack`. Its path is also outside the project root, so `make_portable_inplace` does not rewrite it and a CSV entry's build is - not portable. The fix for #168 proposes keeping a CSV's ordered copy under - the project, next to the content-addressed clone it is built from, which - would settle this for CSVs. If every entry is materialized, a root entry's - own file could serve as the ordered copy. + not portable. Settled in outline by D13: an ordered copy is cache, so it + lives under the project's `compute_cache/`, and `ensure_materialized` makes + it again from the clone when it is missing, so `tallyman pack` has no need + to ship it. Not yet confirmed by Paddy in so many words. What remains is + the directory's name. Closed in the grilling session: eviction policy (D12). diff --git a/plans/ADR-008-row-order-of-reads.md b/plans/ADR-008-row-order-of-reads.md index 6cdae2d..fae0ecb 100644 --- a/plans/ADR-008-row-order-of-reads.md +++ b/plans/ADR-008-row-order-of-reads.md @@ -2,8 +2,9 @@ - **Status:** Proposed (2026-09-18, revised 2026-09-20 in the grilling session, and again the same day after a review of PR #184, which made the - fix for #168 a precondition of D2 and D7). Awaiting Paddy's review; nothing - here is implemented. The first draft pinned row order with an engine setting. Paddy + fix for #168 a precondition of D2 and D7, and a third time that day after a + second review: D4 and D6 were tightened, and D10 to D12 are new). Awaiting + Paddy's review; nothing here is implemented. The first draft pinned row order with an engine setting. Paddy proposed baking a row-order column into every file tallyman writes and sorting every page by it. The measurements below favour that, so it is now the decision and the engine setting is the rejected alternative under D5. @@ -15,9 +16,14 @@ 0.15.4, xorq 0.3.26, xorq-datafusion 0.2.7). - **Tickets:** #168 (CSV sources bypass source identity; D2 and D7 depend on its fix), buckaroo-data/buckaroo#974 (Buckaroo's half of D5, see D8), #188 - (diffs, moved out of ADR-007). + (diffs, moved out of ADR-007), #12 (the classifier reads `expr.yaml` with a + regex, which D4 retires), #146 (ordering inside window functions and + ordered aggregates, which D10 leaves there). - **Affected code:** `src/tallyman_xorq/source_cache.py` (`rewrite_for_build`, - `_tie_break_order`), `src/tallyman_xorq/io.py` (`read_project_file`, + `_tie_break_order`, `_canonical_sorted`, `_is_worthy_expr`), + `src/tallyman_xorq/build.py` (`_csv_direct_read_check`, which gains a + parquet sibling, and the hint that turns ibis's name-collision error into + an instruction), `src/tallyman_xorq/io.py` (`read_project_file`, `tallyman_read_csv`, `io.py:627`, and for #168 `_ordered_csv_key` and `_ordered_csv_parquet`), `src/tallyman_xorq/source_identity.py` (the three steps a CSV now goes through), `src/tallyman_xorq/result_cache.py` @@ -36,8 +42,12 @@ - **Evidence:** `scripts/spike_row_order_paging.py` (the decisions), `scripts/spike_window_read_order.py` (the problem, and the rejected engine-setting approach), `scripts/spike_csv_source_identity.py` (D2 and D7: - what a CSV edit does to a content hash) and - `scripts/spike_deep_page_memory.py` (the memory figures under Consequences). + what a CSV edit does to a content hash), + `scripts/spike_deep_page_memory.py` (the memory figures under Consequences), + and four from the second review: `scripts/spike_cheap_classifier.py` (D4), + `scripts/spike_row_order_joins.py` (D6), `scripts/spike_sort_grafting.py` + (D10, D11 and the note on parquet statistics under D5) and + `scripts/spike_ordered_copy_layout.py` (D2 and open question 1). All figures are from those scripts on a 14-core machine. ## Terms @@ -55,6 +65,11 @@ computed columns, renames and casts qualify. Aggregates, joins, unions, distincts and unnests do not. - **Tie:** two or more rows with equal values in every sort key. +- **Natural order:** the order of rows in the file they came from, which + `__row_order` records (D2). +- **Graft:** to add sort keys to a query that its author did not write. +- **Hoist:** to take the keys of a sort from lower in a recipe and lead the + top-level sort with them (D11). - **Exchange operator:** a DataFusion physical-plan step (`RepartitionExec`, `CoalescePartitionsExec`) that moves rows between parallel partitions. After one, rows arrive in whatever order the partitions finish. @@ -150,7 +165,11 @@ Two writers produce it: steps `read_project_file` performs for parquet today, `io.py:78-84`), and the ordered copy is built from the content-addressed clone, which stays the immutable input. A source that already has a `__row_order` column has it - overwritten, which is the right outcome for a file tallyman exported. + overwritten, which is the right outcome for a file tallyman exported. The + copy lives under the project's `compute_cache/`, and `ensure_materialized` + makes it again from the clone when it is missing (decision D13 of + `plans/ADR-007-tallyman-owned-materialization.md`, which files are cache). + D12 closes the one way a parquet file could enter without a copy. CSVs have the ordered-copy step today and not the keying. The first draft of this decision said they already worked this way, which was wrong. The @@ -167,7 +186,8 @@ The canonical sort's tie-break (`_tie_break_order`) puts an inherited `__row_order` where `original_row_order` is today: after the author's own `order_by` keys and before the remaining columns. A worthy entry that keeps its parent's rows, such as one adding a window function, therefore keeps the -parent's order. +parent's order. D10 applies the same tie-break to every sort in a recipe, not +only to the last one. *Rejected:* `row_number()` inside the entry's graph. It needs the same global sort, adds a window function to every worthy build, and leaves contiguity to @@ -191,8 +211,10 @@ descriptions say the same thing up front. This is the feedback channel A worthy entry is exempt, because the writer numbers its rows (D2). An author changes `__row_order` by asking for an order: `order_by` makes the entry worthy, -and the writer numbers the rows in the requested order. Assigning to the column -directly stays an error (D6). +and the writer numbers the rows in the requested order. That holds for an +`order_by` anywhere in the recipe only because of D11. As first drafted it was +true of an `order_by` that is the recipe's last step, and of no other. +Assigning to the column directly stays an error (D6). Tallyman makes one alteration of its own, at the top of the expression only: it moves `__row_order` to the last position, since a computed column added after @@ -220,15 +242,63 @@ A cheap entry inherits its row order, so it must be a row-preserving plan over exactly one file. The classifier changes from a deny-list to an allow-list: an entry is cheap only if every relation operation in its graph is known to be row-preserving (a file read, a filter, a column selection, a computed column, a -rename, a cast, a column drop). Anything else is worthy, including operations -nobody has thought about yet. Today's `_EXPENSIVE_OPS` deny-list classes -`Union`, `Distinct` and `Unnest` as cheap, and none of them can carry one -parent's row order. +rename, a cast, a column drop, a drop of null rows, a fill of nulls). Anything +else is worthy, including operations nobody has thought about yet. Today's +`_EXPENSIVE_OPS` deny-list classes `Union`, `Distinct` and `Unnest` as cheap, +and none of them can carry one parent's row order. + +A list of relation operations is not enough, which the second review measured +(`scripts/spike_cheap_classifier.py`). Paddy confirmed on 2026-09-20 that two +kinds of entry stay (open question 1 of +`plans/ADR-007-tallyman-owned-materialization.md`), so this test is what +guarantees that a cheap entry pages repeatably, and it has to look inside the +allowed operations as well. The parent has 6 rows: + +| Recipe shape | Relation operations | Today's deny-list | Relation allow-list alone | Rows | `__row_order` unique | +| --- | --- | --- | --- | --- | --- | +| `t.select("k", "__row_order", tag=t.tags.unnest())` | read, select | cheap | cheap | 8 | no | +| `t.mutate(rn=ibis.row_number())` | read, select | worthy | cheap | 6 | yes | +| `t.mutate(prev=t.v.lag())` | read, select | worthy | cheap | 6 | yes | +| `t.mutate(r=ibis.random())` | read, select | cheap | cheap | 6 | yes | +| `t.filter(t.k.isin(u.k))` | two reads, filter, select | cheap | cheap | 3 | yes | + +An `unnest` written inside a select is an ordinary column selection to a list +of relation operations. It multiplied the rows and duplicated `__row_order`, +which breaks the "no ties" that D5 depends on. A window function inside a +computed column is an ordinary selection too, and its values depend on the +order the rows arrive in (D10). + +So the test has three parts, each decided on the live expression by the class +of the operation: + +- every relation operation is on the list above; +- the plan reads exactly one file; +- no value operation multiplies rows (`Unnest`), depends on row order + (`WindowFunction`, which also covers `row_number`, `lag` and a total used + inside a computed column), or is not pure (`Impure`, which is `random()` and + `uuid()`; `now()` and `today()` by name, since xorq's ibis classes them as + constants; and any UDF, matched as it is today). + +The verdict is computed once, when the entry is built, and recorded in the +manifest. `result_cache.cache_worthy()` and the gate on primary-key +inheritance (`primary_key.py:204`) read the manifest and stop classifying. +`classify_build` and its regex over `expr.yaml` are retired, which closes #12. +A regex cannot hold an allow-list: `op:` in that file also matches `DataType`, +`FrozenDict`, `float` and `tuple`, which are not operations, and `Field` and +`Literal`, which are not relations. There is then one implementation, and +nothing to keep in lockstep. A new or unknown operation now costs a copy (safe) instead of unstable paging -(unsafe). `classify_build` (which reads the serialized build) and -`_is_worthy_expr` (which reads the live expression) flip together, as they must -today. +(unsafe). So does a second file, and so does `random()`. + +The three parts are what keeping two kinds requires, as proposed in the second +review, and Paddy has not confirmed the list in so many words. The "not pure" +part reaches into #185 (non-pure recipes). It makes a recipe that calls +`random()` worthy, so decision D6 of `plans/ADR-009-digest-stability.md` +(create runs the query twice) checks it and pins its file, and the cheap half +of #185 has nothing left to decide. If that is unwanted, striking `Impure` and +the two names restores today's behaviour, where such an entry re-runs on every +read. Supporting measurement from the first draft: a union of two files returned 2 different pages for 8 identical requests even on a single-partition @@ -241,7 +311,8 @@ operator merges them. - User sort: the user's keys, then `__row_order` ascending as the last key, which breaks every tie. -That is the whole rule, for every entry and both processes. Two faster paths +That is the whole rule, for every entry and both processes. It is the +page-request half of Paddy's rule in D10. Two faster paths exist and are deliberately not part of this decision (Paddy, 2026-09-20: a cohesive system that works reliably comes first, and speed problems are handled as they come up): @@ -254,6 +325,18 @@ as they come up): Both are measured below so the numbers are on hand when they are wanted. +A third was asked about in the second review: leaving the last key off when +the user's sort key is already unique. Parquet's statistics cannot show that. +pyarrow writes a minimum, a maximum and a null count for each column chunk and +no distinct count, so a unique column and one holding a value twice have the +same statistics (`scripts/spike_sort_grafting.py`), and a distinct count would +be per row group in any case. Tallyman's writer could record the fact itself, +since rows reach it sorted and ties on the author's keys are adjacent. It +would save little: when the leading key is unique the comparison never reaches +`__row_order`, and when it is not, the key is needed. It could apply to page +requests only, because the keys D10 adds at build time are part of the hashed +graph, and it would have to be repeated inside Buckaroo. Not planned. + Measured on 3,000,000 rows by 14 columns (287 MB), on the default parallel connection with no engine settings. Every row of the table returned the correct page 6 times out of 6: @@ -301,12 +384,25 @@ semantics, in any engine and any process. - A recipe may read the column and may copy it under another name (D3). A recipe that assigns to `__row_order` is a build error, because arbitrary values could contain ties or gaps, and D5 depends on `0..N-1` with neither. -- Only the exact name is special. `__row_order_v1`, or any other name an author - picks for a copy, is ordinary data and survives materialization. +- Only the exact name is special, and ibis's collision name for it, which the + next item covers. `__row_order_v1`, or any other name an author picks for a + copy, is ordinary data and survives materialization. - A join of two entries leaves the right side's copy behind under ibis's - collision name, `__row_order_right`, and a three-way join silently keeps only - the first two. That column is ordinary data too: it says where the row sat in - the right-hand parent. The writer replaces only `__row_order` itself. + collision name, `__row_order_right`. The writer drops that one column, as + well as replacing `__row_order`. An earlier draft kept it as ordinary data + and said that a three-way join "silently keeps only the first two". Both + were wrong (`scripts/spike_row_order_joins.py`). A three-way join written in + one recipe shows the expected columns and passes `build_expr`, then raises + `IntegrityError: Name collisions: {'__row_order_right'}` from the canonical + sort, which is the next build step. A join entry that kept the column fails + the same way as soon as it is joined to a third entry, and with the column + dropped from its file that second join builds. For a three-way join in one + recipe the author has to drop `__row_order` from the right-hand inputs, and + the build turns ibis's message into that instruction, since the author never + wrote the name it complains about. An author who wants the right-hand + positions keeps them under a name of their own, as D3 describes. Proposed in + the second review as part of what keeping two kinds requires, and not yet + confirmed by Paddy in so many words. - The primary-key search skips it. Nothing excludes `original_row_order` from the candidates today (`primary_key.py:219`), and a column that is unique in every table would win the search for any table without a string or id key. @@ -344,7 +440,7 @@ What INV-2 provided, and what replaces it: | A canonical display order | D5. INV-2 did not deliver this above 10 MB. | | A parquet boundary for chained children | A cheap root's graph is one read node. | | Fixed rows under the root's hash, through its baked snapshot | The content-keyed ordered copy of D2, written once. Requires the fix for #168. | -| A `result_digest` on the root, so a re-parse that produced different rows would be caught | Lost as it stands: cheap entries record no digest (ADR-006 decision D9, "no cheap-entry digests"). It matters only when an ordered copy is deleted and re-created. See open question 5. | +| A `result_digest` on the root, so a re-parse that produced different rows would be caught | A digest recorded for the ordered copy itself, which a re-created copy is checked against (ADR-007 decision D13, which files are cache). Cheap entries still record no digest of their own (ADR-006 decision D9, "no cheap-entry digests"). Not yet confirmed; see open question 5. | Every hash in every CSV lineage changes, so this rides the corpus rebuild of ADR-007 decision D9 ("one change, one rebuild"). @@ -376,6 +472,135 @@ does not establish a split scan and is not what makes that test meaningful; its aggregate is), and `src/tallyman_xorq/source_cache.py:98`. `tests/test_tallyman_read_csv.py:159` already says about 10 MB. +### D10. The natural order is imposed on every `order_by` + +Paddy's rule, 2026-09-20: "every query should have a unique sortby clause, if +the base query didn't have one, it needs to be grafted onto the query so that +everything else is order deterministic", and then: "when the user/mcp supplied +sort isn't deterministic, impose the natural order into each order by so the +resulting query becomes deterministic." + +The reading that was put to him the same day, which he did not overrule: + +- **Always.** Tallyman cannot know whether a supplied sort is unique without + scanning the table (D5 records why parquet statistics do not help), and a + last key added to a sort that is already unique changes nothing. +- **The natural order, then the remaining sortable columns.** The natural + order alone is unique only while rows come one for one from a single file + tallyman wrote. Above a join or a union it has ties, above an outer join it + has nulls, a value-level `unnest` duplicates it (D4), and after an aggregate + it is gone. With the remaining columns after it the sort is total up to rows + that are identical in every sortable column, and those write the same bytes + in either order. This is the tie-break `_tie_break_order` already builds. +- **At every sort in the recipe, and on every page request.** Page requests + are D5. The build-time half is new: `_canonical_sorted` extends an author's + sort only when it is the top node of the expression + (`source_cache.py:113`), and leaves every other `Sort` node as written. + +Why this has to reach every sort: a sort that feeds a `limit` decides which +rows the entry holds, and the top of the expression is too late to break its +ties. `order_by(g).limit(1000)` over 3,000,000 rows, where about 15,000 +rows tie on the smallest `g`, five runs each +(`scripts/spike_sort_grafting.py`): + +| Connection | Sort key | Distinct sets of rows in 5 runs | Equals the rows `(g, id)` picks | +| --- | --- | --- | --- | +| default | `g` | 5 | no | +| `target_partitions = 1` | `g` | 1 | yes | +| default | `g`, then the row position | 1 | yes | + +The middle row is how this case works today. It is repeatable because +materialization runs single-partition (decision D1 of +`plans/ADR-009-digest-stability.md`), which is the engine's behaviour and the +kind of dependence D5 rejects for page requests. The last row is repeatable by +the query's own meaning, on any connection. A cheap entry cannot contain a +sort, because a sort makes an entry worthy (D4), so the build-time half only +ever runs at materialization. + +The added keys are part of the build's graph, so the content hash covers them, +and this rides the corpus rebuild of ADR-007 decision D9 ("one change, one +rebuild"). + +Left to #146 (a lint for row-ordering nondeterminism): the `order_by` inside a +window function, and ordered aggregates such as `first`, `last` and `collect`. +They are the same rule. `cumsum()` with no order gave 5 distinct sets of +values in 5 runs on the default connection and 1 on a single-partition one. +Ordered by the tied key `g`, and by `g` then the row position, it gave 1 on +both. An entry with a window function is always materialized, so it is +repeatable today, by the engine's behaviour again. Only `cumsum()` was +measured. + +*Rejected:* add the keys only when the supplied sort is not unique. That needs +a scan of the table for every sort, to save a key that costs almost nothing +when the leading key is unique. + +### D11. A sort that is not the recipe's last step is hoisted, or the build fails + +`_canonical_sorted` recognizes an author's sort only when it is the top node. +When another step follows, the check fails and the whole expression is wrapped +in a sort that leads with the inherited row order. That column is unique, so +the author's sort has no effect on what is written. The parent's rows are in +the order 40, 10, 60, 20, 50, 30 and the author asks for `amount` descending +(`scripts/spike_sort_grafting.py`): + +| Recipe | Classed | Written as | +| --- | --- | --- | +| `order_by` last | worthy | 60, 50, 40, 30, 20, 10 | +| `order_by`, then `mutate` | worthy | 40, 10, 60, 20, 50, 30 | +| `order_by`, then `select` | worthy | 40, 10, 60, 20, 50, 30 | +| `order_by`, then `filter(amount > 15)` | worthy | 40, 60, 20, 50, 30 | +| `order_by`, then `limit(3)` | worthy | 40, 60, 50 | + +Each of these is classed worthy because of that sort, so the author pays for a +full copy that ignores it, and a top-three entry is not shown in rank order. +Under Paddy's rule (D10) the author did supply a sort, so the system keeps it: + +- From the top of the expression, walk down through steps that keep the order + of rows (a selection, a computed column, a filter, a limit, a column drop, a + rename, a cast, a drop of null rows, a fill of nulls) to the nearest sort. +- When each of that sort's keys is still an output column, unchanged (a rename + is followed), the top-level sort leads with those keys, then the tie-break + of D10. +- When a key did not survive, because it was dropped, overwritten, or was an + expression and not a column, the build fails. The error names the key and + tells the author to keep the column or to sort as the last step. This is the + choice D3 makes for a dropped `__row_order`: report what the author can fix + in one line, and do not repair it silently. + +Paddy, 2026-09-20: "I like your suggestion." The change is about 40 lines in +`_canonical_sorted`. + +*Rejected:* make any `order_by` that is not the last step a build error. It is +one rule, but a top-N recipe would then have to be written +`order_by(...).limit(n).order_by(...)`. + +### D12. A raw parquet read is a build error + +D2 says every file tallyman reads carries `__row_order`, and that both kinds +of source go through source identity first. Neither is enforced for parquet. +A recipe can call `xo.deferred_read_parquet(abs_path)`, and tallyman's own +hints recommend it (`build.py:199`, `source_cache.py:133`, and the namespace +note in a tool description, `src/tallyman_mcp/server.py:275`). Such a read has no digest, no clone and +no `manifest.sources` record, which is the defect of #168 for a parquet file. +It also has no ordered copy, so a root entry built on it has no `__row_order` +to page by. Only the CSV form is banned today (`_csv_direct_read_check`). + +A read of a parquet file that tallyman did not write becomes a build error +that points the author to `read_project_file`, next to the CSV check, and the +three hints change with it. Tallyman's files are the snapshots and ordered +copies under the project's `compute_cache/` (decision D13 of +`plans/ADR-007-tallyman-owned-materialization.md`, which files are cache), so +the check is on the path of each `Read`. Nine files under `tests/` call +`deferred_read_parquet` today, 23 calls in all, so the change carries test +churn. + +Proposed in the second review as part of what keeping two kinds requires, and +not yet confirmed by Paddy in so many words. + +*Rejected:* rewrite a raw read into an ingest at build time. It is the surgery +inside an author's expression that D3 turned down, and the recipe text would +name one file while the entry read another. + ## Testing Every test below goes in the failing-tests commit and is seen red on CI before @@ -410,15 +635,38 @@ that does not exist yet fails on import, and that counts as red. Paddy, - **CSV roots** (D7). A `tallyman_read_csv` entry has no Sort in its build, is classed cheap, and has exactly one row-order column. - **The hint** (D8). The `/load_expr` payload names `__row_order`. +- **The cheap test looks inside** (D4). A value-level `unnest`, a window + function in a computed column, `random()` and a filter against a second file + are each classed worthy, a drop of null rows is classed cheap, and the + verdict is read from the manifest with no `expr.yaml` parsed. +- **Joins** (D6). A join entry's file has no `__row_order_right`, joining it to + a third entry builds, and a three-way join in one recipe fails with a + message that says to drop `__row_order` from the right-hand inputs. +- **Every sort is total** (D10). An `order_by(g).limit(k)` recipe over a file + above the split threshold, with ties on `g` at the cut, holds exactly the + rows that `(g, __row_order)` picks. +- **A sort that is not last is kept** (D11). A top-three recipe is written in + rank order, `order_by` then `mutate` is written in the sorted order, and a + recipe that drops its sort key in a later select fails to build with a + message that names the key. +- **Raw reads** (D12). A recipe that calls `xo.deferred_read_parquet` on a + source file fails to build with a message that names `read_project_file`. ## Consequences - Pages are repeatable for unsorted and sorted requests, in tallyman and in Buckaroo, with no engine settings and no second connection. -- Every table shows one more column, at the end. A join result also shows - `__row_order_right` unless the recipe drops it. +- Every table shows one more column, at the end. A join result does not carry + `__row_order_right`, because the writer drops it (D6). - A recipe whose select list forgets `__row_order` fails to build until the - author adds it. + author adds it. So does a three-way join that keeps the column on its + right-hand inputs (D6), a recipe that drops the key of a sort it made + earlier (D11), and a recipe that reads a parquet file directly (D12). +- Every sort in a recipe gains keys its author did not write (D10), and a sort + that is not the last step now decides the order that is written (D11). +- Whether an entry is cheap is decided once, at build, and read from the + manifest afterwards (D4). More recipes are worthy than before: one that + unnests inside a select, reads a second file, or calls `random()`. - CSV lineages stop writing sorted copies. With ADR-007, revisions of a CSV entry are cheap reads over one intermediate file, and primary-key inheritance applies to them. @@ -444,9 +692,12 @@ that does not exist yet fails on import, and that counts as red. Paddy, 1. **Ordered copies of parquet sources.** D2 adds a copy per parquet source. Adopted as the uniform rule under Paddy's "cohesive first" priority, and not - yet confirmed by him in so many words. It also assumes polars numbers a - parquet scan's rows in file order, as ADR-004 measured for CSV, which needs - checking. + yet confirmed by him in so many words. With two kinds of entry kept, a + cheap root pages by this column, so the copy is needed. It assumed polars + numbers a parquet scan's rows in file order, as ADR-004 measured for CSV. + Checked in the second review (`scripts/spike_ordered_copy_layout.py`, polars + 1.40.1): `scan_parquet().with_row_index()` numbered a source of 184 row + groups in file order on 1, 3 and 14 threads. 2. **Renaming `original_row_order`.** D7 replaces it with `__row_order`. Adopted on the same basis, and also not yet confirmed. The alternative keeps it as a data column meaning "line of the source file", at the cost of two identical columns on every CSV root. @@ -462,6 +713,9 @@ that does not exist yet fails on import, and that counts as red. Paddy, Recording the copy's digest in the root entry's manifest, and verifying it on re-creation, would cover that. It wants ADR-009's digest definition, and it touches where the copies live: `csv_ordered` is global, is never - collected, and is not packed (ADR-007 open question 3). Nothing re-creates a - missing copy automatically yet, which ADR-007 decision D5 (one entry point - makes files exist) records as an accepted gap. + collected, and is not packed (ADR-007 open question 3). The second review + made re-creation a designed path and not a gap: ADR-007 decision D13 (which + files are cache) puts the copies under the project's `compute_cache/`, has + `ensure_materialized` make a missing one again from the clone, and records + the digest the new copy is checked against. That answer follows from + D13's rule and is not yet confirmed by Paddy in so many words. diff --git a/plans/ADR-009-digest-stability.md b/plans/ADR-009-digest-stability.md index 1003329..fa92869 100644 --- a/plans/ADR-009-digest-stability.md +++ b/plans/ADR-009-digest-stability.md @@ -5,7 +5,10 @@ `plans/ADR-008-row-order-of-reads.md`, D1 lost its speed gate, and D6 is new; and again the same day after a review of PR #184: D1 and D3 now say what single-partition execution leaves undetermined, and D6's cheap-entry - half moved to #185). Awaiting Paddy's review; nothing here is implemented. Amends + half moved to #185; and a third time that day after a second review: D1 now + says how a loaded build gets onto the single-partition connection, and D3's + format version covers the ordered copies of sources). Awaiting Paddy's + review; nothing here is implemented. Amends `plans/ADR-004-result-digest-canonical-ordering.md` (Option A's "hash the snapshot bytes") and decision D5 of `plans/ADR-006-read-path-loads-builds.md` (the canonical sort), which said @@ -34,7 +37,10 @@ - **Evidence:** `scripts/spike_float_aggregate_digest.py`, `scripts/spike_logical_digest.py`, and `scripts/spike_float_layout_digest.py` (D1 and D3: what the layout of the - parent file does to a float total). + parent file does to a float total), and two from the second review: + `scripts/spike_single_partition_loaded_build.py` (D1: a loaded build has to + be rebound) and `scripts/spike_ordered_copy_layout.py` (D3: the layout + polars writes). ## Terms @@ -120,6 +126,31 @@ layout (see "What this does not fix" below). It is a separate connection from the default backend that serves page reads, because a long materialization must not share a context with them. +Making that connection is not enough. `materialize` executes a build that +`load_expr` loaded, and `load_expr` makes its own backend objects, so a loaded +build ignores a single-partition connection it was never bound to +(`scripts/spike_single_partition_loaded_build.py`, a float `SUM` and `AVG` +group-by over 3,000,000 rows): + +| How the loaded build is executed | Distinct digests in 5 runs | +| --- | --- | +| as loaded | 5 | +| as loaded, while a single-partition connection exists on the side | 5 (the build's own backend reports 14 partitions) | +| rebound onto that connection with `replace_sources` | 1 | +| `SET` applied to each backend the load made | 1 | + +Either of the last two works, and neither changes the process default +backend, which still reports 14. `materialize` rebinds. That is what +`_rebind_to_default_backend` (ADR-006 decision D3, rebind composition onto the +default backend) already does, aimed at the materialization connection. + +The same connection is why a bare `limit`, or a window function with no order, +is repeatable at materialization: on it a row-preserving plan streams its rows +in the parent file's order (`scripts/spike_stream_order.py`). That is the +engine's behaviour and not the query's meaning, which is what decision D10 of +`plans/ADR-008-row-order-of-reads.md` (the natural order is imposed on every +`order_by`) is for. + Cost: about 3x on the spike's aggregate. ADR-004 measured a 3.1M-group aggregate at 0.5 s parallel against 3.4 to 3.9 s single-partition, and a full 43-column read at 2.5 s against 6.9 s, on the 11.8M-row parking file. A @@ -262,6 +293,19 @@ connection's `batch_size` are therefore part of the reproducibility contract: the manifest records a snapshot format version that stands for both, and changing either is a corpus rebuild. +The same holds for the ordered copy of a source (ADR-008 decision D2), which +polars writes and this writer does not. Its layout is pinned separately, in +`_CSV_PARQUET_WRITE` (`io.py:91-96`, row groups of 122,880 rows), and an entry +that totals a float column straight from a source reads that layout. The +format version covers those settings too. The second review checked that +polars honours them: `sink_parquet` wrote full row groups of exactly 122,880 +rows, with an identical layout, on 1, 3 and 14 threads, from a CSV source and +from a parquet one (`scripts/spike_ordered_copy_layout.py`, polars 1.40.1). + +The size is fixed in rows and not in bytes. A table with long text columns +therefore holds a large row group in memory while it is written, and the +contract above means the size cannot be tuned for one table. + ### D4. A mismatch record names its likely cause With D1 and D2 in place the causes left are the recipe's own nondeterminism @@ -311,7 +355,11 @@ materialized entry is created: `plans/ADR-007-tallyman-owned-materialization.md` all rely on. Paddy moved it to #185 on 2026-09-20. Until that is settled a cheap entry that calls `random()` behaves as it does today: the #88 lint warns, and the entry - re-runs on every read. + re-runs on every read. The second review proposed a smaller answer, recorded + in ADR-008 decision D4 (the test for cheap) and not yet confirmed by Paddy: + an operation that is not pure makes an entry worthy. Such a recipe is then + checked here like any other materialized entry, and no cheap entry calls + `random()`. The cost is a second execution of every create of a materialized entry. It is accepted under the priority recorded in ADR-007 ("a cohesive system that works @@ -325,9 +373,10 @@ file survives. The pin protects the file from the Cache page's delete and from nothing else, since `compute_cache/` is deletable by definition (ADR-007 decision D7, the cold state is an empty `compute_cache`). -A heal is still verified against the recorded digest, as now. After D6 a -mismatch there means something changed underneath a reproducible entry, which -D4 attributes. +A heal runs the query once and is verified against the recorded digest, as +now. Only a create runs it twice, since a create has nothing recorded to +compare against. After D6 a mismatch at a heal means something changed +underneath a reproducible entry, which D4 attributes. ## Testing @@ -351,8 +400,14 @@ that does not exist yet fails on import, and that counts as red. Paddy, - **Create passes a reproducible recipe.** A deterministic recipe is recorded as reproducible, and the query is observed to run exactly twice. - **The layout is pinned** (D1, D3). Every snapshot has row groups of 1,048,576 - rows, the materialization connection reports the pinned `batch_size`, and - the manifest records the snapshot format version. + rows, every ordered copy of a source has row groups of 122,880 rows, the + materialization connection reports the pinned `batch_size`, and the manifest + records the snapshot format version. +- **The build runs on the single-partition connection** (D1). While + `materialize` executes a loaded build, every backend the plan touches reports + `target_partitions = 1`, and the process default backend does not. +- **A heal runs once** (D6). Reopening an entry whose file was deleted runs its + query exactly once. - **A pinned file survives an explicit delete.** The Cache page's delete skips it and says why. - **The schema comes from the file.** An entry with a `timestamp[s]` column @@ -381,7 +436,8 @@ that does not exist yet fails on import, and that counts as red. Paddy, - The same function can digest an ordered copy of a source (ADR-008, open question 5). - The row-group size and the materialization connection's `batch_size` are - frozen for the life of the corpus, and changing either is a rebuild (D3). + frozen for the life of the corpus, and so are the settings polars writes an + ordered copy of a source with. Changing any of them is a rebuild (D3). ## Open questions diff --git a/scripts/spike_cheap_classifier.py b/scripts/spike_cheap_classifier.py new file mode 100644 index 0000000..3cc5f1d --- /dev/null +++ b/scripts/spike_cheap_classifier.py @@ -0,0 +1,108 @@ +"""ADR-008 evidence (D4): what a test for "cheap" has to look at, now that two kinds of entry stay. + +A cheap entry has no file of its own and pages by its parent's ``__row_order``, so the test has to guarantee that the +column stays unique through the plan. D4's first wording was an allow-list of RELATION operations. This script runs +three tests over the same recipe shapes: + +- today's deny-list (``result_cache.classify_build``, a regex over ``expr.yaml``); +- a relation-only allow-list, which is D4 as first worded, with drop-null and fill-null added to its list; +- the test D4 now specifies: the relation allow-list, exactly one file read, and no value operation that multiplies + rows, depends on row order, or is not pure, decided by base class on the live expression. + +For each shape it also executes the plan and reports whether ``__row_order`` is still unique. + + uv run python scripts/spike_cheap_classifier.py +""" + +from __future__ import annotations + +import os +import re +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_cheap_classifier_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 +import xorq.vendor.ibis as ibis # noqa: E402 +import xorq.vendor.ibis.expr.operations as ops # noqa: E402 +from xorq.common.utils.graph_utils import walk_nodes # noqa: E402 +from xorq.expr.relations import Read # noqa: E402 +from xorq.ibis_yaml.compiler import build_expr # noqa: E402 +from xorq.vendor.ibis.expr.operations.core import Node # noqa: E402 + +from tallyman_xorq.result_cache import classify_build # noqa: E402 + +ROW_PRESERVING_RELATIONS = (Read, ops.Filter, ops.Project, ops.DropColumns, ops.DropNull, ops.FillNull) +NEVER_CHEAP_VALUES = (ops.Unnest, ops.WindowFunction, ops.Impure) +NEVER_CHEAP_BY_NAME = {"TimestampNow", "DateNow"} # these are Constant, not Impure, in xorq's ibis + + +def relation_only_allow_list(expr) -> bool: + relations = [n for n in walk_nodes((Node,), expr) if isinstance(n, ops.Relation)] + return all(isinstance(n, ROW_PRESERVING_RELATIONS) for n in relations) + + +def is_cheap(expr) -> bool: + nodes = list(walk_nodes((Node,), expr)) + relations = [n for n in nodes if isinstance(n, ops.Relation)] + if len({n for n in relations if isinstance(n, Read)}) != 1: + return False + if not all(isinstance(n, ROW_PRESERVING_RELATIONS) for n in relations): + return False + for n in nodes: + if isinstance(n, NEVER_CHEAP_VALUES) or type(n).__name__ in NEVER_CHEAP_BY_NAME: + return False + if any("UDF" in base.__name__ for base in type(n).__mro__): + return False + return True + + +def main() -> None: + n = 6 + tags = [["x", "y"], ["z"], [], ["x"], ["y", "z", "w"], ["q"]] + parent = {"k": list(range(n)), "v": [1.5, 2.5, None, 4.5, 5.5, 6.5], "tags": tags, "__row_order": list(range(n))} + pq.write_table(pa.table(parent), HOME / "t.parquet") + pq.write_table(pa.table({"k": [1, 3, 5], "__row_order": [0, 1, 2]}), HOME / "u.parquet") + t = xo.deferred_read_parquet(str(HOME / "t.parquet")) + u = xo.deferred_read_parquet(str(HOME / "u.parquet")) + + shapes = { + "filter + computed column": t.filter(t.k > 0).mutate(w=t.v * 2), + "rename, cast, drop a column": t.rename(key="k").mutate(v=t.v.cast("float32")).drop("tags"), + "drop_null, fill_null": t.drop_null(["v"]).fill_null({"v": 0.0}), + "unnest inside a select": t.select("k", "__row_order", tag=t.tags.unnest()), + "row_number() in a mutate": t.mutate(rn=ibis.row_number()), + "share of total in a mutate": t.mutate(share=t.v / t.v.sum()), + "lag() in a mutate": t.mutate(prev=t.v.lag()), + "random() in a mutate": t.mutate(r=ibis.random()), + "filter by membership in a second file": t.filter(t.k.isin(u.k)), + "filter against a scalar subquery": t.filter(t.v > t.v.mean()), + "limit": t.limit(3), + "distinct": t.select("k", "__row_order").distinct(), + } + print(f"parent has {n} rows\n") + print(f"{'recipe shape':40s} {'today':8s} {'relations only':15s} {'D4':7s} rows __row_order unique") + names: set[str] = set() + for label, expr in shapes.items(): + build = Path(build_expr(expr, builds_dir=HOME / "builds")) + names |= {m for p in build.glob("*.yaml") for m in re.findall(r"op:\s*([A-Za-z_]+)", p.read_text())} + today = "worthy" if classify_build(build)["worthy"] else "cheap" + relations = "cheap" if relation_only_allow_list(expr) else "worthy" + proposed = "cheap" if is_cheap(expr) else "worthy" + out = expr.execute() + print(f"{label:40s} {today:8s} {relations:15s} {proposed:7s} {len(out):4d} {out['__row_order'].is_unique}") + + relation_names = {c.__name__ for c in ops.Relation.__subclasses__()} | {"Read"} + not_relations = sorted(n for n in names if n not in relation_names and not hasattr(ops, n)) + print(f"\nnames the classify_build regex matches that are not operations at all: {not_relations}") + print( + f"value operations it matches alongside the relations: {sorted(n for n in names if n in ('Field', 'Literal'))}" + ) + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_ordered_copy_layout.py b/scripts/spike_ordered_copy_layout.py new file mode 100644 index 0000000..5d70873 --- /dev/null +++ b/scripts/spike_ordered_copy_layout.py @@ -0,0 +1,100 @@ +"""ADR-008 (D2, open question 1) and ADR-009 (D3) evidence: the ordered copy of a source, which polars writes. + +The ordered copy is not written by the snapshot writer of ADR-009 D3. An ungrouped float total depends on the layout +of the file it reads (#187), so the layout of an ordered copy is part of the reproducibility contract too. + +Questions, asked with polars running on 1, 3 and the default number of threads: + +1. CSV source: are the row groups exactly the size asked for, and is the layout the same on every thread count? +2. CSV source: is the row index 0..N-1 in file order? +3. Parquet source with many row groups: does ``scan_parquet().with_row_index()`` number the rows in FILE order? + ADR-008 open question 1 said this needed checking. + +polars reads ``POLARS_MAX_THREADS`` when it is imported, so each thread count runs in a child process. + + uv run python scripts/spike_ordered_copy_layout.py +""" + +from __future__ import annotations + +import hashlib +import json +import os +import subprocess +import sys +import tempfile +from pathlib import Path + +N = 1_500_000 +WRITE = {"compression": "zstd", "compression_level": 3, "row_group_size": 122880, "statistics": True} # io.py + + +def child() -> None: + import numpy as np + import polars as pl + import pyarrow as pa + import pyarrow.parquet as pq + + home = Path(tempfile.mkdtemp(prefix="spike_ordered_copy_")) + rng = np.random.default_rng(7) + positions = np.arange(N) + + def layout(path: Path) -> tuple[list[int], str]: + md = pq.ParquetFile(path).metadata + sizes = [md.row_group(i).num_rows for i in range(md.num_row_groups)] + return sizes, hashlib.sha256(repr(sizes).encode()).hexdigest()[:12] + + def ordered_copy(scan, out: Path) -> None: + indexed = scan.with_row_index("__row_order") + indexed.select(["id", "v", pl.col("__row_order").cast(pl.Int64)]).sink_parquet(str(out), **WRITE) + + csv = home / "src.csv" + pl.DataFrame({"id": positions, "v": rng.normal(size=N)}).write_csv(csv) + from_csv = home / "from_csv.parquet" + ordered_copy(pl.scan_csv(str(csv)), from_csv) + + parquet = home / "src.parquet" + pq.write_table(pa.table({"id": positions, "v": rng.normal(size=N)}), parquet, row_group_size=8192) + from_parquet = home / "from_parquet.parquet" + ordered_copy(pl.scan_parquet(str(parquet)), from_parquet) + + result = {"threads": pl.thread_pool_size(), "polars": pl.__version__} + for name, path in (("csv", from_csv), ("parquet", from_parquet)): + sizes, signature = layout(path) + table = pq.read_table(path) + result[name] = { + "row_groups": len(sizes), + "full_groups_exact": all(s == WRITE["row_group_size"] for s in sizes[:-1]), + "layout": signature, + "index_is_0_to_n": bool((table["__row_order"].to_numpy() == positions).all()), + "index_is_file_position": bool((table["__row_order"].to_numpy() == table["id"].to_numpy()).all()), + } + result["source_row_groups"] = pq.ParquetFile(parquet).metadata.num_row_groups + print(json.dumps(result)) + + +def main() -> None: + print(f"{N:,} rows; row groups of {WRITE['row_group_size']:,} asked for\n") + for threads in ("1", "3", None): + env = dict(os.environ) + if threads is None: + env.pop("POLARS_MAX_THREADS", None) + else: + env["POLARS_MAX_THREADS"] = threads + done = subprocess.run( + [sys.executable, __file__, "--child"], env=env, capture_output=True, text=True, check=True + ) + r = json.loads(done.stdout.strip().splitlines()[-1]) + groups = r["source_row_groups"] + print(f"polars {r['polars']} on {r['threads']} thread(s); the parquet source has {groups} row groups") + for name in ("csv", "parquet"): + s = r[name] + print( + f" {name:8s} source: {s['row_groups']} row groups, full groups exact={s['full_groups_exact']}, " + f"layout {s['layout']}, index 0..N-1={s['index_is_0_to_n']}, " + f"index equals file position={s['index_is_file_position']}" + ) + + +if __name__ == "__main__": + child() if "--child" in sys.argv else main() diff --git a/scripts/spike_reset_roundtrip.py b/scripts/spike_reset_roundtrip.py new file mode 100644 index 0000000..e9c1c4e --- /dev/null +++ b/scripts/spike_reset_roundtrip.py @@ -0,0 +1,93 @@ +"""ADR-007 evidence (D13, D14): a reset back and then forward, played through on today's code. + +Three kinds of file sit behind an entry, and ``reset_to`` treats them differently: + +- the entry directory: moved to the bullpen, copied back by a forward reset; +- the snapshots under ``compute_cache/``: the same, driven by a git-tracked list of the files that existed; +- the content-addressed clone of the source under ``data/.cas/``: DELETED by ``gc_cas``, not moved. + +Step s1 has an entry over ``orders.parquet``. Step s2 adds a cheap entry and a worthy entry over ``extra.parquet``. +The script resets to s1, resets forward to s2, and reads both s2 entries. ``extra.parquet`` is never touched. It then +empties the cache, which is the cold state of ADR-007 D7, and reads the worthy entry again so that it has to heal. + +Runs in a scratch ``TALLYMAN_HOME``. + + uv run python scripts/spike_reset_roundtrip.py +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_reset_roundtrip_")) +os.environ["TALLYMAN_HOME"] = str(HOME) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import pandas as pd # noqa: E402 + +from tallyman_core import catalog_state as cs # noqa: E402 +from tallyman_core import data_dir, ensure_project, set_active_project # noqa: E402 +from tallyman_core.paths import compute_cache_dir # noqa: E402 +from tallyman_xorq import build_and_persist # noqa: E402 +from tallyman_xorq.result_cache import cached_result_expr # noqa: E402 + +PROJECT = "spike" + + +def recipe(source: str, tail: str = "") -> str: + return ( + "from tallyman_xorq.io import read_project_file\n" + f"t = read_project_file({source!r}, project={PROJECT!r})\n" + f"expr = t{tail}\n" + ) + + +def clones() -> list[str]: + cas = data_dir(PROJECT) / ".cas" + return sorted(p.name[:8] for p in cas.iterdir()) if cas.is_dir() else [] + + +def read(label: str, content_hash: str) -> None: + cached_result_expr.cache_clear() + try: + print(f" {label}: ok, {len(cached_result_expr(PROJECT, content_hash).execute())} rows") + except Exception as exc: # noqa: BLE001 - the spike reports whatever is raised + print(f" {label}: raises {type(exc).__name__}: {str(exc)[:80]}") + + +def main() -> None: + ensure_project(PROJECT) + set_active_project(PROJECT) + cs.ensure_catalog_repo(PROJECT) + data = data_dir(PROJECT) + data.mkdir(parents=True, exist_ok=True) + pd.DataFrame({"region": ["a", "b", "a"], "price": [1.0, 2.0, 3.0]}).to_parquet(data / "orders.parquet") + pd.DataFrame({"k": ["x", "y", "x", "z"], "v": [1.0, 2.0, 3.0, 4.0]}).to_parquet(data / "extra.parquet") + + build_and_persist(PROJECT, recipe("orders.parquet")) + s1 = cs.checkpoint_catalog(PROJECT, "s1") + cheap = build_and_persist(PROJECT, recipe("extra.parquet", ".filter(t.v > 1)")) + worthy = build_and_persist(PROJECT, recipe("extra.parquet", ".group_by('k').aggregate(s=t.v.sum())")) + s2 = cs.checkpoint_catalog(PROJECT, "s2") + + print(f"at s2, clones: {clones()}") + read("cheap entry", cheap.content_hash) + read("worthy entry", worthy.content_hash) + + cs.reset_to(PROJECT, s1) + print(f"after the reset to s1, clones: {clones()}") + cs.reset_to(PROJECT, s2) + print(f"after the reset forward to s2, clones: {clones()}; extra.parquet is unchanged on disk") + read("cheap entry, which reads the clone on every read", cheap.content_hash) + read("worthy entry, whose snapshot came back from the bullpen", worthy.content_hash) + + for snapshot in compute_cache_dir(PROJECT).rglob("*.parquet"): + snapshot.unlink() + print("with the cache emptied, the worthy entry has to heal from its build:") + read("worthy entry", worthy.content_hash) + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_row_order_joins.py b/scripts/spike_row_order_joins.py new file mode 100644 index 0000000..ab7600d --- /dev/null +++ b/scripts/spike_row_order_joins.py @@ -0,0 +1,93 @@ +"""ADR-008 evidence (D6): what happens to ``__row_order`` when entries are joined. + +Every file tallyman reads carries ``__row_order`` (ADR-008 D2), so both sides of a join have a column of that name and +ibis renames the right side's to ``__row_order_right``. Raw xorq plus tallyman's ``_canonical_sorted``. + +Questions, in the order printed: + +1. A two-way join: which columns come out? +2. A three-way join written in one recipe: does it build, and where does it fail? +3. A join ENTRY's snapshot keeps ``__row_order_right`` as data. Can that entry be joined to a third entry? +4. The same, when the writer drops ``__row_order_right`` from the snapshot (the rule D6 adopts). +5. What an author has to write for a three-way join in one recipe. +6. After a fan-out join or an outer join, is the left side's ``__row_order`` still unique and non-null? + + uv run python scripts/spike_row_order_joins.py +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_row_order_joins_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 +from xorq.ibis_yaml.compiler import build_expr # noqa: E402 + +from tallyman_xorq.source_cache import _canonical_sorted # noqa: E402 + +N = 5 + + +def entry(name: str) -> Path: + path = HOME / f"{name}.parquet" + pq.write_table( + pa.table({"k": list(range(N)), name: [f"{name}{i}" for i in range(N)], "__row_order": list(range(N))}), path + ) + return path + + +def attempt(label: str, fn) -> object: + try: + out = fn() + except Exception as exc: # noqa: BLE001 - the spike reports whatever is raised + print(f" {label}: raises {type(exc).__name__}: {str(exc)[:100]}") + return None + shown = list(out.columns) if hasattr(out, "columns") else out + print(f" {label}: ok -> {shown}") + return out + + +def main() -> None: + a, b, c = (xo.deferred_read_parquet(str(entry(n))) for n in ("a", "b", "c")) + + print("1. two-way join") + ab = attempt("a.join(b, 'k')", lambda: a.join(b, "k")) + + print("2. three-way join in one recipe") + abc = a.join(b, "k").join(c, "k") + attempt("the expression's columns", lambda: abc) + attempt("build_expr", lambda: Path(build_expr(abc, builds_dir=HOME / "builds")).name) + attempt("_canonical_sorted (the next build step)", lambda: _canonical_sorted(abc)) + attempt("execute", lambda: len(abc.execute())) + + print("3. a join entry's snapshot, with __row_order_right kept as data, joined to a third entry") + kept = HOME / "ab_kept.parquet" + pq.write_table(ab.to_pyarrow(), kept) + ab_kept = xo.deferred_read_parquet(str(kept)) + attempt("execute", lambda: len(ab_kept.join(c, "k").execute())) + + print("4. the same, when the writer drops __row_order_right") + dropped = HOME / "ab_dropped.parquet" + pq.write_table(ab.to_pyarrow().drop_columns(["__row_order_right"]), dropped) + ab_dropped = xo.deferred_read_parquet(str(dropped)) + attempt("execute", lambda: len(ab_dropped.join(c, "k").execute())) + + print("5. a three-way join in one recipe, with __row_order dropped on the right-hand inputs") + attempt("execute", lambda: len(a.join(b.drop("__row_order"), "k").join(c.drop("__row_order"), "k").execute())) + + print("6. the left side's __row_order after a fan-out join and after an outer join") + twice = xo.union(b, b).drop("__row_order") + fan = a.join(twice, "k").execute() + print(f" fan-out join: {len(fan)} rows, {fan['__row_order'].nunique()} distinct values of __row_order") + outer = a.filter(a.k < 3).outer_join(c.drop("__row_order"), "k").execute() + print(f" outer join: {len(outer)} rows, {int(outer['__row_order'].isna().sum())} with a null __row_order") + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_single_partition_loaded_build.py b/scripts/spike_single_partition_loaded_build.py new file mode 100644 index 0000000..be688c6 --- /dev/null +++ b/scripts/spike_single_partition_loaded_build.py @@ -0,0 +1,98 @@ +"""ADR-009 evidence (D1): making a LOADED BUILD run single-partition. + +``scripts/spike_float_aggregate_digest.py`` runs SQL on a connection it made and configured itself. ``materialize`` +(ADR-007 D4) executes a build that ``load_expr`` loaded, and ``load_expr`` makes its own backend objects. + +Questions, in the order printed. A float SUM and AVG group-by over 3,000,000 rows, distinct value digests in 5 runs: + +1. The loaded build, executed as loaded. +2. The same, while a single-partition connection exists on the side. Which backend does the build run on? +3. The loaded build rebound onto the single-partition connection with ``replace_sources``. +4. ``SET`` applied to the backends the load made, with no rebinding. +5. Does either change the process default backend, which serves page reads? + + uv run python scripts/spike_single_partition_loaded_build.py +""" + +from __future__ import annotations + +import hashlib +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_single_partition_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import numpy as np # noqa: E402 +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 +from xorq.common.utils.graph_utils import find_all_sources, replace_sources # noqa: E402 +from xorq.config import default_backend # noqa: E402 +from xorq.ibis_yaml.compiler import build_expr, load_expr # noqa: E402 + +N = 3_000_000 +RUNS = 5 +SINGLE = "SET datafusion.execution.target_partitions = 1" + + +def digest(expr) -> str: + table = expr.to_pyarrow() + h = hashlib.sha256() + for column in ("s", "m"): + h.update(np.asarray(table[column].to_numpy()).tobytes()) + return h.hexdigest()[:10] + + +def partitions(con) -> str: + return str(con.raw_sql("SHOW datafusion.execution.target_partitions").to_pandas().iloc[0, 1]) + + +def report(label: str, make) -> None: + seen = {digest(make()) for _ in range(RUNS)} + print(f" {label}: {len(seen)} distinct digest(s) in {RUNS} runs") + + +def main() -> None: + rng = np.random.default_rng(3) + source = HOME / "source.parquet" + table = pa.table({"g": rng.integers(0, 500, size=N), "v": rng.normal(scale=1e6, size=N)}) + pq.write_table(table, source, row_group_size=100_000) + t = xo.deferred_read_parquet(str(source)) + build_dir = Path( + build_expr(t.group_by("g").agg(s=t.v.sum(), m=t.v.mean()).order_by("g"), builds_dir=HOME / "builds") + ) + + print("1. the loaded build, executed as loaded") + report("as loaded", lambda: load_expr(build_dir)) + + print("2. a single-partition connection on the side; the build is not rebound") + side = xo.connect() + side.raw_sql(SINGLE) + backends = find_all_sources(load_expr(build_dir)) + print(f" side connection reports {partitions(side)}; the build's {len(backends)} backend(s) report", end=" ") + print( + f"{[partitions(b) for b in backends]}, and none is the side connection: {all(b is not side for b in backends)}" + ) + report("not rebound", lambda: load_expr(build_dir)) + + def rebound(): + loaded = load_expr(build_dir) + return replace_sources({id(b): side for b in find_all_sources(loaded)}, loaded) + + def set_on_loaded(): + loaded = load_expr(build_dir) + for b in find_all_sources(loaded): + b.raw_sql(SINGLE) + return loaded + + print("3. rebound onto the single-partition connection") + report("replace_sources", rebound) + print("4. SET on the backends the load made") + report("SET on each", set_on_loaded) + print(f"5. the process default backend reports {partitions(default_backend())}") + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_sort_grafting.py b/scripts/spike_sort_grafting.py new file mode 100644 index 0000000..1f3c393 --- /dev/null +++ b/scripts/spike_sort_grafting.py @@ -0,0 +1,139 @@ +"""ADR-008 evidence (D10, D11, and the uniqueness note under D5): where a unique sort has to be imposed. + +Paddy's rule: when a supplied sort is not deterministic, the natural order is imposed into each ``order_by``. Today +tallyman extends an author's sort only when it is the TOP node of the expression (``source_cache._canonical_sorted``). +``original_row_order`` and ``id`` stand in for ``__row_order``, which is not implemented yet. + +Questions, in the order printed: + +1. An ``order_by`` followed by another step: in what order is the snapshot written? +2. A top-3 entry: is it written in rank order? +3. A sort that feeds a ``limit``, with ties at the cut: which ROWS come back, run to run? +4. A window function: are its VALUES the same run to run, with no order, a tied order, and a unique order? +5. Can parquet statistics tell a unique column from one with duplicates? + + uv run python scripts/spike_sort_grafting.py +""" + +from __future__ import annotations + +import hashlib +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_sort_grafting_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import numpy as np # noqa: E402 +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 +import xorq.vendor.ibis.expr.operations as ops # noqa: E402 + +from tallyman_xorq.source_cache import _canonical_sorted, _is_worthy_expr # noqa: E402 + +N = 3_000_000 +RUNS = 5 + + +def connection(partitions: int | None): + con = xo.connect() + if partitions is not None: + con.raw_sql(f"SET datafusion.execution.target_partitions = {partitions}") + return con + + +def keys_of(expr) -> list[str]: + node = expr.op() + if not isinstance(node, ops.Sort): + return [] + return [k.expr.name + ("" if k.ascending else " desc") for k in node.keys] + + +def nonfinal_sorts() -> None: + small = HOME / "small.parquet" + rows = {"name": list("abcdef"), "amount": [40, 10, 60, 20, 50, 30], "original_row_order": list(range(6))} + pq.write_table(pa.table(rows), small) + t = xo.deferred_read_parquet(str(small)) + by_amount = t.order_by(t.amount.desc()) + shapes = { + "order_by last": by_amount, + "order_by, then mutate": by_amount.mutate(double=t.amount * 2), + "order_by, then select": by_amount.select("name", "amount", "original_row_order"), + "order_by, then filter": by_amount.filter(t.amount > 15), + } + print("1. parent rows are in the order 40, 10, 60, 20, 50, 30; the author asks for amount descending") + for label, expr in shapes.items(): + written = _canonical_sorted(expr) + print(f" {label:24s} worthy={_is_worthy_expr(expr)!s:5s} sort keys={keys_of(written)}") + print(f" {'':24s} written as {written.execute()['amount'].tolist()}") + top3 = _canonical_sorted(by_amount.limit(3)).execute()["amount"].tolist() + print(f"2. top 3 by amount is written as {top3}; the author asked for [60, 50, 40]") + + +def big_file() -> Path: + rng = np.random.default_rng(5) + path = HOME / "big.parquet" + table = pa.table({"id": np.arange(N), "g": rng.integers(0, 200, size=N), "v": rng.integers(0, 1000, size=N)}) + pq.write_table(table, path, row_group_size=100_000) + return path + + +def sort_feeds_limit(path: Path) -> None: + def row_set(partitions, keys) -> str: + t = xo.deferred_read_parquet(str(path), con=connection(partitions)) + ids = np.sort(t.order_by(keys).limit(1000).to_pyarrow()["id"].to_numpy()) + return hashlib.md5(ids.tobytes()).hexdigest()[:10] + + print(f"3. order_by(g).limit(1000) over {N:,} rows; about 15,000 rows tie on the smallest g") + unique_answer = row_set(1, ["g", "id"]) + for label, partitions, keys in ( + ("default connection, key g", None, ["g"]), + ("single-partition, key g", 1, ["g"]), + ("default connection, key (g, id)", None, ["g", "id"]), + ): + seen = [row_set(partitions, keys) for _ in range(RUNS)] + same = all(s == unique_answer for s in seen) + print(f" {label:34s} {len(set(seen))} distinct row set(s) in {RUNS} runs; equals the (g, id) answer: {same}") + + +def window_values(path: Path) -> None: + def digest(partitions, shape) -> str: + t = xo.deferred_read_parquet(str(path), con=connection(partitions)) + total = { + "no order": lambda: t.v.cumsum(), + "order_by g (tied)": lambda: t.v.cumsum(order_by=t.g), + "order_by (g, id)": lambda: t.v.cumsum(order_by=[t.g, t.id]), + }[shape]() + out = t.mutate(c=total).order_by("id").select("c").to_pyarrow() # final order fixed, so only values can differ + return hashlib.md5(out["c"].to_numpy().tobytes()).hexdigest()[:10] + + print("4. cumsum() as a computed column; distinct sets of VALUES") + for shape in ("no order", "order_by g (tied)", "order_by (g, id)"): + for label, partitions in (("default connection", None), ("single-partition", 1)): + seen = {digest(partitions, shape) for _ in range(RUNS)} + print(f" {shape:20s} {label:20s} {len(seen)} in {RUNS} runs") + + +def statistics() -> None: + path = HOME / "stats.parquet" + pq.write_table(pa.table({"unique": [0, 1, 2, 3, 4], "duplicated": [0, 0, 2, 3, 4]}), path, write_statistics=True) + group = pq.ParquetFile(path).metadata.row_group(0) + print("5. parquet statistics of a unique column and of one holding 0 twice") + for i in range(2): + s = group.column(i).statistics + name = group.column(i).path_in_schema + print(f" {name:11s} min={s.min} max={s.max} nulls={s.null_count} has_distinct_count={s.has_distinct_count}") + + +def main() -> None: + nonfinal_sorts() + path = big_file() + sort_feeds_limit(path) + window_values(path) + statistics() + + +if __name__ == "__main__": + main() diff --git a/scripts/spike_stream_order.py b/scripts/spike_stream_order.py new file mode 100644 index 0000000..0eed4bd --- /dev/null +++ b/scripts/spike_stream_order.py @@ -0,0 +1,82 @@ +"""ADR-007 evidence (open question 1, the alternative that was not taken) and ADR-009 (D1). + +On the connection ``materialize`` uses (``target_partitions = 1``), do the rows of a row-preserving plan reach the +writer in the parent file's order? If so, a writer could number rows in parent order with no ``__row_order`` column +carried through the recipe, which is what materializing every entry would have relied on. It is also why a bare +``limit`` or a window function with no order is repeatable at materialization: by the engine's behaviour, not by the +query's own meaning (ADR-008 D10). + +The parent is 3,000,000 rows in 100,000-row groups, above the 10,485,760-byte scan-split threshold. ``id`` is the file +position. Each plan is streamed three times per connection. + + uv run python scripts/spike_stream_order.py +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +HOME = Path(tempfile.mkdtemp(prefix="spike_stream_order_")) +os.environ["XORQ_CACHE_DIR"] = str(HOME / "_global_xorq") # must be set before xorq is imported + +import numpy as np # noqa: E402 +import pyarrow as pa # noqa: E402 +import pyarrow.parquet as pq # noqa: E402 +import xorq.api as xo # noqa: E402 + +N = 3_000_000 +RUNS = 3 + + +def in_file_order(expr) -> bool: + last = -1 + for batch in expr.to_pyarrow_batches(): + ids = batch.column("id").to_numpy() + if len(ids) == 0: + continue + if ids[0] < last or (np.diff(ids) < 0).any(): + return False + last = ids[-1] + return True + + +def plans(t) -> dict: + return { + "bare read": t, + "filter + computed column": t.filter(t.g < 150).mutate(w=t.v * 2), + "select two columns": t.select("id", "s"), + "string filter + cast": t.filter(t.s.endswith("7")).mutate(gf=t.g.cast("float64")), + "window function with no order": t.mutate(c=t.v.cumsum()), + } + + +def main() -> None: + rng = np.random.default_rng(11) + parent = HOME / "parent.parquet" + columns = { + "id": np.arange(N), + "g": rng.integers(0, 200, size=N), + "v": rng.normal(size=N), + "s": pa.array([f"row-{i % 1000}" for i in range(N)]), + } + pq.write_table(pa.table(columns), parent, row_group_size=100_000) + print( + f"parent: {parent.stat().st_size / 1e6:.0f} MB, {pq.ParquetFile(parent).metadata.num_row_groups} row groups\n" + ) + + for label, partitions in (("target_partitions = 1", 1), ("default connection", None)): + print(label) + for name in plans(xo.deferred_read_parquet(str(parent))): + results = [] + for _ in range(RUNS): + con = xo.connect() + if partitions is not None: + con.raw_sql(f"SET datafusion.execution.target_partitions = {partitions}") + results.append(in_file_order(plans(xo.deferred_read_parquet(str(parent), con=con))[name])) + print(f" {name:32s} streamed in file order: {results}") + + +if __name__ == "__main__": + main()