refactor!: audit and prune redundant planning enums - #447
Merged
Merged
Conversation
Downstream repos depend on Sketchlib by bare git URL; Cargo treats ?rev= as a distinct source, so consumers got two copies of asap_sketchlib. Cargo.lock keeps builds reproducible. Closes #450 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Closes #428. Remove redundant planning representations while retaining enums with implemented consumers. The enum audit and consumer migration are included below.
What
ExecutableOperator,ExecutableDagNode.operator,MaterializationMedium, andSummaryWindowFramework::Extension(String).CostUnitfrom mapping; recurring formulas reject total-cost units.PhysicalHandoffKind,CacheProfile,EvaluationSchedule,QueryTimeScope,Predictability, andExponentialHistogrambecause they drive estimation, lifecycle selection, or guarantee validation.How
Use
ExecutableOperatorPayloadas the sole operator identity. Keep network/materialization byte dimensions separate, without an unused storage-medium label. Limit window frameworks to those with defined planning semantics.Before this PR
Compiling a sum into an executable DAG emitted both
operator: "SummaryAgg"andpayload.kind: "summary_agg"; these could disagree. Declaring a materialization required a medium even though all media used the same byte counter and coefficient. Mapping and shared cost annotations used distinct RustCostUnittypes.After this PR
The same sum emits only
payload.kind: "summary_agg"in a version-2 executable DAG. A materialization is{"kind":"materialization"}and preserves its byte estimate. Mapping and annotations share one cost-unit type; recurring formulas rejectCostUnitsinstead of interpreting a total as a rate.Evidence
The executable-DAG round-trip test checks payload-only node identity, rejection of the removed field, and rejection of document version 1. Physical handoff integration coverage preserves distinct once-only materialization and per-consumer transfer multiplicities. The rate-unit regression failed before the guard was added.
Verification
cargo test --workspace --locked— 1,165 passed.cargo fmt --all -- --check— passed.cargo clippy --workspace --all-targets --all-features --locked -- -D warnings— passed.New tests verify that opaque window extensions cannot enter planning and that shared total-cost units cannot enter recurring formulas. Existing DAG and physical handoff tests verify the simplified wire contracts.
Architectural decisions
Keep enums with actual semantic consumers: for example, predictable advance notice enables prepared state, moving real-time lookbacks require deletion support, and exponential histograms require framework-specific accuracy evidence. Remove the opaque window extension because name validation alone supplies no framework semantics.
Planning enum audit and migration
This records the current code audit for issue #428 for library and wire-contract
consumers. Retention decisions depend on implemented consumers, not future uses.
PhysicalHandoffKindphysical_handoff_cost::estimate_physical_handoffsdistinguishes network endpoint validation and network bytes from materialization bytes.CacheProfileanalytical_costresolves cache evidence into hit/miss work;physical_plan_cost_modelrejects cache evidence for boundary/storage models that cannot account for it.SummaryWindowFramework::ExponentialHistogramsummary_maintenance_cost/window.rschecks framework-specific accuracy evidence; candidate ranking composes its error with the root guarantee and checks the required accuracy.SummaryWindowFramework::Extension(String)EvaluationScheduleQueryTimeScopePredictabilityExecutableOperatorandExecutableDagNode.operatorpayload.kindin JSON.CostUnitasap_types::cost::CostUnit. Recurring formulas requireCostUnitsPerSecond; totals are rejected.MaterializationMediumConsumer migration
The executable DAG document version is 2. Nodes no longer contain an
operatorfield: for example, a summary aggregate has only"payload": {"kind": "summary_agg", ...}for its operator identity. Updateconsumers to match
ExecutableOperatorPayloador inspectpayload.kind.Version 1 documents are not supported; regenerate them with the new compiler.
The strict node decoder rejects the removed field.
Boundary materialization JSON changes from
{"kind":"materialization","medium":"disk"}to{"kind":"materialization"}. Rust callers useBoundaryKind::Materialization.Remove the medium from supplied boundary profiles. Network evidence and byte
counters are unchanged. A boundary is still explicitly declared; ordinary
in-memory dataflow does not become a materialization.
Window frameworks accept
tumbling,sliding, andexponential_histogram.Opaque
{"extension":"..."}values are rejected. New frameworks need definedplanning and accuracy semantics before entering this contract.
Existing mapping imports of
CostUnitremain valid as re-exports of the sharedtype. Its existing shared serde representation is unchanged;
as_str()retainscost_units_per_secondand also names totals ascost_units.Known downstream migration
The local ASAPQuery backend checkout still imports
ExecutableOperatorincrates/asap_types/src/executable_plan.rsand constructs the duplicated fieldin runtime tests. Those consumers must migrate to the payload before upgrading
the planner dependency. Its window compiler uses
Extensiononly in a testthat rejects an unsupported hierarchical rollup; move that check to invalid
wire input or an unsupported layout. No materialization-medium consumer was
found there. This PR changes ASAPPlanner only.
Limitations and follow-up
Breaking API/wire cleanup: regenerate version-1 executable DAG documents; consumers must match the payload, remove materialization media from profiles, and stop supplying window extensions. The local ASAPQuery backend still imports the old executable operator API and must migrate before upgrading its planner dependency; its only window-extension use is a test rejecting an unsupported layout. No materialization-medium consumer was found there. Backend changes are outside this PR.
Human review — do not complete with an agent