[HDX-5126] Apply HAVING/ORDER BY/LIMIT to the composed metric join, not per-series branches - #2946
Conversation
…-series branches (HDX-5126) The composed multi-series metric query spread the whole chart config into each per-series branch, so having/orderBy/limit ran inside every UNION branch — a scope where the user-facing output names don't exist (the value column is renamed __hdx_value) — and the outer join then discarded the per-branch ordering entirely. Each series was also truncated to its own arbitrary LIMIT window, producing mismatched group sets (spurious NULL cells) and incoherent table pagination. All three clauses now render once, on the final joined SELECT, where they reference the output columns: operand aliases (when shown), formula names/aliases, the ratio column, group-by passthroughs and the time bucket. Time charts stay bucket-ordered first, with the user sort as a tiebreaker. Row-level filters (where/filters/aggCondition) stay per-branch, where they belong. Reference contract, pinned by integration tests on CH 26.5: quoted output names or user aliases. Raw expressions over source columns (e.g. ResourceAttributes['service.name']) do not resolve in the outer scope — verified UNKNOWN_IDENTIFIER under the new analyzer — so an expression group-by is referenced via its quoted ClickHouse-derived name or a group-by alias.
🦋 Changeset detectedLatest commit: d5f8306 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
review/tier-2
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Additional context: touches the query rendering engine lightly (53 lines, under the 150-line bar for Tier 4) Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
Greptile SummaryThe PR moves composed metric HAVING, ORDER BY, and LIMIT clauses from individual series branches to the final joined result, with a post-window wrapper for share-of-total filtering.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported share-of-total HAVING issue is addressed by filtering the completed window projection through an outer query, with integration coverage confirming pre-filter denominator semantics.
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/renderChartConfig.ts | Moves composed-query clauses to the final projection and safely filters share-of-total window results through an outer wrapper. |
| packages/common-utils/src/tests/renderChartConfig.test.ts | Verifies clause placement, wrapper shape, bucket-first sorting, string-form ordering, number charts, and formula aliases. |
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Adds ClickHouse integration coverage for joined filtering, ordering, pagination, group aliases, ratios, and post-window share-of-total filtering. |
| packages/common-utils/src/tests/snapshots/renderChartConfig.test.ts.snap | Pins the generated SQL for standard outer clauses and the share-of-total wrapper. |
| .changeset/composed-outer-clauses.md | Documents the corrected composed metric filtering, ordering, and pagination behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Chart configuration] --> B[Render per-series branches]
B --> C[UNION ALL]
C --> D[Compose joined projection]
D --> E{Share-of-total with HAVING?}
E -- No --> F[Apply HAVING]
E -- Yes --> G[Wrap projection and apply WHERE]
F --> H[Apply final ORDER BY and LIMIT]
G --> H
H --> I[Final ClickHouse query]
Reviews (5): Last reviewed commit: "Merge branch 'main' into warren/hdx-5126" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
E2E Test Results✅ All tests passed • 305 passed • 1 skipped • 1023s
Tests ran across 4 shards in parallel. |
Deep ReviewComposed multi-series metric charts now render Independent verification of the highest-risk paths: ✅ No critical issues found. 🟡 P2 -- recommended
🔵 P3 nitpicks (3)
Reviewers (5): maintainability, project-standards, performance, agent-native, learnings-researcher. Coverage note: The correctness, adversarial, testing, and TypeScript reviewers were dispatched but had not returned when synthesis was forced; the correctness/adversarial-class concerns (dangling clauses, window-function leakage into Testing gaps:
|
The share_of_total ratio is the one composed projection built on a window function (sum(...) OVER (...)), and ClickHouse prohibits window functions in HAVING — alias substitution of the ratio output name would pull the OVER() expression straight into the clause and fail the query. When having is set on a share_of_total ratio, the filter now runs as WHERE on a wrapper around the joined result, which evaluates after the window with identical filter-the-output-rows semantics. ORDER BY and LIMIT follow on the outermost statement either way (filter, then order, then limit). Every other projection keeps plain HAVING, rendered byte-identically to before. Flagged by Greptile on the PR; verified with an integration test that the share divides by the pre-filter group total, i.e. the window evaluates over the full joined result before the filter.
Fills the gaps in the HDX-5126 coverage: - number shape + HAVING: the outer query has no GROUP BY ALL (implicit global aggregation) — pinned both as rendered SQL and end-to-end (filters the single row / drops it) - time-series HAVING: filters (bucket, group) joined rows; NULL fails the predicate like any SQL comparison - share_of_total on a time series: the window partitions per bucket inside the wrapper, per-bucket totals divide each share, and the bucket-first ORDER BY sits outside the wrapper - heterogeneous branch classes: HAVING on a gauge column drops histogram rows, whose gauge value is NULL (plain vs Array group columns never share a merge key) - ORDER BY a formula output column (with LIMIT) - hidden operands are not referenceable: showOperandSeries false + HAVING on an operand name rejects instead of silently filtering on a column the chart doesn't show - string-form orderBy (SortSpecificationList string variant) renders once, on the outer statement Deliberately not pinned: seriesLimit still ranks per branch (tracked as an HDX-5126 follow-up — a test would freeze the imperfect behavior) and lucene-language HAVING (the editor and MCP only emit SQL having).
Drop ticket-number references, tighten the outer-clause comment blocks, and correct the fixture-rate recaps to the last-value gauge semantics the table shape actually produces.
2 participants
Fixes HDX-5126 — follow-up to review feedback on #2909: "HAVING is applied within each CTE instead of to the final join … Same with order by."
Why
The composed multi-series metric query (HDX-5077) builds each per-series branch as
{ ...chartConfig, select: [...] }, sohaving,orderByandlimitleaked into every UNION branch:__hdx_value), and filtered each series independently before the join — making it impossible to know what aliases/series/formulas can be referenced.convertToTableChartConfig's default group ordering silent no-ops on composed metric tables.useOffsetPaginatedQueryinjects page offsets intolimit).What
All three clauses now render once, on the final joined SELECT, where they reference the output columns: operand aliases (when shown), formula names/aliases, the ratio column, group-by passthroughs and the time bucket. Time charts stay bucket-ordered first, with the user sort as a tiebreaker. Row-level filters (
where,filters, per-seriesaggCondition) stay per-branch, where they belong.Reference contract (pinned by integration tests on CH 26.5)
Quoted output names or user aliases — e.g.
HAVING "err rate" > 0.5,ORDER BY "avg(metric.total)" DESC, or a group-byalias. Raw expressions over source columns (e.g.ResourceAttributes['service.name']) do not resolve in the outer scope — verifiedUNKNOWN_IDENTIFIERunder the new analyzer — so an expression group-by is referenced via its quoted ClickHouse-derived name ("arrayElement(ResourceAttributes, 'service.name')") or, better, a group-by alias. Hidden operand series (showOperandSeries: false) are not referenceable: the contract is "reference what the result outputs".Tests
renderChartConfig.test.ts): clauses render exactly once, on the outer statement, in HAVING → ORDER BY → LIMIT order; bucket ordering stays first with the user sort appended; HAVING can reference a formula output column. Existing composed snapshots are byte-identical (no clause fields → no change).queryChartConfig.int.test.ts, 8 new): HAVING on an operand output column (including dropping a group whose other series fails the predicate — inexpressible per-branch) and on a formula alias; ORDER BY on a plain group column, an output value column, a quoted derived expression-group-by name, and a group-by alias; bucket-first ordering on time series; ratio-column HAVING/ORDER BY; LIMIT/OFFSET pages are disjoint windows of one consistent joined ordering.Out of scope (noted in HDX-5126)
seriesLimittop-N ranking still runs per-branch.DBTableChartheader-click sorting only quotes explicit user aliases — auto-aliased metric columns and formula columns produce unquotedorderByexpressions that don't resolve (broken before this change too, since branch scopes couldn't resolve them either). Editor autocomplete for output names in the Having/Order By inputs is a separate follow-up.