feat(promql): lower group() and count_values() aggregation operators (#49) - #87
Merged
Merged
Conversation
…49) `group`, `count_values`, `limitk`, `limit_ratio` all parsed but were rejected as UnsupportedAggregateOp. Add the two genuine *aggregation* operators; the sampling operators are split to #86 (they need a selection node, not an aggregate intent). L3 (asap-ir): - `AggIntent::Group` — constant 1 per group ("group presence"). Distinct from Sum/Count (its value is always 1, independent of inputs); previously `group` was mislowered onto `Sum`. - `AggIntent::CountValues { label }` — group the input series by their sample value, count each distinct value, and emit the value as a new label. Unlike every other reducer it adds a synthesized Utf8 label column, so `Aggregate` schema derivation special-cases it (two output columns: the label + count), and drops the by-keys-only unique key. Both added to `agg_is_exact`. L2 (asap-l2): - `AggFunc::Group` / `AggFunc::CountValues { label }` + converter arms. Front end (asap-frontend-promql): - `T_GROUP` → `OuterIntent::Group`; `T_COUNT_VALUES` → `Outer::CountValues` carrying the string label parameter (`str_param`, which unwraps nested parens: `count_values((("v")), m)`). Both flow through the existing two-level `build` / `build_over_subtree` machinery. `limitk`/`limit_ratio` stay rejected: they are series-*sampling* selection (return an unordered subset of series unchanged), so modeling them as topk would change which series pass through. Split to #86, pinned by `limitk_and_limit_ratio_are_rejected__GAP`. Tests: conformance §S (group intent + keys, count_values value→label + count, parenthesised label + by-grouping, limitk/limit_ratio GAP). Flipped the old `group_aggregator_is_rejected` pin and the `group_is_not_sum` equivalence pin (reject → distinct). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`count_values by (job)("job", v)` names the synthesized value-label the same
as a group-by key, which emitted a duplicate `job` column in the output
schema. PromQL's synthesized label takes precedence, so emit a single column:
skip the synthesized label when a column of that name already exists.
Found in self-review of #49.
Co-Authored-By: Claude Opus 4.8 <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.
Closes #49. Splits the sampling operators
limitk/limit_ratioto #86.What
group,count_values,limitk,limit_ratioall parsed but were rejected asUnsupportedAggregateOp. This lands the two genuine aggregation operators; the two sampling operators are split out (they need a selection node, not an aggregate intent).How
L3 (asap-ir)
AggIntent::Group— a constant1per group ("group presence"). Distinct fromSum/Count— its value is always 1 regardless of inputs. (groupwas previously mislowered ontoSum.)AggIntent::CountValues { label }— group the input series by their sample value, count each distinct value, and emit that value as a new label. Unlike every other reducer it adds a synthesizedUtf8label column, soAggregateschema derivation special-cases it (two output columns: the label + the count) and drops the by-keys-only unique key. Both intents added toagg_is_exact.L2 (asap-l2)
AggFunc::Group/AggFunc::CountValues { label }+ converter arms.Front end (asap-frontend-promql)
T_GROUP→OuterIntent::Group;T_COUNT_VALUES→Outer::CountValuescarrying the string label param (str_param, which unwraps nested parens:count_values((("v")), m)). Both route through the existing two-levelbuild/build_over_subtreemachinery, sogroup by (job)/count_values by (job)grouping works.Scope:
limitk/limit_ratiodeferred to #86These are series-sampling selection operators — they return a deterministic-but-unordered subset of the input series unchanged, not a reduction. Modeling
limitkasSort{value} → Limit{k}(topk) would change which series pass through — a silent divergence the corpus actively checks (count(limitk(2, http_requests) and http_requests)).limit_ratioselects a fraction of a runtime-unknown series count and often carries a dynamic ratio. Left rejected, pinned bylimitk_and_limit_ratio_are_rejected__GAP, design in #86.without(...)grouping forms (count_values without (instance)(…),group without(point)(…)) remain rejected by the existing usage-derived-schema limitation — unchanged here.Tests
Conformance §S:
group(up)→Groupintent,groupoutput column;group by (job)keeps the key; regression guard that it is not aSum.count_values("version", build_version)→CountValues{version}; output schema gains aUtf8versionlabel + acountcolumn.count_values by (job) ((("v")), m)— nested-paren label +bygrouping.limitk/limit_ratioGAP.Flipped the old
group_aggregator_is_rejectedconformance pin and thegroup_is_not_sumequivalence pin from reject → distinct-intent.Full workspace suite green; clippy clean.