Skip to content

Tracking issue for refactoring/de-duplicating logic across PromQL/SQL/Elastic #489

Description

@milindsrivastava1997

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 boilerplatepromql/controller.rs, sql/controller.rs, elastic_dsl/controller.rs share an identical new / from_yaml / from_file / generate / generate_to_dir shape. 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 boilerplatepromql/generator.rs, sql/generator.rs, elastic_dsl/generator.rs each re-implement, near-verbatim, in their generate_*_plan functions:

    • cleanup-policy resolution (config.aggregate_cleanup.as_ref().and_then(|c| c.policy).unwrap_or(CleanupPolicy::ReadBased))
    • duplicate-query validation (HashSet + nested loop over qg.queries)
    • sequential ID assignment (dedup_map.keys().enumerate() loop)
    • cleanup_map YAML stanza construction in each build_*_inference_yaml
  • [MEDIUM] cleanup_param wrapper shellplanner/promql.rs, planner/sql.rs, planner/elastic_dsl.rs each wrap their cleanup-param computation in the identical if cleanup_policy == NoCleanup { None } else { Some(get_*_cleanup_param(...).map_err(...)?) }. Only the inner call differs.

asap-query-engine

  • [HIGH] Duplicated "build QueryExecutionContext" tailengines/simple_engine/sql.rs:658-679, engines/simple_engine/promql.rs:317-339, and inline in engines/simple_engine/elastic.rs:71-92 all execute the same 6-statement sequence: clone streaming config, resolve grouping_labels/aggregated_labels with identical fallback semantics, construct QueryExecutionContext. 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" skeletonsql.rs:585-602, sql.rs:735-758, and promql.rs:1029-1059 share 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 boilerplatedrivers/query/adapters/prometheus_http.rs:75-78, clickhouse_http.rs:39-42, elastic_http.rs:48-51 all copy-paste the same SystemTime::now().duration_since(UNIX_EPOCH)...as_secs_f64() snippet to default a missing time parameter.
  • [MEDIUM] Runtime-info merge logicdrivers/query/adapters/prometheus_http.rs:255-301 (handle_runtime_info) and elastic_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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions