Skip to content

Commit 08528ad

Browse files
milindsrivastava1997milindsrivastava1997
committed
Added Claude implementation plan and a doc with the initial prompt and subsequent discussion on design choices
1 parent 8028dba commit 08528ad

2 files changed

Lines changed: 591 additions & 0 deletions

File tree

‎.docs/CAPABILITY_MATCHING_DESIGN.md‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
# Capability-Based Aggregation Matching — Design Decision Record
2+
3+
## Problem Statement
4+
5+
The query engine previously routed every incoming query to a sketch aggregation by matching the
6+
query string against a pre-configured `query_configs` table in `InferenceConfig`. This meant:
7+
8+
- Every distinct query string needed its own config entry, even when the same sketch could answer
9+
multiple queries (e.g. `quantile(0.5, metric[5m])` and `quantile(0.9, metric[5m])` both need a
10+
KLL sketch, but each required a separate config row).
11+
- The system could not answer any query it had not been explicitly pre-configured for.
12+
13+
The goal: let the engine understand what a query *needs* and find an existing aggregation that can
14+
*provide* it, without requiring a one-to-one mapping in config.
15+
16+
---
17+
18+
## Architecture Investigation
19+
20+
Before designing anything, the existing query routing path was traced:
21+
22+
1. An incoming query (PromQL or SQL) is parsed and reduced to a `QueryExecutionContext`.
23+
2. Inside that process, `find_query_config` (exact string match) or `find_query_config_sql`
24+
(structural AST match) look up a `QueryConfig` in `InferenceConfig.query_configs`.
25+
3. `QueryConfig` is nothing more than a join record: query string → list of `aggregation_id`s.
26+
4. `get_aggregation_id_info` then looks up those IDs in `StreamingConfig` to get the actual
27+
`AggregationConfig` (sketch type, window, labels, etc.).
28+
29+
The key insight: **all capability information lives in `AggregationConfig` inside `StreamingConfig`**.
30+
The `QueryConfig` table is just indirection that requires manual pre-population. The fix is to
31+
skip it and match against `AggregationConfig` directly when no pre-configured entry exists.
32+
33+
---
34+
35+
## Design Questions and Answers
36+
37+
The following questions were worked through before writing a single line of implementation.
38+
39+
### Q1: When multiple aggregations are compatible, which one wins?
40+
**Decision**: Prefer the largest `window_size`. Encapsulated in a separate `aggregation_priority`
41+
comparator function so this policy is swappable later without touching the matching logic.
42+
43+
### Q2: Label compatibility — how strict?
44+
**Decision**: Strict exact match for now. A sketch grouped by `{job, instance}` does **not** serve
45+
a query that groups by `{job}` only, even though collapsing labels is mathematically valid for
46+
simple accumulators (Sum, Min, Max). The reason: for sketch types (KLL, CountMin), label collapsing
47+
is not well-defined. Adding a TODO to relax this to "superset ok" for simple accumulators in a
48+
future iteration.
49+
50+
### Q3: Spatial filter compatibility?
51+
**Decision**: If the stored aggregation has a non-empty `spatial_filter` and the query's normalized
52+
filter differs (or is absent), reject. Never silently serve data filtered to `{env="prod"}` to a
53+
query that expects unfiltered data.
54+
55+
### Q4: Multi-population sketches (SetAggregator / DeltaSetAggregator)?
56+
These require two aggregation IDs: one "value" aggregation and one "key" aggregation. The existing
57+
`get_aggregation_id_info` already distinguishes them by type: `SetAggregator` and
58+
`DeltaSetAggregator` are key aggregations; everything else is a value aggregation.
59+
60+
**Decision**: Capability matching finds the value aggregation first (based on the statistic). If
61+
the matched value type is a "multi-population" type (`MultipleSumAccumulator`,
62+
`MultipleMinMaxAccumulator`, `MultipleIncreaseAccumulator`, `CountMinSketchWithHeap`), the matcher
63+
then separately searches for a key aggregation (`SetAggregator` or `DeltaSetAggregator`) on the
64+
same metric. Both IDs are required; if either is missing, the match fails.
65+
66+
### Q5: Backward compatibility — keep old `query_configs` path?
67+
**Decision**: Yes, as a primary route. The `query_configs` lookup runs first; capability matching
68+
fires only when no pre-configured entry is found. This means existing deployments change behavior
69+
only for queries that had no config entry. A `warn!` log is emitted whenever capability matching
70+
is used, so operators can detect fallback usage.
71+
72+
### Q6: Rich error messages on no-match?
73+
**Decision**: Deferred. Collecting per-candidate rejection reasons adds significant complexity.
74+
The matcher returns `None` on failure for now.
75+
76+
### Q7: How to model `avg` (needs both Sum and Count)?
77+
**Decision**: `QueryRequirements` holds `Vec<Statistic>`. For `avg`, this is `[Sum, Count]`.
78+
All statistics in the vec must be satisfied by aggregations that share the **same** `window_size`
79+
and `grouping_labels`. This ensures temporal consistency.
80+
81+
### Q8: How is window type (sliding vs tumbling) expressed in requirements?
82+
Framing the requirement as a specific `window_type` was considered but rejected. Instead,
83+
`QueryRequirements` stores only `data_range_ms: Option<u64>` — the span of historical data the
84+
query reads. Both tumbling and sliding aggregations can satisfy this, subject to different
85+
compatibility rules:
86+
87+
- **Tumbling**: `data_range_ms` must be a positive integer multiple of `window_size_ms` (so
88+
multiple buckets can be merged to cover the range).
89+
- **Sliding**: `data_range_ms` must equal `window_size_ms` exactly (a sliding window precomputes
90+
exactly one range per timestamp; you can't merge overlapping windows).
91+
- **Spatial-only** (`data_range_ms = None`): any window is compatible.
92+
93+
### Q9: Where does the capability matching logic live?
94+
**Decision**: `sketch_db_common` (the shared crate). Rationale: this logic is pure — it takes a
95+
map of `AggregationConfig` values and a `QueryRequirements` and produces an `AggregationIdInfo`.
96+
It has no dependency on query engine internals. Putting it in common means the planner and other
97+
components can eventually reuse it.
98+
99+
`AggregationIdInfo` (previously defined in `simple_engine.rs`) was moved to `sketch_db_common` as
100+
a prerequisite, since the common function needs to return it.
101+
102+
`StreamingConfig` (in `asap-query-engine`) gets a thin wrapper method that delegates to the
103+
common function, so call sites inside the engine don't need to reach into common directly.
104+
105+
### Q10: `aggregation_sub_type` — does it matter for matching?
106+
**Decision**: Yes. `Min` requires `aggregation_sub_type == "min"`, `Max` requires `"max"`. The
107+
`required_sub_type(statistic)` helper encodes this. Other statistics have no sub-type constraint.
108+
109+
### Q11: For `Vec<Statistic>` — must all statistics agree on window and labels?
110+
**Decision**: Yes. For `avg = [Sum, Count]`, the matched Sum aggregation and the matched Count
111+
aggregation must have the same `window_size` and `grouping_labels`. This is the simpler, safer
112+
choice — mixing aggregations with different windows or label granularities would produce
113+
semantically incorrect results.
114+
115+
---
116+
117+
## What Was Rejected
118+
119+
### "Translate PromQL to SQL and execute via DataFusion SQL engine"
120+
Considered as a broader architectural direction. Rejected for this feature because:
121+
- Data is stored as binary sketches (KLL, CountMin, etc.), not raw values. SQL aggregation
122+
functions cannot merge sketches natively.
123+
- Every sketch operation would need a DataFusion UDF, recreating the existing operator logic
124+
with more indirection.
125+
- The existing `execute_plan()` path already uses DataFusion as an execution *framework* with
126+
custom physical operators — that is the right abstraction boundary, not SQL strings.
127+
128+
### Merging PromQL and SQL `build_query_execution_context` paths
129+
The two build paths (PromQL and SQL) were kept separate. They parse different syntaxes into
130+
different AST types. Merging them would require a common intermediate representation before the
131+
current `QueryExecutionContext` and would not reduce complexity. The shared logic is the capability
132+
matching layer, not the parsing layer.
133+
134+
### Rich rejection errors
135+
Collecting per-candidate rejection reasons (e.g. "found KLL for metric X but window 15 m doesn't
136+
match 5 m query") was considered. Deferred: the matching logic touches every candidate and
137+
collecting structured reasons multiplies the implementation surface significantly. Simple `None`
138+
return with `debug!` logging is sufficient for now.
139+
140+
---
141+
142+
## Final Architecture
143+
144+
```
145+
Incoming query (PromQL or SQL)
146+
│
147+
▼
148+
Parse query AST
149+
│
150+
▼
151+
Try find_query_config / find_query_config_sql ← existing path (unchanged)
152+
│
153+
├── found ──► get_aggregation_id_info(config) ──► AggregationIdInfo
154+
│
155+
└── not found ──► warn!("falling back to capability matching")
156+
│
157+
▼
158+
build_query_requirements_{promql|sql}
159+
→ QueryRequirements {
160+
metric, statistics: Vec<Statistic>,
161+
data_range_ms, grouping_labels,
162+
spatial_filter_normalized
163+
}
164+
│
165+
▼
166+
StreamingConfig::find_compatible_aggregation(&requirements)
167+
→ sketch_db_common::find_compatible_aggregation(
168+
&self.aggregation_configs, requirements
169+
)
170+
→ Option<AggregationIdInfo>
171+
```
172+
173+
The `find_compatible_aggregation` function in `sketch_db_common`:
174+
1. For each statistic, collects candidates from `StreamingConfig` passing all filters
175+
(metric, type, sub-type, window, labels, spatial filter).
176+
2. Sorts candidates by `aggregation_priority` (largest window first).
177+
3. For `Vec<Statistic>`, ensures all statistics are satisfied by configs agreeing on
178+
window and labels.
179+
4. If the value aggregation type is multi-population, also finds the paired key aggregation.
180+
5. Returns `AggregationIdInfo` or `None`.

0 commit comments

Comments
 (0)