You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Found while debugging PR #430 (retiring sketch_reducer.rs/shadow_compare.rs). Removing the legacy reducer's silent fallback exposed a pre-existing bug already latent in the merged serving-time cutover (#427) — the reducer was masking it, not the cutover being correct for this shape.
The bug
engine.rs's effective_is_cumulative decides whether SummaryExecutor::readout merges the whole [t0, t1] range into one answer (readout_cumulative, summary_executor.rs:502) or evaluates each distinct sample window independently (readout_per_window, summary_executor.rs:559):
A bare count(hll_metric) (no by, no _over_time) resolves to Capability::CardinalityApprox, and — when the analyzer's outer-agg unwrap leaves candidate.function empty (the count(<bare selector>) idiom; see effective_sketch_function's doc) — falls back to the literal string "cardinality_estimate", which is not in effective_is_cumulative's list. So this instant, whole-range query gets classified as non-cumulative and routed through readout_per_window.
readout_per_window is documented as: "one merged answer per window, not one merged answer for the whole range" — correct for a genuine range/matrix query, but wrong for an instant query like bare count(...), which semantically wants ONE answer merged across everything currently visible, not a value per historical sample timestamp. The caller (l4_readout.rs → engine.rs's instant-vector conversion) then has to pick ONE of the resulting per-window points, and picks whichever is chronologically latest.
Concrete failure mode
live_serve_hll_global_count_merges_across_sids and controller_plan_to_query_full_roundtrip_count_sketch (data_plane/tests/e2e_controller_plans_and_backend_serves.rs, both currently #[ignore]d as of #430) post two samples per sid: a real data window, then a later "watermark" sample (used elsewhere in this codebase to establish a coverage boundary). Under readout_per_window, these are two independent windows; the instant-query path picks the latest one — the watermark — discarding the real data. Confirmed via side-by-side debugging against main that this was already happening before #430 too; it was masked because SummaryExecutor also had a separate, unrelated params-mismatch bug for the same queries (also fixed in #430) that caused it to capability-miss and silently fall through to the legacy sketch_reducer.rs's evaluate_cardinality_global, which merges correctly. With both the reducer and the params mismatch gone, this classification bug is the one thing left standing between these queries and a correct answer.
Suggested fix directions (not investigated in depth yet)
Extend effective_is_cumulative's match to also cover the bare/Plain-outer_fn case for Capability::CardinalityApprox/FrequencyEstimate/FrequencyTopk (i.e. treat "cardinality_estimate"/"frequency"/"topk" as cumulative when there's no explicit _over_time suffix, since an instant aggregate function want one merged answer, not a per-window series) — need to check this doesn't break genuine range/matrix queries that also resolve to these capabilities.
Alternatively/additionally: readout_per_window's caller could pick "the window covering now, merged with earlier windows in scope" rather than "the chronologically latest window" for instant queries — but this seems like it'd just be re-deriving readout_cumulative's job, so (1) is probably the more direct fix.
Context
Found while debugging PR #430 (retiring
sketch_reducer.rs/shadow_compare.rs). Removing the legacy reducer's silent fallback exposed a pre-existing bug already latent in the merged serving-time cutover (#427) — the reducer was masking it, not the cutover being correct for this shape.The bug
engine.rs'seffective_is_cumulativedecides whetherSummaryExecutor::readoutmerges the whole[t0, t1]range into one answer (readout_cumulative,summary_executor.rs:502) or evaluates each distinct sample window independently (readout_per_window,summary_executor.rs:559):A bare
count(hll_metric)(noby, no_over_time) resolves toCapability::CardinalityApprox, and — when the analyzer's outer-agg unwrap leavescandidate.functionempty (thecount(<bare selector>)idiom; seeeffective_sketch_function's doc) — falls back to the literal string"cardinality_estimate", which is not ineffective_is_cumulative's list. So this instant, whole-range query gets classified as non-cumulative and routed throughreadout_per_window.readout_per_windowis documented as: "one merged answer per window, not one merged answer for the whole range" — correct for a genuine range/matrix query, but wrong for an instant query like barecount(...), which semantically wants ONE answer merged across everything currently visible, not a value per historical sample timestamp. The caller (l4_readout.rs→engine.rs's instant-vector conversion) then has to pick ONE of the resulting per-window points, and picks whichever is chronologically latest.Concrete failure mode
live_serve_hll_global_count_merges_across_sidsandcontroller_plan_to_query_full_roundtrip_count_sketch(data_plane/tests/e2e_controller_plans_and_backend_serves.rs, both currently#[ignore]d as of #430) post two samples per sid: a real data window, then a later "watermark" sample (used elsewhere in this codebase to establish a coverage boundary). Underreadout_per_window, these are two independent windows; the instant-query path picks the latest one — the watermark — discarding the real data. Confirmed via side-by-side debugging againstmainthat this was already happening before #430 too; it was masked becauseSummaryExecutoralso had a separate, unrelated params-mismatch bug for the same queries (also fixed in #430) that caused it to capability-miss and silently fall through to the legacysketch_reducer.rs'sevaluate_cardinality_global, which merges correctly. With both the reducer and the params mismatch gone, this classification bug is the one thing left standing between these queries and a correct answer.Suggested fix directions (not investigated in depth yet)
effective_is_cumulative's match to also cover the bare/Plain-outer_fn case forCapability::CardinalityApprox/FrequencyEstimate/FrequencyTopk(i.e. treat"cardinality_estimate"/"frequency"/"topk"as cumulative when there's no explicit_over_timesuffix, since an instant aggregate function want one merged answer, not a per-window series) — need to check this doesn't break genuine range/matrix queries that also resolve to these capabilities.readout_per_window's caller could pick "the window coveringnow, merged with earlier windows in scope" rather than "the chronologically latest window" for instant queries — but this seems like it'd just be re-derivingreadout_cumulative's job, so (1) is probably the more direct fix.References
data_plane/src/query_engines/asap_query_engine/engine.rs:550-582—effective_sketch_function/effective_is_cumulativedata_plane/src/query_engines/asap_query_engine/summary_executor.rs:502/:559—readout_cumulative/readout_per_windowdata_plane/tests/e2e_controller_plans_and_backend_serves.rs— the two#[ignore]d tests, each with a full root-cause comment