Repository navigation
Description
Tracking issue for de-duplication opportunities found across the PromQL, SQL, and Elastic DSL code paths in asap-planner-rs and asap-query-engine. Items are grouped by crate, ranked by impact.
asap-planner-rs
-
[HIGH] Controller boilerplate —
promql/controller.rs,sql/controller.rs,elastic_dsl/controller.rsshare an identicalnew/from_yaml/from_file/generate/generate_to_dirshape.generate_to_dir(create dir, serialize streaming + inference YAML, write both files) is byte-for-byte identical in all three. Extract into a shared free fn or trait default method. -
[HIGH] Generator orchestration boilerplate —
promql/generator.rs,sql/generator.rs,elastic_dsl/generator.rseach re-implement, near-verbatim, in theirgenerate_*_planfunctions:- cleanup-policy resolution (
config.aggregate_cleanup.as_ref().and_then(|c| c.policy).unwrap_or(CleanupPolicy::ReadBased)) - duplicate-query validation (
HashSet+ nested loop overqg.queries) - sequential ID assignment (
dedup_map.keys().enumerate()loop) cleanup_mapYAML stanza construction in eachbuild_*_inference_yaml
- cleanup-policy resolution (
-
[MEDIUM]
cleanup_paramwrapper shell —planner/promql.rs,planner/sql.rs,planner/elastic_dsl.rseach wrap their cleanup-param computation in the identicalif cleanup_policy == NoCleanup { None } else { Some(get_*_cleanup_param(...).map_err(...)?) }. Only the inner call differs.
asap-query-engine
- [HIGH] Duplicated "build
QueryExecutionContext" tail —engines/simple_engine/sql.rs:658-679,engines/simple_engine/promql.rs:317-339, and inline inengines/simple_engine/elastic.rs:71-92all execute the same 6-statement sequence: clone streaming config, resolvegrouping_labels/aggregated_labelswith identical fallback semantics, constructQueryExecutionContext. SQL and PromQL already have private helpers for this; Elastic doesn't even have it functionized yet. - [HIGH] Duplicated "resolve aggregation: query_config first, else build requirements + capability match" skeleton —
sql.rs:585-602,sql.rs:735-758, andpromql.rs:1029-1059share the same try/fallback control shape. Note: Elastic (elastic.rs:45) deliberately lacks the fallback — a real semantic gap worth its own look, not blind unification. - [MEDIUM]
now_secs_f64()-shaped boilerplate —drivers/query/adapters/prometheus_http.rs:75-78,clickhouse_http.rs:39-42,elastic_http.rs:48-51all copy-paste the sameSystemTime::now().duration_since(UNIX_EPOCH)...as_secs_f64()snippet to default a missing time parameter. - [MEDIUM] Runtime-info merge logic —
drivers/query/adapters/prometheus_http.rs:255-301(handle_runtime_info) andelastic_http.rs:245-286(handle_runtime_info_with_headers) duplicate the "fetch earliest-timestamp map, merge into fallback runtime data" logic. ClickHouse's version is just a stub, so this is 2-of-3, not 3-of-3.
Activity
milindsrivastava1997 commentedon Jul 13, 2026
QueryRequirementsisn't a shared vocabulary yetThe config-hit path and the capability-matching fallback path each define "what a query needs" independently, so they can silently disagree. SQL's config-hit path (
build_spatiotemporal_context) derives labels/statistic by hand instead of viaQueryRequirements(#519). PromQL derivesQueryRequirementsunconditionally but only uses it for metadata — never to check that the config-path's resolvedagg_infoactually agrees with what capability-matching would pick.This is the same root cause as
capability_matching.rs's rules being out of sync with what the planner emits (.design_docs/planner-engine-inconsistencies.md), and overlaps #519, #501, #267.