Repository navigation
refactor!: rename AccuracyTarget.error, Rejection.valid, accuracy CLI flags, execution_timed_dag and accuracy_target - #740
Merged
Conversation
- AccuracyTarget.error is max_error (JSON key "max_error"), matching the AccuracyTarget::max_error builder. AccuracySpec.error, the guarantee's bound expression, keeps its name. - Rejection.valid is replaced by cause: RejectionCause, which says why Stage 3 did not select the candidate (a failed check or a loss under the cost model); stage_pipeline writes it as "cause" and the viewer validates it. - stage_pipeline's --epsilon/--delta/--metric are --max-error, --failure-probability and --accuracy-metric; sketch_coverage's --epsilon is --max-error; the viewer's /api/plan payload keys follow. - PlanOutput::execution_timed_dag is physical_asap_dag, and realization::accuracy_target is sizing_target. - Stage 3's node docs say a node is an operator node of the physical plan DAG, keyed by PhysicalASAPNodeId, not a machine. Regenerate the Example 1 viewer fixture for the new keys.
… 1 candidates Port promql_binary_arithmetic_preserves_both_scalar_operand_orders, deleted with the legacy search: every logical candidate of rate(a[1m]) / 2 and 2 / rate(a[1m]) keeps the literal on its side.
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
Several public names did not say what they hold.
AccuracyTarget.erroris the largest error a query accepts, yet its builder is alreadyAccuracyTarget::max_error(ε).Rejection.validwas a bool that only said whether a candidate failed a check, not why it lost. The devtools flags--epsilon/--delta/--metricdid not match the target's field names.execution_timed_dagandrealization::accuracy_targetwere named after how the value is made, not what it is. On #734 the user also asked whether a "node" inNodePriceSourcemeans a machine. It does not.What
Breaking change. Every rename, old → new:
AccuracyTarget.errorAccuracyTarget.max_errorrequirements.accuracy,AggIntent.accuracy, stage_pipeline document)"error""max_error"Rejection.valid: boolRejection.cause: RejectionCause(new enum)stage3_plan_selection.rejected[])"valid": true/false"cause": "<snake_case RejectionCause>"stage_pipeline)--epsilon--max-errorstage_pipeline)--delta--failure-probabilitystage_pipeline)--metric--accuracy-metricsketch_coverage)--epsilon--max-error/api/planpayload key (server.py,editor.js)"epsilon""max_error"/api/planpayload key"metric""accuracy_metric"index.html)plannerEpsilon,plannerMetricplannerMaxError,plannerAccuracyMetricPlanOutput::execution_timed_dag()PlanOutput::physical_asap_dag()realization::accuracy_target(intent)realization::sizing_target(intent)None of the other devtools binaries (
show_logical_dag,variant_coverage,analyze_corpora,convert_rqe_atomic_costs) has accuracy flags.AccuracySpec.error(the guarantee side,AccuracyGuarantee.spec.error) keeps its name. It holds aBoundExpr: the error bound a plan achieves, an expression over the plan's parameters. It is not a maximum someone asked for, andguarantee.spec.max_errorwould read like a requirement. Its doc now says "the error bound", and theAccuracySpecdoc explains how it differs from the target'smax_error.Docs only, no rename:
NodePriceSource,NodeCost,CandidateCost.per_nodeand the Stage 3 module doc now say that a node is one operator node of the candidate's physical plan DAG (PhysicalASAPDAG), keyed byPhysicalASAPNodeIdinCandidateCost.per_node, not a machine. The broken intra-doc linkNodeCost::per_secondnow points toNodeCost::cost_per_second.Test:
promql_binary_arithmetic_preserves_both_scalar_operand_orderswas deleted with the legacy search in #736. It is ported onto the stage pipeline and now checks every Stage 1 logical candidate.Behaviour is unchanged apart from the names: the same candidates are selected at the same costs.
How
RejectionCauseis built from whatselect_planactually produces. It has five failed-check variants, one per rejection site: resize for failure probability,accuracy_check, raw-data check, latency bound, cluster capacity. The latency and capacity checks used to be merged withor_else; they now carry their own causes. It also has six cost-model variants, one perRanking::lost_reasonarm:Costlier,Slower,MoreMemory,NotLowestLatencyOnParetoFront,Dominated,TiedLostOnEnumerationOrder.lost_reasonnow returns(RejectionCause, String).RejectionCause::failed_a_check()gives back the old!valid. I chose the enum overloses_on_cost_only: boolbecause the cause was already computed at each site and only dropped. Thereasonstrings are unchanged.stage_pipelineserializescausewith serde (snake_case). The viewer (stages.js) checks thatcauseis one of the eleven names. It refuses a document without one.error→max_errorin targets,valid→cause, and the reason strings printmax_error:from the target'sDebug. Nothing else changed.library-api.md,input-output-workflow.md,extend-asap-aware-mapping.md,asap-aware-mapping-contracts.md,run-a-query.md,stage-viewer.md,physical-planning-and-deployment.md,updated_interface_with_pluggable_optimization.mdand the viewerREADME.md.Key code interfaces added / modified
crates/types/src/types.rs(at e73fda264e786c83b2bcccfd594b709f7a2b6c35)crates/asap-aware-mapping/src/stage3_plan_selection.rs(at e73fda264e786c83b2bcccfd594b709f7a2b6c35)crates/asap-aware-mapping/src/stage3_plan_selection/measured_operation_costs.rs(at e73fda264e786c83b2bcccfd594b709f7a2b6c35)crates/asap-aware-mapping/src/pass/mod.rs(at e73fda264e786c83b2bcccfd594b709f7a2b6c35) andrealization.rs(at e73fda264e786c83b2bcccfd594b709f7a2b6c35)CLI and viewer:
crates/devtools/src/bin/stage_pipeline.rs,sketch_coverage.rs,tools/dag-viewer/server.py(stage_pipeline, sketch_coverage, server.py)Usage
End to end, from the devtool invocation to Stage 3's rejections. I ran this at the head commit:
Valid candidates that lose under the cost model carry a loss cause. From the regenerated Example 1 fixture (
--example planner-layering-1, P1 selected at 7.940 per second):{"cause": "costlier", "id": "P4", "reason": "costlier: 8.540 vs 7.940 cost_per_second"} {"cause": "tied_lost_on_enumeration_order", "id": "P2", "reason": "tied with the selected candidate P1 at 7.940 cost_per_second; lost on enumeration order"}Before this PR
{"error": 0.01}, and the Rust field wastarget.error, while the builder wasAccuracyTarget::max_error(0.01).stage3_plan_selection.rejected[]had"valid": true|false. Telling "costlier" from "dominated" or "tied", or "accuracy" from "latency" or "capacity", meant parsingreason.stage_pipeline --epsilon 0.01 --delta 0.001 --metric rank.PlanOutput::execution_timed_dag()andrealization::accuracy_target(intent).After this PR
{"max_error": 0.01}andtarget.max_error. Old JSON with"error"is rejected (deny_unknown_fields)."cause": "costlier"(or one of the other ten names). Rust callers match onRejectionCauseor callfailed_a_check().stage_pipeline --max-error 0.01 --failure-probability 0.001 --accuracy-metric rank. The old flags are refused as unknown.PlanOutput::physical_asap_dag()andrealization::sizing_target(intent).PhysicalASAPDAG, not a machine.Verification
All of these were run locally on the changed code. These are only the tests that cover the change, not the full workspace suite.
cargo fmt --all -- --check: clean.cargo clippy -p asap-types -p asap-aware-mapping -p asap-devtools -p asap-integration-tests -p asap-planner -p asap-frontend-promql -p asap-frontend-sql --all-targets --all-features --locked -- -D warnings: clean.asap-types(types::tests,workload::tests,agg_intent::tests): 31 passed. This includes the JSON round trips withmax_errorand the rejection of"error".asap-aware-mapping(stage3_plan_selection,budget,realization,pass::tests,per_computation_candidates,accuracy::tests): 138 passed. The tests that checkedvalidnow assert the specificRejectionCause.asap-devtools(tests/stage_pipeline.rsand the bins' unit tests): 24 passed. The document test checks that a failed-check cause is exactly an unpriced candidate.asap-integration-tests(everyplanner_layering_*binary, which pins selections and costs, pluspromql_to_post_asap): 99 passed, 20 ignored as before. The portedpromql_binary_arithmetic_preserves_both_scalar_operand_orderspasses.asap-planner --test summary_sharing: 9 passed.asap-frontend-promql(promql_conformanceandaccuracy_target_flows_into_quantile_intent): 106 passed.asap-frontend-sqlmedian_threads_the_accuracy_target: passed.python3 -m unittest test_server test_viewerwith py_mini_racer and a builtstage_pipeline: 51 passed, none skipped.stage_pipeline --example planner-layering-1. The diff is only the renamed keys andmax_errorin the reason strings. The selection (P1 at 7.940 per second) and every cost are unchanged.Limitations: the old JSON key and flags have no compatibility alias, as intended. Saved workload or stage documents with
"error"or"valid"must be written again.