Skip to content

perf(api): paginate /api/data off cached_result_expr, not a full result.parquet (#90) - #92

Merged
paddymul merged 2 commits into
mainfrom
perf/90-api-data-cached-result-expr
Jun 16, 2026
Merged

paddymul merged 2 commits into
mainfrom
perf/90-api-data-cached-result-expr

Conversation

@paddymul

Copy link
Copy Markdown
Contributor

Addresses #90.

What

/api/data (app.py:518) paginated a windowed read by first materialising the entry's entire result to disk: it called ensure_result unconditionally, which cached_result_expr(...).to_parquet(result_path)s the full result, then read offset+limit rows back and sliced. An O(limit) request did O(rows) work and wrote a file — for a cheap entry that reverses #74's "materialise nothing"; for an expensive entry it duplicates the baked snapshot under the entry.

This serves the page straight off the expression cached_result_expr already hands back:

  • total comes from the manifest's row_count (recorded at build; api_entry_detail reads it the same way), so no file is needed to populate it.
  • The page is cached_result_expr(project, content_hash).limit(limit, offset=offset).execute(). The window pushes down — for an expensive entry it lowers to a windowed read of the baked snapshot, for a cheap entry the limit pushes through to the source read. Neither writes result.parquet.

ensure_result stays for the callers that need a parquet path (downloads, promote-diff, post-processing) — this is the one read path that needs rows, not a path.

The now-unused _read_head_rows helper (and its pyarrow import) are removed.

Tests

test_api_data_cheap_entry_serves_page_without_materialising and test_api_data_expensive_entry_paginates_without_per_entry_parquet assert no per-entry result.parquet is written when a page is served (cheap path returns the right page+total; expensive path paginates in order across a deep offset). Both are committed first so the regression is visible failing on CI, then the fix lands.

Context

Surfaced while reconciling the caching docs with #74. Same ensure_result-on-read mechanism as #79 (concurrent materialisation race); adjacent to #71 (viewer re-runs the recipe on load).

🤖 Generated with Claude Code

paddymul and others added 2 commits June 16, 2026 14:13
…quet

Cheap and expensive entries both write no per-entry result.parquet at build
(#74). Serving a paginated read through ensure_result reverses that by dumping
the entry's whole result to disk for an O(limit) request — and for an expensive
entry it duplicates the baked snapshot under the entry.

Both tests assert no result.parquet exists under the entry after a page read
(cheap path returns the right page+total; expensive path paginates in order
across a deep offset). They fail today because api_data routes through
ensure_result.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…result.parquet

api_data called ensure_result on every paginated read, which materialised the
entry's entire result to disk (cached_result_expr(...).to_parquet) before reading
offset+limit rows back and slicing — O(rows) work plus a file write to serve an
O(limit) request. For a cheap entry that reversed #74's "materialise nothing";
for an expensive entry it duplicated the baked snapshot under the entry.

Serve the page straight off the expression cached_result_expr already returns:
total from the manifest's row_count (api_entry_detail reads it the same way),
rows from cached_result_expr(...).limit(limit, offset=offset).execute(). The
window pushes down (expensive -> windowed read of the baked snapshot, cheap ->
limit through to the source read) and writes nothing. ensure_result stays for the
callers that need a parquet path (downloads, promote-diff, post-processing).

Removes the now-unused _read_head_rows helper and its pyarrow import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul
paddymul merged commit 8e6f1b5 into main Jun 16, 2026
4 checks passed
paddymul added a commit that referenced this pull request Jun 16, 2026
A cheap (unsorted) entry paginates by re-executing the recipe per page (no
baked snapshot — only Aggregate/Join/Sort entries bake one), so two pages
tile the result with no overlap or gap only if the source scan order is
stable across executions. The old path sliced one on-disk result.parquet,
stable by construction. Assert two pages equal the matching slices of a
single full read. Passes today — a characterization lock on the property #92
introduced, not a bug repro.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul

Copy link
Copy Markdown
Contributor Author

Post-merge review

Reviewed #92 after merge. The change is correct and a real win — it drops an O(rows) full-result write per first page load and restores #74's "materialise nothing" on the read path. Verified the property it now leans on: order-sensitive ops (Aggregate/Join/Sort) are classified worthy → baked to a stable snapshot, so only order-preserving ops take the per-page re-execute path; limit(0)/offset semantics and cross-execution scan-order stability check out empirically.

Findings and dispositions:

# Finding Disposition
1 api_data guards on entry_build_dir but reads manifest.json unguarded for total; the build dir is created before the manifest, so a half-built/pruned entry → FileNotFoundError → 500. Fixed in #95 — fix(#90) 6c5b99c (guarded read, best-effort total; repro abd5e66 seen failing on CI first).
2 total is build-time row_count, not actual rows. Accepted — consistent with api_entry_detail; only drifts for a nondeterministic source, which #88 lints against.
3 cached_result_expr's @lru_cache outlives a compute-cache prune (reset_to), so a post-reset read executes an expression pointing at a deleted snapshot; the path.exists() self-heal only runs on a cache miss. #92 widened the blast radius to the per-page endpoint. Filed as #96.
4 Pagination ordering across pages for a cheap, unsorted entry (independent per-page re-execution) was untested — the cheap test fetched only offset=0; the deep-offset test used order_by. Fixed in #95 — test(#90) 3515bf0 (two pages == matching slices of one full read).
5 Nits: inlines entry_dir(...) / ENTRY_MANIFEST_FILENAME over the entry_manifest_path() helper; tests hardcode "result.parquet". Accepted — matches the neighbouring api_entry_detail / existing test style.
6 PR body lists "downloads" as a remaining ensure_result caller; actual callers are companion/xorq diff + post_processing + salt-mode result_cache. Doc-only; no code change.

Fixes for #1 and #4 are in #95 (green on CI: ruff ✓, fast ✓, integration ✓). #3 is tracked in #96.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant