feat(sql): lower median / approx_median to the φ=0.5 quantile (#111) - #117
Merged
Merged
Conversation
`median(c)` and `approx_median(c)` reached the aggregate map and were rejected
as `unsupported aggregate`. Both are the φ=0.5 quantile, which the intent
algebra already has: `AggIntent::Quantile { col, q: 0.5, accuracy }`.
They share one intent with `approx_percentile_cont(c, 0.5)`, so CSE can merge
the two spellings. The `approx_` prefix does not force an approximation — the
sketch-vs-exact choice belongs to the AccuracyTarget (`plan::boundary`), which
is how `approx_distinct` and `approx_percentile_cont` already behave.
The argument resolves through `reducer_col`, so `median(a * b)` is rejected
rather than silently lowered with no column (#115).
This closes the `median` part of #111; subquery predicates, DISTINCT ON, and
array_agg remain open there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 9, 2026
zzylol
added a commit
that referenced
this pull request
Jul 10, 2026
…122) `array_agg` has no test, so "deliberately rejected" and "not implemented yet" look identical in the code. Assert the rejection, with the reasoning inline. AggIntent is the vocabulary the planner binds sketches and mergeable accumulators to. array_agg pre-aggregates nothing (output is O(input rows)), has no bounded-memory approximate form, and its partial state is the data itself. Since plan::boundary::realize is an exhaustive match, an AggIntent::ArrayAgg would force boundary, bind and schema derivation to each handle a variant whose every answer is PassThrough. Contrast median (#117), from the same bucket in #111: it *is* Quantile{q:0.5} and feeds the sketch path, so it was implemented rather than rejected. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Part of #111 (the
medianitem; subquery predicates,DISTINCT ONandarray_aggremain open there).Change
median(c)andapprox_median(c)reached the aggregate map incrates/frontend-sql/src/sql/mod.rsand fell through tounsupported aggregate. Both are the φ=0.5 quantile, which the intent algebra already models:Three properties fall out of the existing machinery rather than being special-cased:
reducer_colresolves the argument, somedian(latency)andmedian(bytes)produce distinct intents — andmedian(a * b)is rejected instead of silently lowering withcol: None. Both inherited from L3: AggIntent::Quantile / Cardinality / TopK drop their input column — distinct aggregates compare equal #115.approx_medianis not forced to approximate. The sketch-vs-exact choice is theAccuracyTarget’s (plan::boundary), matching howapprox_distinctandapprox_percentile_contalready behave. UnderEpsilon(0.01)it binds a quantile sketch; underExactit passes through.median(c)andapprox_percentile_cont(c, 0.5)lower to the same intent, soplan::csemerges the two spellings.Tests (4 new)
median_lowers_to_the_half_quantileQuantile { col: Some(2), q: 0.5 }median_is_the_same_intent_as_an_explicit_half_percentilemedian_threads_the_accuracy_targetapprox_medianunderEpsilon(0.01)median_over_an_expression_is_rejectedVerification
cargo fmt --allwas not run —maincarries 159 pre-existing rustfmt diffs, so it would bury the change. Each touched file has the same violation count as onmain.Note on
array_aggThe other aggregate in #111. It has no sketch or summary representation and pre-aggregates nothing, so it does not belong in the intent algebra. I would close that item as working-as-intended rather than implement it — happy to be overruled.
🤖 Generated with Claude Code