refactor(ir): single canonical Aggregate output-schema derivation (#41) - #92
Merged
Merged
Conversation
The "output schema of an Aggregate{by, aggs}" was implemented twice: the
canonical `QueryExpr::output_schema_in` arm and a hand-mirrored
`column_resolution::output_schema_for_aggregate` (used only to resolve HAVING
against the aggregate's output). The mirror had already drifted — it lacked
the per-series-reduction branch (rate/increase/*_over_time preserve labels
instead of collapsing to `by ++ aggs`) and, after #49, the count_values
two-column special case and its unique_keys conservatism.
Extract one `aggregate_output_schema(in_schema, by, aggs, output_names,
per_series)` in query_expr.rs and have both callers delegate to it:
- `output_schema_in`'s Aggregate arm computes `per_series` from the child
(the `TimeRange`/`Subquery` marker OR `aggs[0].is_per_series()`), then calls
the shared fn.
- `output_schema_for_aggregate` becomes a thin wrapper: it can't see the child
node, so it computes the child-independent part of `per_series`
(`by.is_empty() && aggs.len()==1 && aggs[0].is_per_series()`) — enough to
agree with the canonical derivation on every reachable input (HAVING is
SQL/cross-series-only and never co-occurs with the range-child marker). It
now returns `Result` (the shared fn errors on an out-of-range group-by id,
matching the canonical arm); the one converter call site threads `?`.
Test: `having_schema_agrees_with_canonical_for_a_per_series_reduction` builds a
per-series `Aggregate{[Rate], TimeRange{Scan}}` and asserts the two
derivations now produce identical schemas — the exact divergence the issue
described, which had no coverage before.
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 #41.
Problem
"What is the output schema of an
Aggregate { by, aggs }?" was answered by two independent implementations:QueryExpr::output_schema_in'sAggregatearm — the canonical, general-purpose one.column_resolution::output_schema_for_aggregate— a hand-mirror used only to resolve aHAVINGpredicate against the aggregate's output columns.The mirror was documented as a copy rather than a shared call, and had already drifted: it lacked the per-series-reduction branch (
rate/increase/*_over_time, which preserve label columns instead of collapsing toby ++ aggs) and — after #49 — thecount_valuestwo-column special case and itsunique_keysconservatism. Latent today (HAVING is SQL-only, per-series is PromQL-only, so they never disagree on a reachable input), but nothing prevented that, and no test asserted agreement.Fix
Extract one canonical
aggregate_output_schema(in_schema, by, aggs, output_names, per_series)inquery_expr.rs; both callers delegate to it:output_schema_in's arm computesper_seriesfrom the child node (theTimeRange/Subquerymarker oraggs[0].is_per_series()) and calls the shared fn — its behavior is unchanged.output_schema_for_aggregatebecomes a thin wrapper. It can't see the child node, so it computes the child-independent part ofper_series(by.is_empty() && aggs.len()==1 && aggs[0].is_per_series()) — enough to agree with the canonical derivation on every reachable input (HAVING is cross-series-only and never co-occurs with the range-child marker). It now returnsResult(the shared fn errors on an out-of-range group-by id, matching the canonical arm); the single converter call site threads?(auto-converted via the existingConvertError: #[from] QueryExprError).Net: the
count_values/ per-series /unique_keys/closedlogic now lives in exactly one place, so the two can never drift again. Future language paths that resolve HAVING get the full derivation for free.Test
having_schema_agrees_with_canonical_for_a_per_series_reductionconstructs the exact shape the issue named —Aggregate{ by: [], [Rate], child: TimeRange{Scan} }— and assertsoutput_schema_for_aggregatenow equalsQueryExpr::output_schemafor it (label-preserving[ts, value], not a collapsed[rate]). This test fails onmainand passes here.Full workspace suite green; clippy clean.