Repository navigation
refactor(plan-selection): delete the unused physical-DAG, storage and handoff cost estimators - #833
Merged
Merged
Conversation
zzylol
force-pushed
the
cleanup-dead-cost
branch
from
October 21, 2026 19:57
f798a76 to
6080bea
Compare
… handoff cost estimators No planning stage, devtool or downstream repository reads the whole-DAG cost path any more: #667, #686 and #734 removed its last callers. Delete it, the types only it used, and the legacy wire compatibility no committed payload needs. - plan-selection: delete cost/query_physical_lowering.rs, storage_io.rs and physical_handoff_cost.rs; in analytical_cost.rs delete estimate_physical_dag{,_with_cache,_comparison}, the cache profile resolver, PhysicalDAGNode/EvidenceBackedPhysicalDAG and their validators; in physical_operator_statistics.rs delete ComparisonScope, ScanSelection, validate_comparison_scopes and OperatorStatisticsProvider; drop the error variants only they produced. - types: delete resources::{cache, physical_handoff, storage}; read Schema with a plain derive (no `columns` / bare-dtype layouts) and require AggIntent's `col` / `cols` keys. - executor: delete the `dag` compatibility re-export; tests import the real module paths. - docs: delete the storage and handoff develop docs; mark the deleted path in the analytical-cost proposal and physical-plan integration doc.
zzylol
force-pushed
the
cleanup-dead-cost
branch
from
October 21, 2026 20:34
6080bea to
5b04d65
Compare
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
Stage 3 prices each node of a physical candidate with
estimate_operator. A second, whole-DAG cost path still sits beside it: lowering a query to aPhysicalDAGNodeDAG, pricing that DAG with cache, storage-request and handoff profiles, and comparing two DAGs over oneComparisonScope. Nothing reads it any more. #667 removed its summary-maintenance caller, #686 removed thedag_exportdevtool that used the lowering and the profiles, and #734 removed the Stage 3 comparison adapter. About 5,000 lines of code and 60 tests keep compiling and passing against an API nothing calls, and two develop docs still describe it as available. This follows the delete-unused-fields rule: delete public code that no non-test code reads.What
cost/query_physical_lowering.rs,cost/storage_io.rsandcost/physical_handoff_cost.rs, plus their test files. Fromanalytical_cost.rs, delete the physical-DAG estimator and the cache resolver. Fromphysical_operator_statistics.rs, deleteComparisonScope,ScanSelectionand the statistics-provider trait. Delete theAnalyticalCostErrorvariants that only the deleted code produced.workload::resources::{cache, physical_handoff, storage}, whose only readers were the deleted estimators.Schemanow deserializes with a plain derive, so it no longer reads the oldcolumnsand bare-dtype layouts.AggIntent::Cardinality'scolskey is now required. The#[serde(default)]on theOptioncolfields is dropped as redundant: serde already reads a missingOptionfield asNone, so a measure withoutcolstill loads.dagcompatibility re-export module. The 8 test files that used it (the 6 the audit found plus 2 with a multi-line import) now import the real modules.develop_docs/physical-handoff-costs.mdandstorage-operation-costs.md. Both documented only deleted estimators. Add a status note to the analytical-resource-cost proposal and the physical-plan integration doc, and fix the vocabulary-migration record, the develop-docs index and the architecture map.Planning output does not change.
How
These are plain deletions. Once the estimator chain is gone, clippy (
-D warnings, all targets) found every private helper that had lost its only caller (validate_operator_statistics,validate_promql_child_edges,expected_input_arity/OperatorInputArity), and those are deleted too. Public items and enum variants never trigger dead-code warnings, so each one was checked with grep across the workspace and in ASAPQuery and ASAPCollector. ASAPCollector does not depend on ASAPPlanner crates. ASAPQuery-backendmaindepends onasap-plannerandasap-executorat pinned rev05fb36e4; none of the deleted items appears in it, so a later pin bump is unaffected.Before dropping the legacy wire forms, every committed JSON file was scanned (23 files: examples, workloads, settings). None has a
columnsschema, a bare-stringdtype, or afieldsschema withoutclosed. The one"kind": "sum"measure intools/dag-viewer/examples/planner-layering-example1.jsoncarries"col": null. Serialization always writesclosed,colandcols, so only hand-written legacy JSON is affected. The separate--tableJSON format is unchanged.Deleted items
The grep column is run on
origin/main(e2f9c81) asgit grep -n <name> origin/main -- crates ':!<the three deleted cost modules>' ':!crates/*/tests/**' ':!*.md', plusgit greponorigin/mainof ASAPQuery, ASAPQuery-backend and ASAPCollector, which found 0 hits in all three for every item.cost/query_physical_lowering.rs(3,028 lines, 17 unit tests)pub modline and module doc incost/mod.rs.dag_export, the lastlower_query_physical_dagcaller), and #734 (removed the Stage 3 adapter'squery_physical_loweringimport)cost/storage_io.rs(StorageIoProfile,estimate_storage_io; 3 unit tests) andtests/storage_io.rs(8 tests)StorageIoProfile: 0 hits.storage_io: only thecost/mod.rslines.crate::storage_io::estimate_storage_iofrom the physical-plan adapter); #686 removed the devtool fieldcost/physical_handoff_cost.rs(PhysicalHandoffProfile,estimate_physical_handoffs) andtests/physical_handoff_cost.rs(9 tests)PhysicalHandoffProfile: 0 hits.physical_handoff_cost: only thecost/mod.rslines.analytical_cost::estimate_physical_dag,estimate_physical_dag_with_cache,estimate_physical_dag_comparisonanalytical_cost.rstest module.estimate_physical_dagcalls), #734 (estimate_physical_dag_comparisonin the adapter)analytical_cost::cache_hit_ratios,resolve_cache_profile,ResolvedCacheProfiledag_export'scache_hit_ratioscall)PhysicalDAGNode,PhysicalNodeEvidence,EvidenceBackedPhysicalDAG,ExecutionMultiplicity,PhysicalDAGEstimateRequest,PhysicalDAGComparisonEstimate,ANALYTICAL_COST_MODEL_VERSIONanalytical_cost.rsdefinitions, the deleted estimator, and tests.ANALYTICAL_COST_MODEL_VERSION: definition only.EvidenceBackedPhysicalDAGusers)validate_operator_statistics,validate_promql_child_edges,expected_input_arity,OperatorInputArity(private)estimate_physical_dag_with_cache; clippydead_codeflags them after it goes.ComparisonScope(with::from_workloadand::validate),ScanSelection,validate_comparison_scopes,evaluations_in_horizon,OperatorStatisticsProviderphysical_operator_statistics.rsdefinitions, the deleted estimator, and tests.from_workload: definition only.ComparisonScope::from_workloadcallers), #734types::workload::resources::CacheProfile,CacheEvidence,CacheCapacityEvidence(resources/cache.rs, 2 tests)analytical_cost.rsand the deleted resolver.cache_profilefield); #686 (devtool field)types::workload::resources::StorageResources(resources/storage.rs, 2 tests)storage_iore-export.types::workload::resources::PhysicalHandoffBytes,PhysicalHandoffKind(resources/physical_handoff.rs, 2 tests;planner_vocabulary.rs::renamed_handoff_apis_are_public)physical_handoff_costre-export, and that test.AnalyticalCostError::{UnsupportedDataArrival, NoEvaluationsInHorizon, InvalidCacheEvidence, UnsupportedQueryOperator, MissingComparisonScope, MissingScanSelection, ScanOutsideComparisonScope, ComparisonScopeMismatch, MissingOperatorStatistics, ConflictingEdgeStatistics, InvalidPhysicalDAG}grep -rnw <variant> crateshits only the enum).SchemaWire,FieldWire,FieldDataTypeWire(thecolumnsand bare-dtype branches); testslegacy_pre_asap_schema_deserializes,legacy_pre_asap_schema_without_closed_or_table_is_open,legacy_summary_schema_deserializes_as_liftedSchema#[serde(default)]onAggIntent'scol(9 variants; redundant, a missingOptionstill reads asNone) andcols(now required); testagg_intent_serde_reads_pre_115_payloadscols, and serialization always writes it.asap_executor::dag(executor/src/dag/mod.rs)git grep 'asap_executor::dag' origin/main -- crates ':!crates/*/tests/**': 0 hits. Test files and a README doctest only.analytical_cost.rstests: 26physical_dag_*/cache_*/ comparison / scope tests,operator_arity_distinguishes_source_statistics_from_dag_children, and thecomparison_scope/cache_test_scan/cache_profilehelpersNot deleted (out of scope; already dead on
mainbefore this PR):AnalyticalCostError::{InvalidIngestionRate, InconsistentBootstrapEvidence, InvalidOperationCost, ParameterMismatch, UnsupportedWithoutDistribution, UnsupportedCandidate, UnsupportedSummaryOperation}are declared but never constructed onmaineither.Key code interfaces added / modified
Removed only. No interface is added or changed. Files deleted outright are linked at the base commit.
crates/plan-selection/src/cost/mod.rs (removed: query_physical_lowering.rs, storage_io.rs, physical_handoff_cost.rs)
crates/plan-selection/src/cost/analytical_cost.rs
crates/plan-selection/src/cost/physical_operator_statistics.rs
crates/types/src/workload/resources.rs
crates/types/src/ir/schema/mod.rs, crates/types/src/ir/operator/agg_intent.rs
The other
AggIntentvariants do not change.crates/executor/src/lib.rs
Usage
Nothing user-facing changes. The live pricing path is the same as before: a workload enters
stage_pipeline, Stage 3 prices each node of each physical candidate withestimate_operator, and cost model 1 turns the result into cost per second. Run at this head:stage3_plan_selectioninu.json:Before this PR
The crate exported a second cost path, from lowering through DAG estimation to the cache, storage and handoff profiles. It compiled and had about 60 tests, but no stage, devtool or downstream repository called it. Its develop docs said it was "kept" and waiting for a consumer.
SchemaandAggIntentalso accepted JSON layouts that no build has written since the schema unification and #115. The executor kept adagalias module that only tests used.After this PR
Stage 3's per-node analytical estimate is the only analytical cost path. Cost-model code is about 5,000 lines smaller. The docs say which parts of the analytical-cost design were deleted. Legacy
columnsschemas andcardinalitymeasures withoutcolsnow fail to deserialize with serde's error instead of loading.Verification
All runs below are local, at this head. GitHub CI was not waited on.
cargo fmt --all -- --check: clean.cargo clippy --workspace --all-targets --all-features --locked -- -D warnings: clean apart from two lints already onmainin the untouchedplan-selection/src/pass/components.rs(redundant_closureat :78,neg_cmp_op_on_partial_ordat :310). The local clippy 0.1.98 reports them; they were allowed for this run.cargo check -p asap-planner --tests --locked: ok.cargo test --workspace --locked: 1769 passed, 0 failed, 36 ignored. After rebasing ontomain(9bf3834; test: drop duplicate and stale tests, keep slow variants out of debug CI, run the DAG viewer tests in CI #834 dropped tests): 1776 passed, 0 failed, 33 ignored; the promql-only run: 51 passed.cargo test -p asap-planner --no-default-features --features promql --locked: 48 passed, 0 failed, 2 ignored.crates/executor/tests/physical_dag.rsandweighted_topk_binding.rs. Whichever PR merges second needs a small rebase.