fix(l3): thread the input column onto Quantile and Cardinality intents (#115) - #116
Merged
Merged
Conversation
#115) `AggIntent::Quantile` and `AggIntent::Cardinality` carried no input column, so two aggregates over *different* columns lowered to byte-identical L3: approx_percentile_cont(v, 0.5) -> Quantile { q: 0.5, accuracy: Exact } approx_percentile_cont(w, 0.5) -> Quantile { q: 0.5, accuracy: Exact } // equal! Three consequences, all silent: 1. `plan::cse` dedupes on `AggIntent` equality, so a query computing `median(v)` and `median(w)` collapsed to one. 2. `plan::bind` builds the summary over `summarised_column(intent, …)`, which resolves `input_col()`. With no column it always fell through to `ColumnRef::SampleValue`, so every SQL `COUNT(DISTINCT c)` bound its HLL to the wrong column. 3. `approx_percentile_cont(v * 8, 0.95)` succeeded and dropped the expression, because the column slot that would have rejected it did not exist. Root cause was in `l2::lower`: `col` is already in scope and threaded onto Sum/Avg/Min/Max/StdDev/Variance, but was not passed to these two. Adds `col: Option<ColumnId>` to both (`#[serde(default)]`, so pre-#115 payloads still deserialize as `None`), threads it through the converter, and includes them in `input_col()`. `None` keeps its meaning: the PromQL sample value. `TopK` deliberately keeps no `col` — it ranks by the aggregate output rather than a base column (#13 / #25). SQL-side, `approx_percentile_cont` / `COUNT(DISTINCT …)` / `approx_distinct` now resolve their argument with `reducer_col`, so an expression argument is rejected like `SUM(a*b)` instead of silently lowering to `col: None`. A SQL query has no sample value to fall back on. This makes `agg_col_ref` dead; it existed only to perform that fallback. Verified: 363 tests pass, `cargo clippy --all-targets` clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 9, 2026
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.
Closes #115.
The bug
AggIntent::QuantileandAggIntent::Cardinalitycarried no input column, so two aggregates over different columns lowered to byte-identical L3:while
Sum/Avg/Min/Max/StdDev/Varianceall carrycol: Option<ColumnId>and distinguish correctly.Three consequences, all silent:
plan::csededupes onAggIntentequality, so a query computingmedian(v)andmedian(w)collapsed into one.plan::bindpicks the summarised column viainput_col(). With no column it always fell through toColumnRef::SampleValue, so every SQLCOUNT(DISTINCT c)built its HLL over the wrong input.approx_percentile_cont(v * 8, 0.95)succeeded and discardedv * 8, because the column slot that would have rejected it did not exist.SUM(v * 8)correctly errors.Root cause
crates/l2/src/lower.rs.colis already in scope and threaded onto the six reducers above it, but was simply not passed to these two — an omission at the L2→L3 boundary, not a deliberate deferral to L4 (the L2AggFuncdoes carry theColumnRef).The fix
col: Option<ColumnId>onQuantileandCardinality,#[serde(default)]so pre-L3: AggIntent::Quantile / Cardinality / TopK drop their input column — distinct aggregates compare equal #115 serialized L3 still deserializes (asNone).Nonekeeps its existing meaning: the PromQL sample value.l2::lower, and both added toinput_col()— which fixesplan::bindfor free.TopKdeliberately keeps nocol: it ranks by the aggregate output rather than a base column (Do we need AggIntent::TopK? #13 / L3 IR structural inconsistency: SQL and PromQL heavy-hitter topk produce different tree shapes #25). Noted in the type.approx_percentile_cont/COUNT(DISTINCT …)/approx_distinctnow resolve their argument throughreducer_col, so an expression argument is rejected exactly likeSUM(a*b). A SQL query has no sample value to fall back on, socol: Nonethere means "we lost it". This makesagg_col_refdead code — its only purpose was that fallback — so it is removed.Tests (6 new)
quantile_carries_its_input_columncount_distinct_carries_its_input_columnquantile_and_count_distinct_over_an_expression_are_rejectedcse::quantiles_over_different_columns_do_not_dedupebind::sketch_binds_the_intents_input_columnNone→SampleValueagg_intent_serde_reads_pre_115_payloads#[serde(default)]back-compatVerification
The diff is deliberately kept to semantic changes.
mainis not currently rustfmt-clean (159 pre-existing diffs), socargo fmt --allwas not run; each touched file has the same rustfmt-violation count as onmain.🤖 Generated with Claude Code