Skip to content

SummaryExecutor::execute() has no grouping model -- would silently cross-merge grouped queries #159

Description

@zzylol

Context

Implementing SummaryExecutor for ASAPQuery-backend's data_plane (per
data_plane/docs/l4node-plan-executor-design.md, Step C of the
plan-shaped-serving migration, ASAPQuery-backend#409) surfaced a gap in
crates/sketch/src/exec.rs's execute().

The gap

SummaryExpr::SummaryAgg carries a by: &[ColumnId] field (the grouping
columns), but execute() never uses it for anything structural -- it's
passed straight through to find_candidates(sketch, params, col, by, child) and never touched again:

SummaryExpr::SummaryAgg { child, sketch, params, col, by } => {
    let handles = exec.find_candidates(sketch, params, col, by, child)?;
    ...
    let states = handles.iter().map(|h| exec.fetch_state(h))...;
    let state = fold_states(states, exec)?;   // <-- merges ALL handles into ONE state
    ...
}

fold_states merges every handle find_candidates returned into a
single State, and execute() reads out a single Value per tree. There
is no concept anywhere in the walker of "one output series per distinct
group value" -- the existing test suite in the same file only exercises
ungrouped, single-scalar cases (single_agg_reads_out_directly,
multiple_candidates_for_one_agg_are_merged, etc.).

Why this matters

A query like quantile(0.9, sum by (zone) (m)) is the normal case for a
deployment like ASAPQuery-backend's data_plane (grouped queries, not
ungrouped, dominate real traffic) -- not an edge case. If a deployment
calls execute() the way the trait currently reads, naively, once per
query, find_candidates would return every zone's matching sids, and
fold_states/merge_states would merge sketches across zones into one
blob -- silently producing a wrong answer (one merged series instead of
one series per zone), not an error.

Possible resolutions (not proposing one over another -- flagging for design input)

  1. Deployment-side workaround: the deployment enumerates distinct
    group values itself (outside execute()) and calls execute() once
    per group, with find_candidates scoped to that one group via
    whatever per-call context the deployment's Self closes over. Works
    today with zero changes to this crate, but every deployment with
    grouped queries has to reinvent the same loop, and it's easy to miss
    -- implementing the trait "as documented" without this wrapper
    produces a plausible-looking but silently wrong result, with no error
    to catch it.
  2. First-class support in execute(): SummaryAgg already carries
    by; execute() (or a sibling entry point) could take a
    group-enumeration hook and do the per-group loop + fan-out generically,
    the same way it already owns the other structural rule
    (SummaryMerge's (SummaryKind, SummaryParams) agreement check)
    instead of leaving that to each deployment.
  3. Document the current scope explicitly as "ungrouped queries only"
    if that's an intentional near-term limit, so deployments don't
    discover it by shipping a grouping bug.

Happy to prototype (1) on the ASAPQuery-backend side regardless of which
direction this issue lands on -- filing this first since a workaround that
quietly hides a real gap in the shared trait seemed worse than surfacing
it before more deployments hit the same thing independently.

🤖 Filed with Claude Code while implementing ASAPQuery-backend#409.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions