Skip to content

feat(cost): estimate network and materialization boundary bytes - #355

Merged
zzylol merged 8 commits into
mainfrom
feat/issue-346-boundary-bytes
Sep 8, 2026
Merged

zzylol merged 8 commits into
mainfrom
feat/issue-346-boundary-bytes

Conversation

@zzylol

@zzylol zzylol commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Why

Physical-plan selection needs explicit evidence to price network transfers and materialization; logical in-memory edges are insufficient. Addresses #346 independently of #345 and #349.

What and how

Bind explicit actions to complete physical alternatives. Cost bytes as encoded_bytes × copies × executions; shared actions follow producer multiplicity and per-consumer actions follow the actual consumer. Exactly one fresh root/node/statistics binding must match each alternative. Add calibrated network/materialization costs to ranking and export totals, node/boundary terms, and provenance. Positive boundary-only pricing permits zero base coefficients.

Before this PR

A retained producer with two consumers has no estimate for initial materialization or repeated transfers, so neither affects selection.

After this PR

Materializing 40 encoded bytes once and sending two copies to each of two consumers over three evaluations produces 40 materialization bytes and 480 network bytes. Ordinary memory edges produce no boundary traffic. Alternatives sharing a producer ID can declare different consumers without mixing evidence.

Verification

  • Independent review found alternative-specific boundary evidence was not representable. A failing-before/passing-after regression covers the fix; another covers boundary-only pricing.
  • 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.
  • Six boundary integration tests cover shared actions, consumer multiplicity, missing/ambiguous/mismatched plans, invalid endpoints/bytes, duplicate IDs, stale evidence, coefficients, and overflow.
  • JSON-to-ranking/export coverage verifies 12,000 candidate network bytes, preference reversal, boundary-only pricing, and ambiguous-profile rejection.
  • All 22 viewer Python tests passed, including standalone annotation preservation.

Examples are executable analytical estimates. Screenshots and runtime throughput measurements: not applicable; none were produced.

Architectural decisions

Boundary profiles contain plan-scoped node evidence because a shared producer can have different consumers in different alternatives. Full snapshot matching prevents rebinding; explicit action IDs distinguish shared transfers from independent ones. Existing generic cost inputs expose dimensions without a new viewer format.

Limitations and follow-up

Models bytes, not latency, contention, memory lifetime, or storage request counts. Lifecycle-specific maintenance and cache integration remain separate. Rust constructors gain an optional profile field; existing JSON without it remains supported. This branch includes current main and is not stacked on the other cost PRs.

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:46
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Review and fixes pushed in 4e6e15b (plus main merge f851b52, including shared physical resources from #357). Finding P2: PhysicalPlanCostModel accepted an empty/whitespace base calibration version, including boundary-only objectives, leaving combined cost provenance incomplete. Added a failing-before/passing-after regression and reject it before ranking. Also added passing boundary accumulation and calibrated-overflow regressions; no new byte/multiplicity bug was found in those cases. Validation passed: cargo test --workspace --locked; cargo clippy --workspace --all-targets --all-features --locked -- -D warnings; format/diff checks; 22 viewer tests. Pushed normally, no force push and no PR merge. Human-review checklist remains untouched.

@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Updated in f06e275: rebased onto main c3410d1 (linear PR history, no merge commits), preserving the previous correctness fixes. BoundaryResources, BoundaryKind, and MaterializationMedium now have a single canonical definition in asap_types::resources; the mapping module re-exports these types for import compatibility. Estimation and calibration remain in the mapping layer. Added shared-schema serialization and checked-add overflow tests plus a legacy-import type-identity regression. Validation passed: workspace tests --locked, boundary integration tests (8), strict workspace Clippy (all targets/features), formatting, and viewer tests (22). Branch updated with an explicit force-with-lease; this PR has not been merged.

@zzylol
zzylol force-pushed the feat/issue-346-boundary-bytes branch from 4e6e15b to f06e275 Compare September 8, 2026 19:11
@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 force-pushed the feat/issue-346-boundary-bytes branch from a83aa89 to e5136d9 Compare September 8, 2026 19:33
@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Kept boundary.rs and documented its scope in English code comments: network transfers and materialization in memory, on disk, or in object storage; byte work is distinct from occupancy and request counts. Integrated cache and storage changes with a joint regression proving both supplemental costs and their provenance survive. CacheProfile::Evidence plus boundaries now fails closed because evaluation-based multiplicity cannot express post-cache executions; the regression failed before this guard and passes afterward. Explicit NoCache remains supported. Full combined workspace tests and strict all-target/all-feature Clippy pass. Waiting for CI and #354 merge before finalizing history and merging.

@zzylol

zzylol commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

All combined local tests, strict Clippy, and GitHub CI passed on e5136d9. After #354 was squash-merged as e6fb07f, rebased only the boundary commits onto that main. Final head 6da1673 has exactly the same tested Git tree 935a1b8e5c1cfa13895342803db14f77479265ed; git diff e5136d9 HEAD is empty. No source changes were made after validation.

@zzylol
zzylol force-pushed the feat/issue-346-boundary-bytes branch from e5136d9 to 6da1673 Compare September 8, 2026 19:39
@zzylol
zzylol merged commit abb2f20 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