docs(plan): remove the on-demand result.parquet layer, rely on xorq .cache()

[paddymul]

Plan only — no implementation, no tests. Adds plans/remove-ondemand-result-parquet.md.

What

#73 stopped writing result.parquet at build time but kept ensure_result to regenerate it lazily; #71/#90 moved the viewer and paginated reads onto cached_result_expr. This plan finishes that arc: delete the on-demand result.parquet layer entirely and let xorq's baked .cache() snapshot (already produced by rewrite_for_build for expensive entries) be the single materialised copy. Cheap entries recompute on read, as today.

The core of the work is deleting ensure_result and pointing its three real consumers at cached_result_expr:

• post_processing → cached_result_expr(...).execute() instead of pq.read_table(ensure_result(...))
• companion/diff.build_diff_expr → cached_result_expr directly (drops a double-materialisation)
• xorq/diff.full_diff → drop the now-dead _ensure_entry_parquet fallback (all live callers already pass exprs)

Plus removing the path plumbing (ENTRY_RESULT_FILENAME, entry_result_path, the manifest result_path field), repointing the cache-admin surfaces at the baked snapshot, and migrating the cache-lab benchmark off the materialised parquet.

Decisions (resolved in a grilling pass)

• Scope A: keep the cheap/expensive classifier; remove only the result.parquet layer.
• Drop the pandas/polars full_diff backends — xorq-only (every live caller passes exprs).
• Repoint the /api/result_cache inspector + delete at the baked snapshot; the inspector reads manifest.cache_bytes (perf/caching: log the cache admission decision — structural cache_worthy verdict + measured value-per-byte (compile_seconds, bytes) side by side — so the #30 flip is data-driven #87), so the CacheEntry JSON contract and the frontend are untouched.
• Migrate the cache lab (test_cache_lab.py) onto cached_result_expr; view_timings becomes snapshot cold-vs-warm.
• Salt mode: delete the branch — reads fall through the existing recompute paths, no replacement code.

Context

Builds on #73 (drop blanket result.parquet), #71/#90 (read path onto cached_result_expr), and #87 (manifest cache_bytes, reused by the repointed inspector). Implementation will follow in a separate PR under the repo's TDD split — failing-tests commit first (post-processing / promote-diff / full_diff asserting no result.parquet is written), watched red on CI, then the fixes.

🤖 Generated with Claude Code

[paddymul]

Review: plan to remove the on-demand result.parquet layer

This is a review of the plan document. I verified every claim against main (the plan's baseline — PR 101 is just main + this one markdown file) and cross-checked against feat/remove-ondemand-result-parquet, which already implements the plan. So for each finding I can show how the real implementation resolved it.

The core mechanics are accurate and the implementation followed the plan closely. The issues below are the places where following the plan literally would have produced a bug, broken a passing test, or left dead code.

Would mislead the implementation

1. The cache inspector's "manifest, not a file walk" design is wrong (§5, plan lines 155–169). The plan drives the inspector from manifest.cache_bytes and explicitly forbids per-entry reconstruction: "No per-entry recipe reconstruction (baked_snapshot_path re-imports the recipe — too slow to call across every entry)" (line 159), with size read from manifest.cache_bytes. Both halves are wrong, because the repointed delete (§5 lines 185–189) unlinks the snapshot but never clears manifest.cache_bytes (it's written once at build, build.py:432). A cache_bytes-only listing therefore shows a phantom row after a delete: stale size, freed bytes still in the total, a delete button that 404s. The implementation had to do the opposite — keep cache_bytes as a cheap pre-filter, then call baked_snapshot_path() per surviving entry and read snap.exists() / snap.stat().st_size from the live file (app.py:1036–1042, commits 13c2fc7 + 0d3941d). An implementer following §5 verbatim reintroduces the phantom row.

2. The delete endpoint does not "self-heal on the next read" for a warm process (§5, plan line 188). cached_result_expr is @lru_cache(256) and its evicted-self-heal only fires on a cache miss. A warm entry holds a deferred_read_parquet of the file the delete just unlinked, so the next viewer read 500s ("At least one path is required") instead of re-baking. The implementation had to add cached_result_expr.cache_clear() to the delete endpoint (app.py:1109, commit bbed3ba; failing-first test e386c4b). The plan mentions cache_clear() only for the cache-lab (line 343), never for delete.

3. test_reset_to_revision.py — the premise is false (Test plan, ~lines 307–310). The plan says to switch the result.parquet stand-in "since it's no longer in ENTRY_CACHE_NAMES." But prune/restore copies the whole entry dir (catalog_state.py prune_entries / restore_from_bullpen), independent of ENTRY_CACHE_NAMES membership, so the stand-in still round-trips. The implementation left this file untouched. Following the plan would have corrupted a correct, passing test.

4. The "Error-semantics change" note cites strings that don't exist. It anchors on a broken entry "used to surface as HTTPException(404, 'result.parquet not found') / PostProcessingRunError('no result.parquet found')." The HTTPException string lives only in the DELETE endpoint (app.py:1089), not a read path; PostProcessingRunError('no result.parquet found') doesn't exist anywhere in src/, and ensure_result is called outside post_processing's PostProcessingRunError guard, so a broken entry already raised a raw, unwrapped error. The directional point (post-processing now heals like diff) is fine — the "before" anchor is invented.

Omissions the implementation had to fill

• perf(viewer): serve cache-worthy entries from result.parquet + share the path with Tier-B (#71) #102 build helpers. The branch's real baseline (post-perf(viewer): serve cache-worthy entries from result.parquet + share the path with Tier-B (#71) #102) has ensure_result_build and ensure_viewer_expanded_build with live consumers; the plan, written against main, deletes only ensure_result. Attributable to perf(viewer): serve cache-worthy entries from result.parquet + share the path with Tier-B (#71) #102 landing after the plan was drafted, but real — the structural test asserts all three are gone.
• §4 dead logging import. Once _ensure_entry_parquet is deleted, import logging + log = logging.getLogger(__name__) (diff.py:12,31) are dead. The §4 import-drop list names only pd/pq/ENTRY_RESULT_FILENAME. ruff F401 catches the import but not the module-level log binding, so a literal follower can leave dead code.
• §10 migration has no named test. The marker bump deliberately re-triggers a destructive disk sweep — that wants a regression guard. The implementation added test_migrate_drop_result_parquet_sweeps_legacy (test_build.py, commit 07b215a) and broadened the sweep to a tuple that also rmtrees the perf: viewer /load_expr sends the bare recipe for cache-worthy entries — count + every page re-runs the whole DAG instead of reading result.parquet #71 .xorq_result_build* dirs (the plan's single result.parquet literal would have left them behind).
• scripts/perf_report.py. Unnamed anywhere in the plan; it has a result.parquet size probe that would silently report None forever after the removal — repointed to res.cache_bytes.
• test_buckaroo.py. The viewer/ensure_session "no result.parquet" failing-first assertion went here (commit b350c80); the plan's Commit-1 list has no viewer-path test.

Scope note (for the implementation PR, not this doc)

The plan is correctly scoped to the result.parquet removal, but the branch that implements it also carries two unrelated, unmerged bodies of work: the #88 nondeterminism build-lint feature (plumbed through build.py/server.py/app.py plus ~6 tests) and the #71 viewer-serve-from-snapshot work (ensure_viewer_expanded_build, the .xorq_result_build* dirs). Neither is on main; the plan treats #71 as already-merged context. Worth unstacking these before the eventual implementation PR per "keep PRs focused."

Minor (line drift, all within tolerance)

build_diff_expr block cited as 224–230 in §3 vs 227–230 in the table; CachePage heading :41 vs actual :42; delete-failure alert :31 vs :30; "~20 read sites" in §12 is actually 14 pd.read_parquet(ensure_result(...)) calls (18 ensure_result calls total); cache-lab build() probe :239 not :244.

What the plan got right

The hard parts hold up. The salt fall-through reasoning is verified sound (salt skips cache injection at source_cache.py:121 → top node is never a CachedNode → _cached_node_path is None → worthy-recompute-fallback returns raw). The single-backend contract for build_diff_expr holds. The dead-result_path claim is correct — nothing in src/ reads it and pydantic ignores the unknown key, so no migration is needed. The build-dir existence proxy is valid. The JSON contract is preserved via the row_count int-coercion and created reformat, so types.ts and Header.tsx stay untouched. The test_paths.py collection-error hazard was flagged correctly. And the TDD test-vs-fix split is coherent and was honored across the implementation (failing-first commits precede their fixes).

_{Reviewed against main (baseline) and the feat/remove-ondemand-result-parquet implementation branch. 🤖 Generated with Claude Code}

[paddymul]

Closing unmerged. The implementation landed in #104, which also unwound #102 and corrected three of this plan's prescriptions during implementation. Rather than merge a plan that no longer matches reality, I've rewritten it as an as-built record in #108 (supersedes this). The forward-looking plan served its purpose; #104's PR body + #108 are the durable record.