Skip to content

Commit bac6b16

Browse files
zzylolclaude
andcommitted
fix: select only compilable populations; tighten readout decoding
Review follow-ups: a population is selected only if Planner compiles its readout, so a failed compile runs the query exactly instead of failing the deployment. Grouped population outputs must carry exactly the grouping labels. An Int64 count is the sample only when it is the sole numeric column. The summation difference from Prometheus is documented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent 78a2875 commit bac6b16

5 files changed

Lines changed: 109 additions & 18 deletions

File tree

‎control_plane/src/physical/workload_cost.rs‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,12 @@ fn enumerate_frontier_candidates(
848848
.map(|root| {
849849
root.as_ref()
850850
.and_then(|root| strategy.candidate(root))
851-
.filter(|node| super::maintained_population::supported_node(node))
851+
.filter(|node| {
852+
// Every readout must compile to the Planner program installed with it.
853+
super::maintained_population::supported_node(node)
854+
&& asap_physical_operators::physical_planner::promql_rows::compile_current_series_readout(node)
855+
.is_ok()
856+
})
852857
})
853858
.collect();
854859
if maintained_roots.iter().any(Option::is_some) {
@@ -1044,6 +1049,37 @@ mod tests {
10441049
})));
10451050
}
10461051

1052+
/// Planner cannot yet read out `without` groups, so such a population is
1053+
/// never selected, while the `by` form is.
1054+
#[test]
1055+
fn without_grouping_selects_no_population() {
1056+
for (query, supported) in [
1057+
("quantile without (pod) (0.5, m)", false),
1058+
("quantile by (job) (0.5, m)", true),
1059+
] {
1060+
let root = std::rc::Rc::new(
1061+
asap_physical_operators::physical_planner::promql_rows::with_series_identity(
1062+
&crate::query_parser::parse_query_expr_canonical(
1063+
query,
1064+
crate::types::AccuracyTarget::Exact,
1065+
)
1066+
.unwrap(),
1067+
)
1068+
.unwrap(),
1069+
);
1070+
let strategy =
1071+
asap_aware_mapping::maintained_population::MaintainedPopulationStrategy::new(
1072+
std::slice::from_ref(&root),
1073+
);
1074+
let candidate = strategy.candidate(&root).unwrap();
1075+
assert_eq!(
1076+
super::super::maintained_population::supported_node(&candidate),
1077+
supported,
1078+
"{query}"
1079+
);
1080+
}
1081+
}
1082+
10471083
/// Instant counts select current membership, never accumulated observations.
10481084
#[test]
10491085
fn local_grouped_count_has_a_bindable_candidate() {

‎crates/asap_types/src/query_plan/native.rs‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,19 @@ impl QueryPlanEntry {
5454
SummaryFamilyType::Plain(DataType::Float64 | DataType::Int64)
5555
)
5656
};
57+
let labels = output
58+
.schema
59+
.fields
60+
.iter()
61+
.filter(|field| !numeric(field))
62+
.map(|field| {
63+
(field.dtype == SummaryFamilyType::Plain(DataType::Utf8)).then_some(&field.name)
64+
})
65+
.collect::<Option<BTreeSet<_>>>();
5766
let grouped_value = output.schema.time_index.is_none()
5867
&& output.schema.fields.iter().filter(|f| numeric(f)).count() == 1
59-
&& output.schema.fields.iter().all(|field| {
60-
numeric(field)
61-
|| (field.dtype == SummaryFamilyType::Plain(DataType::Utf8)
62-
&& population.grouping.labels.contains(&field.name))
68+
&& labels.is_some_and(|labels| {
69+
labels == population.grouping.labels.iter().collect::<BTreeSet<_>>()
6370
});
6471
if input.schema != output.schema && !grouped_value {
6572
return Err(invalid(

‎data_plane/src/query_engines/asap_query_engine/logical_dag/native_values.rs‎

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,43 @@ mod tests {
508508
);
509509
}
510510

511+
// A population count is an Int64 sample only when it is the sole numeric
512+
// column; a Float64 value always wins over an Int64 helper column.
513+
#[test]
514+
fn selected_program_reads_counts_and_prefers_float_values() {
515+
for (fields, row, expected) in [
516+
(
517+
vec![("job", DataType::Utf8), ("count", DataType::Int64)],
518+
vec![Value::Utf8("api".into()), Value::Int64(3)],
519+
3.,
520+
),
521+
(
522+
vec![("members", DataType::Int64), ("value", DataType::Float64)],
523+
vec![Value::Int64(7), Value::Float64(2.5)],
524+
2.5,
525+
),
526+
] {
527+
let input = schema(&fields);
528+
let plan = CompiledPhysicalDag::from_operators(
529+
[(0, InputContract::bounded(input.clone()))].into(),
530+
Default::default(),
531+
vec![0],
532+
)
533+
.unwrap();
534+
let (result, _) = execute_batches(&plan, 1 << 20, 1, 42, false, |_, contract| {
535+
Ok(BoundInput::Rows(Batch::try_new(
536+
contract.schema.clone(),
537+
vec![row.clone()],
538+
)?))
539+
})
540+
.unwrap();
541+
let crate::query_engines::query_result::QueryResult::Vector(result) = result else {
542+
panic!("vector expected")
543+
};
544+
assert_eq!(result.values[0].value, expected);
545+
}
546+
}
547+
511548
// Count-like values are bound as integers only when the protocol sample is exact.
512549
#[test]
513550
fn integer_input_binding_preserves_type_and_rejects_rounding() {
@@ -867,17 +904,25 @@ fn execute_batches(
867904
.fields
868905
.iter()
869906
.position(|field| field.name == SERIES_IDENTITY_COLUMN);
870-
let value = batch
871-
.schema()
872-
.fields
873-
.iter()
874-
.position(|field| {
875-
matches!(
876-
field.dtype,
877-
SummaryFamilyType::Plain(DataType::Float64 | DataType::Int64)
878-
)
879-
})
880-
.ok_or_else(|| miss("physical output loses sample value"))?;
907+
let fields = &batch.schema().fields;
908+
let typed = |dtype: DataType| {
909+
fields
910+
.iter()
911+
.enumerate()
912+
.filter(|(_, field)| field.dtype == SummaryFamilyType::Plain(dtype.clone()))
913+
.map(|(i, _)| i)
914+
.collect::<Vec<_>>()
915+
};
916+
// The sample is the Float64 column; an Int64 count is the sample
917+
// only when it is the sole numeric column.
918+
let value = match (
919+
typed(DataType::Float64).as_slice(),
920+
typed(DataType::Int64).as_slice(),
921+
) {
922+
([value, ..], _) => *value,
923+
([], [count]) => *count,
924+
_ => return Err(miss("physical output loses sample value")),
925+
};
881926
for row in batch.rows() {
882927
let sample = &match row[value] {
883928
Value::Float64(sample) => sample,

‎data_plane/src/storage_engines/sketch_db/current_series.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,7 @@ impl Population {
156156
.map(|(k, v)| (k.clone(), v.clone()))
157157
.collect();
158158
self.remove(&labels);
159-
// Bound the retained keys, tree nodes, and shared readout caches together.
159+
// Bound the retained keys and tree nodes together.
160160
let bytes = 1024
161161
+ labels
162162
.iter()

‎docs/developer_docs/query-engine/current-series-aggregations.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# Shared current-series quantiles and TopK
1+
# Shared current-series aggregations
22

33
Backend-local PromQL workload compilation can export a maintained current-value
44
alternative for `quantile(q, metric)`, `topk(k, metric)`, `sum`, `count` and
@@ -50,6 +50,9 @@ A native exact alternative remains available for cost selection and execution fa
5050
unobserved generation or exceeded resource bounds cause native fallback.
5151
- State is in memory. Restart and generation replacement require warmup again;
5252
historical range queries continue to use native execution.
53+
- Sum and average use Planner's aggregate operator, which adds values in order
54+
without Prometheus' compensated summation; results can differ in the last
55+
bits, and an average whose sum overflows is infinite.
5356
- Populations divide the configured retained-summary memory budget and cap series
5457
cardinality. Bounds include conservative space for labels and trees.
5558
The existing Remote Write adapter accepts finite sample values and stale markers.

0 commit comments

Comments
 (0)