Skip to content

L3: AggIntent::Quantile / Cardinality / TopK drop their input column — distinct aggregates compare equal #115

Description

@zzylol

Severity: HIGH — silent semantic collapse, directly on the sketch path

AggIntent::Quantile, AggIntent::Cardinality and AggIntent::TopK carry no input column, unlike every other value reducer. Two aggregates over different columns therefore lower to byte-identical L3 intents.

This is load-bearing: Cardinality is what binds to an HLL sketch and Quantile to a t-digest/KLL. Neither can be built without knowing which column feeds it.

Reproduction (verified on main @ da2ac8e)

SELECT approx_percentile_cont(v, 0.5),
       approx_percentile_cont(w, 0.5),
       count(distinct v),
       count(distinct w)
FROM t;

lowers to:

Quantile { q: 0.5, accuracy: Exact }     <-- v
Quantile { q: 0.5, accuracy: Exact }     <-- w
Cardinality { accuracy: Exact }          <-- v
Cardinality { accuracy: Exact }          <-- w

aggs[0] == aggs[1]  ->  true
aggs[2] == aggs[3]  ->  true

Contrast with the reducers that do carry a column:

sum(v) -> Sum { col: Some(1) }
sum(w) -> Sum { col: Some(2) }

Consequences

  1. CSE miscompilation. crates/plan/src/cse.rs dedupes on AggIntent equality. median(v) and median(w) are equal, so a query computing both collapses to one.
  2. Sketch binding is underdetermined. crates/plan/src/bind.rs maps Cardinality → HLL and Quantile → quantile sketch, but cannot know the input column.
  3. approx_percentile_cont(v * 8, 0.95) silently succeeds and drops the expression entirely, because the column slot that would have rejected it does not exist. SUM(v * 8) correctly errors via reducer_col (unsupported aggregate: sum over a non-column expression) — see SQL: expression GROUP BY (date_trunc time bucketing), aggregates over expressions, and GROUPING SETS/ROLLUP/CUBE are rejected #110.

Root cause

crates/l2/src/lower.rs:660-683. The col binding is already in scope and threaded into Sum / Avg / Min / Max / StdDev / Variance, but simply not passed to the three intents below it:

AggFunc::Sum => AggIntent::Sum { col },
// …
AggFunc::Quantile(q)   => AggIntent::Quantile   { q: *q,          accuracy: acc.clone() },   // col dropped
AggFunc::CountDistinct => AggIntent::Cardinality {                accuracy: acc.clone() },   // col dropped
AggFunc::HeavyHitters { k } => AggIntent::TopK  { k: *k as usize, accuracy: acc.clone() },   // col dropped

It reads as an omission rather than a deliberate deferral to L4 — AggFunc::Quantile/CountDistinct at L2 (relational.rs) do carry the ColumnRef, so the information exists and is discarded at the boundary.

Suggested fix

Add col: Option<ColumnId> to Quantile and Cardinality (mirroring Sum's #[serde(default)]), and thread col through at lower.rs. TopK needs a decision: its ranked column may legitimately be the implicit aggregate output rather than a base column (see #13, #25) — worth splitting out if so.

Consumers to update: crates/plan/src/boundary.rs, crates/plan/src/bind.rs, crates/plan/src/cse.rs, crates/l2/src/canonicalize.rs.

Notes

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

    bugSomething isn't workingpromqlPromQL front-end lowering (L1→L2)sqlSQL front-end lowering (DataFusion → L2)

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions