Skip to content

SummaryExecutor::find_candidates: empty by is ambiguous between "no grouping concept" and "explicit full reduction" #163

Description

@zzylol

Context

Working on ASAPQuery-backend's data_plane implementing SummaryExecutor
(crates/sketch/src/exec.rs) for serving-time execution — same migration as
issues #150/#151. find_candidates(sketch, params, col, by: &[ColumnId], child)'s job is to resolve matching sids and partition them into output
groups via by. This works cleanly whenever by is a genuine, resolvable
grouping request. It breaks down — ambiguously, not just incompletely — when
by is empty.

The problem

An empty by on a SummaryAgg is reachable from two genuinely different
PromQL intents, and the resulting L4Node is byte-identical either way:

  1. No grouping concept exists for this shape at all. A bare per-series
    range function with no by(...) and no label selector — e.g.
    quantile_over_time(0.99, m[10s]). There's nothing in the query text to
    resolve a label column against, so the L3/L4 schema simply never carries
    one. The correct output is one row per underlying series, each
    keeping its own full label identity — merging two series here would be
    silently wrong (combining unrelated distributions/populations).
  2. An explicit, empty reduction was requested. A genuine PromQL
    aggregation operator invoked with no by(...) — e.g. count(hll_metric),
    sum(exact_metric). This means "reduce every matching series into one."
    The correct output is exactly one row, merging every matched sid
    together, regardless of what labels they individually carry.

I confirmed empirically (dumping the bound tree for both) that both cases
produce:

SummaryAgg {
    child: L4Node { ... },
    sketch: <family>,
    params: <params>,
    col: SampleValue,
    by: [],
}

— identical shapes. The distinction exists at the PromQL/L1 surface (an
aggregation operator's own, possibly-empty by(...) clause vs. a construct
that has no such clause to begin with) but doesn't survive the L2→L3
canonicalization that unifies both into the same Aggregate node.

This isn't narrow to cardinality/HLL. Every sketch family in our deployment
(DDSketch/Kll/Hll/Cms/CountSketch) is reachable through both a
bare range function (case 1) and a genuine aggregation operator (case 2) —
e.g. quantile_over_time(...) and quantile(...) both bind to the same
SummaryKind. A find_candidates implementation genuinely cannot tell
which behavior the caller wants from (sketch, params, col, by, child)
alone.

Concretely reproduced: two HLL sids with disjoint item sets, queried via a
count(hll_metric)-shaped tree (by: []), produce two separate ~3-item
cardinality estimates instead of one correct ~6-item merged estimate, under
the conservative "never silently merge on ambiguous input" default (see
below).

What we did instead (for now)

Two family-specific defaults, each independently correct for what it covers,
neither resolving the general question:

  • ExactAgg (Sum/Increase): these AggregationTypes map only from
    genuine aggregation operators (sum(), increase()) — there's no
    bare-range-function path into this family with the same ambiguity
    (sum_over_time/avg_over_time map to different, currently-unmatched
    intents on our end). So an empty by is unambiguous here: reduce fully.
    Projecting onto by (empty → one shared key) is correct as-is.
  • Sketch families: default to case 1's behavior — when by is empty,
    use the sid's own full label identity instead of collapsing to a shared
    empty key. This can never silently merge two series that weren't meant to
    be merged, at the cost of under-serving case 2 (a true full-reduction
    query gets one row per sid instead of one merged row). Case 2 remains
    unsupported through this path; it's still handled, for now, only by our
    own legacy flat-candidate reducer via a bespoke, capability-specific
    special case (global HLL cardinality merges registers before estimating,
    since estimating-then-summing double-counts overlapping members).

Request

Some way to distinguish, at or before find_candidates, "this by is empty
because there's no grouping syntax for this shape" from "this by is empty
because an aggregation operator explicitly reduced everything." Two shapes
that seem plausible, offered as a starting point rather than a specific ask:

  • A signal on SummaryAgg (or an adjacent structure) preserving whichever
    PromQL/L1-level distinction produced the empty by, surviving L2→L3
    canonicalization.
  • Leaving this to deployments entirely, via an extension-point analogous to
    CostModel — a hook a deployment can consult at find_candidates time to
    learn "was this reduction explicit," with asap-sketch itself staying
    agnostic to the answer.

Whichever direction, worth deciding once rather than per-deployment, since
(per the analysis above) every deployment implementing SummaryExecutor
against a sketch family reachable from both a range function and an
aggregation operator will hit the same ambiguity.

References

  • crates/sketch/src/exec.rsSummaryExecutor::find_candidates
  • Related but distinct from Tracking issue for "degree of freedom" we should add when converting from L3 to L4 #152 (L3→L4 degrees of freedom tracking issue) —
    that issue's "Collapsable?" summary property is adjacent but doesn't cover
    this specific by-emptiness ambiguity
  • Downstream: ASAPQuery-backend's data_plane/docs/l4node-plan-executor-design.md,
    "Grouping semantics" section (a deployment-side design doc for the same
    SummaryExecutor work, not part of this repo)

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