Repository navigation
[Refactor] Remove legacy public types from asap_executor (Statistic, KeyByLabelValues, Measurement) and the uncalled query_time module #967
Description
Example
The executor reads an exact rate from merged panes with its own statistic enum and key type (crates/executor/src/evaluation.rs:42):
pub fn exact_evaluation(
states: impl IntoIterator<Item = Arc<dyn AggregateCore>>,
statistic: Statistic, // asap_executor::Statistic
range_ms: Option<(i64, i64)>,
key: Option<&KeyByLabelValues>, // asap_executor::KeyByLabelValues
) -> Result<Option<f64>, String>The plan it executes names the same readout with IR types: the exact family is ExactKind::Increase/Sum/Count/Min/Max/... (types/src/ir/schema/sketch.rs:18), and a sketch readout is a SketchStatistic / StatisticKind (types/src/ir/schema/family_capability.rs:36). Lowering then translates IR into the executor's enum by hand, for example crate::Statistic::Sum and crate::Statistic::Count in physical_planner/precompute.rs:374-379.
What should happen: the executor's public API uses the IR's types for statistics, plain values for measurements, and keeps its internal item-key encoding private.
Concrete maintenance problem
These public items came over from the earlier backend and are not part of the operator library or the compiled plan:
| Item | What it is | Problem |
|---|---|---|
Statistic (statistic.rs) |
Count, Sum, Cardinality, FrequencyL2, FrequencyEntropy, Increase, Rate, Min, Max, Quantile, Topk, plus from_str and Display |
A third statistic vocabulary next to ExactKind and StatisticKind. Lowering maps between them by hand, so adding a readout means editing three enums. Outside the executor, only executor tests use it. |
KeyByLabelValues (key_by_label_values.rs) |
Vec<String> joined with ; to hash a sketch item (CMS, Count Sketch, HydraKLL, KMV, exact keyed states) |
It is the kernels' internal item encoding, but it is public and appears in AccumulatorUpdater::update_keyed and exact_evaluation. Outside the executor, only tests use it (executor/tests/deployment.rs, integration-tests/tests/precompute_raw_samples.rs). |
Measurement (measurement.rs) |
struct Measurement { value: f64 } with Add |
It wraps one f64 (used in summary_kernels/{exact,increase}.rs), and shares its name with the unrelated types/src/workload/resources/measurement.rs. |
query_time/ (error_estimation.rs) |
posterior error-bound functions for CMS, CU sketch and Count Sketch (#239) | Its own module docs say nothing in the planner or executor calls it. |
Behavior that must remain unchanged
Every query returns the same answers and the same errors. Sketch item hashing (the ;-joined key) stays the same, so existing sketch states keep their meaning.
Before and after structure
Statisticis removed. Exact evaluation takes the IR'sExactKind(plus the range forRate/Increase), and sketch evaluation takesSketchStatistic. The hand mapping inprecompute.rsgoes away.KeyByLabelValuesbecomes crate-private (for examplepub(crate) struct ItemKey), or the kernels take&[String]. It leavesAccumulatorUpdater::update_keyedandexact_evaluation's public signatures.Measurementis replaced byf64.query_time/is deleted. Add posterior (query-time) error estimation for CMS/CountSketch, per Chen et al. IMC'21 #239 and Feed observed posterior sketch error back into re-planning (execution-time → planning-time loop) #250 keep the formulas and the idea. If a sketch evaluation path later needs them, it adds them where that path lives.
Why the proposed structure is minimally complex
Nothing is added. One enum and two wrapper types leave the public API, and one statistic vocabulary (the IR's) remains.
Existing unit and end-to-end protection
crates/executor/tests/{exact_state_columns,physical_semantics,deployment,deployment_computation,hydra_cms_execution,univmon_execution,state_column_updater}.rs.crates/integration-tests/tests/{precompute_raw_samples,materialization_e2e,kll_pane_execution}.rs.- The
insufficient_counter_raw_data_samplestests inevaluation.rs(counters with fewer than two samples are absent).
Compatibility or migration impact
This is a public API change in asap_executor: Statistic, KeyByLabelValues, Measurement and query_time are removed from the public API. ASAPQuery-backend main pins an older Planner revision (f46cbf6c) and does not depend on asap_executor, so no backend code breaks.
Parent: #968 (PR 3 of 6; independent of the others). Related: #239, #250.
Activity