Repository navigation
refactor!: AccuracyTarget builders, AccuracyMetric naming, simpler Stage 3 interfaces and unread workload fields removed - #722
Merged
Conversation
- AccuracyTarget is built with exact() or max_error(ε) and the optional builders failure_probability(δ), accuracy_metric(m) and accuracy_scope(s); epsilon, epsilon_delta, with_metric and with_scope are removed. - ErrorMetric is renamed AccuracyMetric, and the metric and scope fields of AccuracyTarget and AccuracySpec are renamed accuracy_metric and accuracy_scope (JSON keys too). - target_in and for_quantile leave the public type. Frontends keep the query's target on each intent as written; asap-aware-mapping's accuracy_target restates it in the intent's own metric for sizing.
…erfaces - equal_shares and equal_share become one per_input_target(target, metric, approximate_inputs); the legacy core keeps its nested-layer split as a private helper. - whole_result_guarantee(g, output_values: Option<f64>) becomes AccuracyGuarantee::over_whole_result(values_per_result: Option<u64>), and OUTPUT_VALUES becomes VALUES_PER_RESULT. - A candidate whose summaries fail to rebuild after resizing for the failure probability is rejected with the reason instead of failing select_plan; SizingError and SelectionError::Sizing are removed. - Selection.accepted holds one AcceptedAccuracy per query (no Option). - Renames: sized_roots -> resize_for_failure_probability, summary_source -> distinct_bound_source, hll::precision_for -> hll::min_precision. - Tests: the resize rejection (failing first) and the EntryCount error.
- TimeSelection, QueryTimeScope and the time_selection field of QueryWorkloadEntry, BatchEntry and RepeatingEntry: the window a query reads is in its text. ComparisonScope loses the copy it compared. - DataWorkload.ingestion_volume and DataWorkload.distribution, the DataDistribution type, and PropagationStats.data_distribution, which only carried it to no reader.
strictest_sibling_accuracy compared a sibling's written target with a restated one, so identical count and distinct-count siblings never matched and the strictest-sibling candidate was never offered. Compare both intents with the same target. Also fix a vacuous viewer assertion and the stale lookback/as_of field claim in planner-layering.md.
zzylol
force-pushed
the
refactor/accuracy-target-api
branch
from
October 15, 2026 22:59
1177e49 to
2af382c
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
Review of #690, #712 and #717 found several interfaces that are harder to read than they need to be, and several fields that no stage reads:
AccuracyTarget::epsilon_delta(ε, δ).with_metric(m)names its builders after Greek letters and awith_prefix that the fields do not have.target_inandfor_quantileare planner sizing helpers, but they sit on the public type, and every frontend calls them.ErrorMetricand themetric/scopefields of a target do not say that they are about accuracy.equal_shares,equal_share).whole_result_guaranteetakes a fractional count of values. When resizing one candidate for its failure probability fails, the wholeselect_plancall fails.TimeSelection(withQueryTimeScope) is set on every workload entry, but no stage reads it: the window comes from the query text.DataWorkload.ingestion_volume,DataWorkload.distributionandPropagationStats.data_distributionare also never read.What
AccuracyTargetis built withexact()ormax_error(ε), plus the optional builders.failure_probability(δ),.accuracy_metric(m)and.accuracy_scope(s).epsilon,epsilon_delta,with_metric,with_scope,target_inandfor_quantileare removed.ErrorMetricis renamedAccuracyMetriceverywhere. OnAccuracyTargetandAccuracySpec, the fieldsmetricandscopebecomeaccuracy_metricandaccuracy_scope; the JSON keys change with them.per_input_targetreplacesequal_sharesandequal_share. The methodAccuracyGuarantee::over_whole_result(values_per_result: Option<u64>)replaceswhole_result_guarantee.SizingErrorandSelectionError::Sizingare gone.Selection.acceptedholds oneAcceptedAccuracyper query, with noOption.sized_roots→resize_for_failure_probabilitysummary_source→distinct_bound_sourcehll::precision_for→hll::min_precisionTimeSelection,QueryTimeScope, and thetime_selectionfield ofQueryWorkloadEntry,BatchEntry,RepeatingEntryandComparisonScopeDataWorkload.ingestion_volume,DataWorkload.distributionand theDataDistributiontypePropagationStats.data_distributionSelection.accepteddoc, thesummary_sharing.rsmodule doc, "ImplicitExact", and "null without a target" instages.js.Out of scope: the Pass 2
*Candidaterenames, theFamilyCapabilitychanges and the cost model.How
Targets.
realization::accuracy_target(intent)now returns an owned target. It restates the intent's raw target in the intent's own metric through a privatetarget_inhelper:CountandTopK;CardinalityandFrequencyL2;FrequencyEntropy;Quantile, the set metric, orRankwhen none is set.This is what the frontends used to stamp onto each intent. Every Stage 1 sizing and per-summary check already went through this accessor. Stage 3 still checks the query's own target at the root.
An equality check that compares intents must restate both sides the same way. The legacy
strictest_sibling_accuracycompared a restated target with a written one, and is fixed to compare both intents under the same target.Budget split.
per_input_targetis the one-share case of the oldequal_shares. Only the legacy core usedshares > 1, for a nested sub-DAG that holdsn − 1ofnlayers, and that case cannot be written as a per-input split. It stays there as the privatenested_layers_target.Resize rejection.
sized_for_failure_probabilitynow returns the rejection reason as aString.select_planrecords it as aRejection { valid: false, .. }and goes on to the next candidate.Deleted fields. For each deleted field, the only readers were tests, the
ComparisonScopeequality check, and the evidence copy intoPropagationStats. The integration test that readtime_selection.lookbacknow takes each window from thePATTERN_A/PATTERN_B_WINDOW_MSconstants, which restate the query text. Theempirical_costdistributionis a different type (DistributionDescriptor) and stays.Key code interfaces added / modified
crates/types/src/types.rscrates/types/src/post_asap/guarantee.rscrates/asap-aware-mapping/src/accuracy/budget.rscrates/asap-aware-mapping/src/realization.rscrates/asap-aware-mapping/src/stage3_plan_selection.rscrates/asap-aware-mapping/src/accuracy/estimators/hll.rs,crates/asap-aware-mapping/src/per_computation_candidates.rscrates/types/src/workload.rscrates/asap-aware-mapping/src/accuracy/evidence.rscrates/asap-aware-mapping/src/physical_operator_statistics.rsThe JSON and viewer formats change in the same way:
AccuracyTargetandAccuracySpecuse the keys"accuracy_metric"and"accuracy_scope"(previously"metric"and"scope")."time_selection".DataWorkloadno longer takes"ingestion_volume"or"distribution".stage_pipelineoutput,stage3_plan_selection.accepted[id]is nevernull.tools/dag-viewer/stages.jsreadsaccuracy_metric.Usage, end to end. One PromQL p99 panel with a rank-error target over its whole result, planned with
run_planning_stagesat this head. The numbers come from running this code as a test.Before this PR
AccuracyTarget::epsilon_delta(0.01, 0.01).with_metric(ErrorMetric::Rank), and the JSON was{"metric": "rank", ...}.target_inorfor_quantileon every intent.select_planreturnErr(SelectionError::Sizing), which hid every other candidate.time_selectionthat no stage read.After this PR
AccuracyTarget::max_error(0.01).failure_probability(0.01).accuracy_metric(AccuracyMetric::Rank), and the JSON is{"accuracy_metric": "rank", ...}.Verification
cargo nextest run --workspaceafter rebasing ontoc127c62c: 1855 passed, 35 skipped.cargo test --doc -p asap-types -p asap-aware-mapping: passed.cargo fmt --all -- --checkandcargo clippy --workspace --all-targets --all-features --locked -- -D warnings: clean.tools/verify_metricsql_external_consumer.sh: passed.python3 -m unittest test_viewer test_serverintools/dag-viewer: 50 tests OK. The unset-metric assertion intest_server.pynow checksaccuracy_metric, so it is no longer vacuous.a_candidate_that_fails_to_resize_is_rejected_alone. Written first, it failed on the previous commit withSizing { candidate: "P2", error: Export(UntimedNode { operator: "Project" }) }and passes now.an_entry_count_mismatch_is_an_error.identical_count_and_distinct_siblings_share_the_strictest_target. This is a regression found in review: twocount by (job)siblings (and two distinct-count siblings) at ε 0.05 and 0.01, with no metric set, must size the looser one for 0.01. Written first, it failed on the reviewed head (Noneinstead ofSome(max_error(0.01))) and passes after the fix. I found no other equality check that mixes a written target with a restated one:shared_quantile_targetcompares written targets on both sides, and reconciliation compares restated targets on both sides.a_nested_sub_dag_gets_its_layers_share_of_the_target, which keeps the coverage of the 2-of-3 split thatequal_shareshad.tools/dag-viewer/examples/planner-layering-example1.jsonwas regenerated. Its diff is only the key renames, plus intents that no longer carry the frontend-stamped"accuracy_metric": "absolute_stream_l1".Limitations and follow-ups
FrequencyL2intents, while SQL and the root-metric table used a relative one. The single restating helper now uses relative for both. No test depended on the absolute version.planner-layering.mdstill useslookbackandas_ofas columns in its example tables, and it still lists "volume" and "distribution" among the data-workload facts in its overview table and diagram. I only removedtime_selectionfrom the stage table, said thatlookbackandas_ofare stated by the query text (its range and offset) rather thanworkload.rsfields, replacedas_of: Nonewith "no offset", and dropped theingestion_volumeanddistributionrows from the shared-data-workload table. A fuller rewrite belongs with the docs work in Update the docs after the G1–G11 consolidation groups land #677.end-to-end-accuracy-guarantees.mdshows aResultGuarantee { metric: AccuracyMetric, .. }. Only the type name was renamed there.AccuracyTarget.errorwould read better asmax_error, to match its constructor.Rejection.validreads as a property of the rejection;passed_checkswould say what it means.realization::accuracy_targetreturns the restated target, sosizing_targetwould say that.stage_pipelineand viewer request flags--epsilon/--metric(and the"metric"key of the viewer's/api/planrequest) still use the old words.PATTERN_Atuple(query, window, offset)has unnamed fields.PropagationStats.topk_*are set only by tests and by deployment evidence providers, but a rule reads them, so they stay.SourceFacts) is left for a later PR.