Repository navigation
sql.rs: query-requirements derivation duplicated across 3 independent implementations #519
Description
SQL's query-requirements derivation (statistic mapping, grouping labels, data range) exists as three separate, non-shared implementations that happen to agree today but have no mechanism keeping them in sync:
asap-query-engine's main path (build_spatiotemporal_context,asap-query-engine/src/engines/simple_engine/sql.rs) derivesquery_output_labels,statistic_to_compute, and timestamps directly fromSQLQueryData/SQLQuery, without going throughQueryRequirementsat all.asap-query-engine's capability-matching fallback path callsbuild_query_requirements_sql, a privateSimpleEnginemethod used only there.asap-planner-rs'sSQLSingleQueryProcessor::get_streaming_aggregation_configs(asap-planner-rs/src/planner/sql.rs) has its own from-scratchget_sql_treatment_type/get_sql_statistics, sharing no code with either function above.
Concretely: parse_single_statistic (query-engine, via AggregationOperator::to_statistics()) and get_sql_statistics (planner-rs, a hand-written match name.to_uppercase()) encode the same name→Vec<Statistic> mapping as two separately-maintained match statements. get_sql_treatment_type (planner-rs: "MIN"|"MAX" => Exact, _ => Approximate) happens to agree with what Statistic::is_approximate() would say for every statistic SQL can produce, but is a separate hand-written check, not a call to that method.
This is the same shape of problem #508 fixed for PromQL, which now has one canonical build_query_requirements_promql shared across all 4 of its own call sites (query-engine's main + fallback paths, planner's SingleQueryProcessor + AQE extractor). For SQL it's three-way, and unlike PromQL there's no cross-crate-shared function to converge onto yet — build_query_requirements_sql is private to asap-query-engine, not exported via asap_types the way PromQL's equivalent is.
Activity
milindsrivastava1997 commentedon Jul 10, 2026
Can PromQL and SQL be unified here?
Partially — the underlying business logic can be shared, but the parsing/detection layer genuinely can't be, for a few structural reasons:
topk(k, expr)) withksitting right there in the aggregation token. SQL's topk is inferred —detect_sql_topkreverse-engineers it fromORDER BY <agg> DESC LIMIT kshape-matching, a heuristic reconstruction, not a direct read. The extraction step can't be shared; the source ASTs don't correspond.sum(min_over_time(x))) is about nested aggregation-of-function. SQL sidesteps this entirely by rejecting nested queries outright ("Nested SQL queries (n=%d) are not supported"). A shared "is this combination valid" check would carry dead weight on the SQL side.spatial_filteris PromQL-only (label matchers baked into{job="api"}selector syntax) — SQL folds the equivalent into GROUP BY/WHERE differently, with no analogous string to normalize.What should be shared but currently isn't: the pure logic once you already have "a statistic name, a set of labels, a duration, a metric" in hand — the name→
Vec<Statistic>mapping (avg→[Sum,Count] expansion),Statistic::is_approximate()-based treatment typing, and label-routing arithmetic (all_labels.difference(&grouping_labels)). None of that cares which language produced the inputs.Recommendation: a small trait (something like
MatchedQuery { fn metric(&self) -> String; fn statistic_name(&self) -> String; fn grouping_labels(&self) -> Vec<String>; fn data_range_ms(&self, interval_ms: u64) -> u64; fn topk_info(&self) -> Option<TopkInfo>; }) implemented once forPromQLMatchResultand once forSQLQuery, with one genericbuild_query_requirements<T: MatchedQuery>doing the actual derivation. That gives SQL the same "one canonical implementation" guarantee #508 gave PromQL, without forcing SQL's fundamentally different AST shape through PromQL's token-bag abstraction (or vice versa).