Repository navigation
fix: run exact per-series range reductions and reject per-entity aggregates the executor cannot run (#770) - #775
Merged
Merged
Conversation
…er_time (#770) - The executor's per-entity Aggregate path now maps Count, Sum, Avg, Min, Max and Quantile to the columnar Aggregate, grouped by the series identity as distinct/l2/entropy_over_time already were. - asap_types::ir::operator::agg_has_exact_per_entity_kernel lists the intents that path runs; the executor refuses any other intent through it, and Stage 3 reads the same list. - A grouped PromQL avg built from exact sum and count casts the count (Int64) to Float64, as it does a non-Float64 sum, so its division compiles: `avg by (region) (...)` failed in the executor's PromQL binary operator before. - Tests: per-function execution of the exact per-series plans with hand-computed answers, and `avg by (region) (quantile_over_time(0.99, latency_ms[100s]))` planned through plan_installable on a raw-retaining deployment and run end to end.
… run (#770) Stage 3 rejects a candidate whose physical DAG holds an exact per-entity Aggregate without an executor kernel (one measure listed by agg_has_exact_per_entity_kernel, no measure filter, no HAVING) as RejectionCause::PerEntityAggregateNotExecutable, a failed check. The stage_pipeline doc and the DAG viewer's cause list include it.
A Prometheus stale marker (the end-of-series NaN that Remote Write carries) is not a sample. window_batch passed it through, so the exact per-series reductions counted it: over 3, stale, 5 the count was 3 and the sum NaN. The selector now drops stale markers once for every consumer of a window: a range window keeps the samples around one, and an instant selector whose latest sample is one has no row, as in PromQL. An ordinary NaN stays a sample. STALE_MARKER has one definition.
…s for per-series reductions (#770) The per-series execution tests now feed rows through RawSampleSelector, as a runtime does, and cover: a stale marker is not a sample (count 2, sum 8, avg 4, quantile(0) 3 over 3, stale, 5); an ordinary NaN is one; phi of 0, 1, -1 and 2; a sample on the left edge of the window is excluded and one on the right edge included; a series with no sample in the window has no answer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
plan_installablereturns plans its own executor cannot compile. Foravg by (region) (quantile_over_time(0.99, data[100s]))with raw data retained, Stage 3 selects the exact plan, which reduces each series' raw samples with a per-entityAggregate. The executor runs a per-entity aggregate only for distinct, L2 and entropy, so compiling the plan fails withper-entity aggregate requires an explicit entity binding. A runtime using this plan then has to forward every evaluation of these queries elsewhere.What
Aggregateofquantile/sum/avg/min/max/count_over_time, as it already did fordistinct/l2/entropy_over_time.RawSampleSelector::window_batchdrops Prometheus stale markers, which Remote Write carries when a series ends, so no range function counts one as a sample. An instant selector whose latest sample is a stale marker returns no row for that series, as in PromQL. An ordinary NaN is still a sample.Aggregatethat the executor cannot run (for example exactstddev_over_time), with the new causePerEntityAggregateNotExecutable. Such a candidate is never selected.avgbuilt from exact sum and count now compiles. The issue's query needs this fix too: itsavg by (region)failed in the executor's PromQL binary operator, because the count was Int64.How
agg_has_exact_per_entity_kernel(inasap-types) lists the intents the executor's per-entity path runs. The executor refuses every other intent with this predicate, and Stage 3 rejects with it. Plan selection does not depend on the executor, so the list sits in the shared types crate, and the two sides cannot drift.Count/Sum/Avg/Min/Max/Quantileto the existing columnarOperator::aggregatereductions. It groups by every column except the value and time columns (the series identity and labels), as the distinct path does. No new operator is added: grouping useschunked_group_ids, memory is charged per chunk, and cooperative checkpoints are already incolumnar.rs. The existing projection casts an Int64 count to the Float64 sample, and range functions drop__name__.per_entity_aggregate_violationscans the candidate's physical DAG. It accepts a per-entityAggregateonly with one measure that has a kernel, no measure filter and noHAVING, which is exactly what the executor's branch accepts. The check runs before the accuracy check.RawSampleSelector::window_batch), so every consumer of a window gets clean samples: the new per-series path, exact accumulators and sketch builds. The instant selector picks each series' latest sample first and then drops it if it is a stale marker, so an ended series is absent rather than falling back to an older sample. This matches the precompute boundary's contract that stale markers are not samples.STALE_MARKERkeeps a single definition.avgcasts a grouped count to Float64 (last_as_float64), as it already did for a non-Float64 sum.Key code interfaces added / modified
crates/types/src/ir/operator/agg_intent.rscrates/executor/src/samples.rscrates/plan-selection/src/lib.rsUsage (the end-to-end test
an_exact_per_series_quantile_runs_from_raw_samples):For
stddev_over_time(x[5m])at an exact target, each candidate holding the per-entityStdDevaggregate is rejected withcause: per_entity_aggregate_not_executableand a reason such asnode 2: the executor has no exact per-series aggregate [StdDev { col: None, population: true }] over raw samples.Before this PR
With the quantile fixed alone, the grouped avg then failed:
Exact
stddev_over_timeand the other per-series reducers without a kernel were selected and then failed in the executor in the same way.After this PR
{region="east"} 54.505and{region="west"} 5, equal to the query-time reference.NoValidCandidatewith the named cause, so a runtime learns at planning time rather than at compile time.Verification
Regression tests. Each new test failed on the code before its fix and passes now.
planner/tests/runtime_install.rs::an_exact_per_series_quantile_runs_from_raw_samples: the end-to-end case above. Before the fix it failed with both errors shown, one after the other.integration-tests/tests/per_series_exact_execution.rs, with rows fed throughRawSampleSelector:exact_per_series_range_functions_execute:sum/avg/min/max/count_over_timeandquantile_over_time(0.5, …)over two series, against hand-computed values, with no__name__in the output.a_stale_marker_is_not_a_sample: over 3, stale, 5 it expects count 2, sum 8, avg 4, min 3, max 5 and quantile(0) 3. Before the stale fix it got count 3.a_nan_sample_is_a_sample: over 1, NaN, 3 it expects count 3, sum and avg NaN, min 1, max 3 and quantile(0.5) 1.quantile_bounds_follow_promql: φ of 0, 1, −1 and 2 give 10, 40, −Inf and +Inf.window_edges_and_an_empty_window: a sample exactly on the left edge is excluded and one on the right edge included. A series with no sample in the window has no answer.executorsamples::tests::stale_markers_are_not_rows: a range window drops a stale marker and keeps an ordinary NaN. An instant selector whose latest sample is a stale marker returns no row.plan-selection:a_per_entity_aggregate_the_executor_cannot_run_is_rejectedchecks thatstddev_over_timeis rejected with the named cause and never selected.an_executable_per_entity_aggregate_is_not_rejectedchecks that the exact quantile is not rejected.Commands run at the head commit. Each was run with
CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0, which only reduces disk use. All passed.cargo test --workspace --locked: exit 0, 150 test binaries, 0 failures.cargo fmt --all -- --checkcargo clippy -p asap-executor -p asap-integration-tests --all-targets --all-features --locked -- -D warnings, for the crates touched by the stale-marker commits.cargo clippy -p asap-types -p asap-executor -p asap-plan-selection -p asap-logical-asap-optimization -p asap-planner -p asap-integration-tests -p asap-devtools --all-targets --all-features --locked -- -D warnings, run at 05a5c34. No later commit touches crates outside the line above.cargo check -p asap-planner --tests --lockedNot run:
Review. An independent review was done at 05a5c34. Its Medium finding (stale markers) and Low finding (edge-case tests) are addressed in 1eb5928 and 511f010.
Closes #770