Skip to content

feat(cost): add cache evidence to physical plan estimates - #353

Merged
zzylol merged 6 commits into
mainfrom
fix/issue-349-cache-cost
Sep 8, 2026
Merged

zzylol merged 6 commits into
mainfrom
fix/issue-349-cache-cost

Conversation

@zzylol

@zzylol zzylol commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Why

Repeated-query caching changes the baseline cost and can change whether retained summaries are worthwhile. Addresses the physical-plan portion of #349; #345 and #346 remain separate PRs.

What and how

Add versioned result-cache and buffer-cache evidence to physical comparisons. Capacity/working-set residency and declared streaming invalidation determine result hits; result hits reduce CPU and reads, while buffer hits reduce reads only (including build-once scans). Both alternatives share immutable cache assumptions. Export demand, capacities, hit ratios, and provenance; reject aggregation across incompatible profiles. Legacy JSON defaults to named no-cache.

Before this PR

Six evaluations of a 100-row, 1,000-byte scan cost 600 CPU operations and 6,000 scan bytes, regardless of identical repeats.

After this PR

Two distinct evaluations plus four identical repeats with a resident result cache cost 200 CPU operations and 2,000 scan bytes. With 50% buffer residency, reads fall to 1,000 bytes while CPU stays at 200. These hand-computed estimates are executable tests, not runtime measurements.

Verification

  • Independent review identified numeric overflow, provenance mixing, and compatibility gaps; fixes include failing-before/passing-after regressions.
  • Passed cargo test --workspace --locked, including current main's test(promql): verify TopK planning and numeric reference results #351 tests.
  • Passed cargo clippy --workspace --all-targets --all-features --locked -- -D warnings, formatting, and diff checks.
  • Passed all 25 viewer Python tests, including JavaScript execution with py_mini_racer.
  • JSON-to-ranking/export tests cover warm caching, distinct-only demand, invalid evidence, and legacy inputs. Unit tests cover full-width integer accounting, build-once buffer reads, and streaming invalidation.

Architectural decisions

Cache evidence belongs to the immutable deployment snapshot, not logical operators. Shared annotations carry profile identity without depending on estimator types. Legacy non-cache models retain their existing aggregation; known and missing profiles cannot be mixed.

Limitations and follow-up

This is ready for human review of the scoped physical-plan implementation; it does not close all of #349. The summary-maintenance lifecycle adapter still uses the compatibility no-cache estimator. Residency is a steady-state heuristic without cold-start ordering, replacement policy, or lookup/fill overhead. Streaming invalidation is supplied, not derived from arrival rate and query schedule. Rust snapshot/request constructors require the new fields. Storage request counts and boundary bytes are separate work.

Screenshots and empirical runtime benchmarks: not applicable to the claims here; none were produced.

Human review — do not complete with an agent

  • The MVP boundary is correct.
  • New conceptual layers or public interfaces are necessary.
  • The before/after description matches the intended product behavior.
  • Human reviewer:
  • Decision and rationale:

@zzylol
zzylol marked this pull request as ready for review September 7, 2026 16:45
@zzylol
zzylol requested a review from Selvomega September 7, 2026 16:54
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Review and fixes pushed: 701ab8b + ff1a99e, after merging main in 8067f7f and adapting to the #357 shared resource API. Findings: (1) cache hit subtraction and floating-point ceil could erase genuine misses or miscount bytes, changing final plan selection. Regressions include a nearly resident cache incorrectly reading 0 instead of 1 byte, full-width integer loss, and ordinary scan=15 / working-set=3 / capacity=2 incorrectly reading 6 instead of 5 bytes. Miss fractions now use integer uncovered bytes and integer-execution byte accounting uses checked full-width rational arithmetic. (2) missing base calibration provenance was accepted; blank versions now fail before ranking. Six new regressions were observed failing under the original logic and passing after fixes, including actual global selection. Validation: cargo test --workspace --locked (371 mapping tests); strict workspace/all-target/all-feature clippy; 25 viewer tests; format/diff checks. Normal push, no PR merge, no human-review attestation.

@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Updated in 9e5cba5: truly rebased onto main c3410d1 (linear history), preserving all previous arithmetic and ranking regressions. CacheProfile, CacheEvidence, and CacheCapacityEvidence now have one canonical definition in asap_types::resources, with legacy mapping type paths re-exported. Cache assumptions remain distinct from additive resource consumption. Numerical validation and cost calculations stay in mapping; callers now use cache_hit_ratios(&profile, count, arrival), documented in the migration notes. Shared JSON compatibility and type-identity tests were added. Validation passed: full workspace tests --locked (372 mapping, 167 types, 20 dag_export), strict workspace Clippy across all targets/features, viewer tests (25), and formatting. Updated with an explicit force-with-lease; not merged.

@zzylol
zzylol force-pushed the fix/issue-349-cache-cost branch from ff1a99e to 9e5cba5 Compare September 8, 2026 19:13
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: split the remaining shared definitions into resources/cpu.rs (ModeledCpu and MeasuredCpu), resources/measurement.rs (Measurement), and resources/physical.rs (PhysicalResources and MeasuredResources). resources.rs retains module declarations, re-exports, and cross-resource contract tests. Existing root import paths, fields, units, defaults, and JSON formats are unchanged. Full workspace tests --locked and strict workspace Clippy across all targets/features pass.

@zzylol
zzylol merged commit 53e76d4 into main Sep 8, 2026
3 checks passed
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