[HDX-5202] Fix multi-series metric ORDER BY on expression group-bys - #3013
[HDX-5202] Fix multi-series metric ORDER BY on expression group-bys#3013wrn14897 wants to merge 1 commit into
Conversation
…es metric queries (HDX-5202) A multi-series metric table grouped by an expression (e.g. ResourceAttributes['service.name']) failed with "Unknown expression or function identifier ResourceAttributes": convertToTableChartConfig defaults orderBy to the raw groupBy text, but the composed outer query can't evaluate those expressions — the source columns don't exist there and the passthrough columns carry ClickHouse-derived names that can't be reproduced node-side. Matched sort items now render through internal __hdx_sort_<n> companion columns projected by each scalar branch (NULL-padded in histogram branches), excluded from the output via * EXCEPT, and referenced from the outer ORDER BY as an aggregate. Group-by entries with a user alias sort through the alias; plain column references are left untouched. Output columns and meta are unchanged.
wrn14897
added
the
ai-generated
🦋 Changeset detectedLatest commit: 713af49 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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.
|
Greptile SummaryFixes multi-series metric ordering when a raw expression group-by is no longer resolvable in the composed query’s outer scope.
Confidence Score: 4/5The share-of-total expression-group-by path still fails and should be fixed before merging. The new companion rewrite repairs ordinary multi-series queries, but its window-projection guard sends share-of-total ratios back through the raw outer ORDER BY that cannot resolve source-dependent expressions. Files Needing Attention: packages/common-utils/src/core/renderChartConfig.ts
|
| Filename | Overview |
|---|---|
| packages/common-utils/src/core/renderChartConfig.ts | Adds expression-aware companion sort columns, but excludes share-of-total ratios and leaves their raw expression ordering unresolved. |
| packages/common-utils/src/tests/renderChartConfig.test.ts | Adds broad SQL-rendering coverage for expression sorts, aliases, histogram padding, and plain-column passthrough, but omits the share-of-total combination. |
| packages/common-utils/src/tests/queryChartConfig.int.test.ts | Exercises expression sorting against ClickHouse, including structured sorts and mixed branches, without covering share-of-total expression ordering. |
| packages/common-utils/src/tests/snapshots/renderChartConfig.test.ts.snap | Records the expected companion-column SQL for the newly covered table configuration. |
| .changeset/multiseries-metric-orderby.md | Documents the user-visible query-generation fix and affected packages. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Multi-series chart config] --> B[Render per-series branches]
B --> C[Project expression companions in scalar branches]
B --> D[NULL-pad companion slots in histogram branches]
C --> E[UNION ALL]
D --> E
E --> F[Pivot values and GROUP BY ALL]
F --> G[ORDER BY companion aggregate]
G --> H[ClickHouse result]
Reviews (1): Last reviewed commit: "fix(common-utils): resolve expression gr..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const rewrittenSort = | ||
| chartConfig.orderBy != null && hasScalarGroups && !usesWindowProjection |
There was a problem hiding this comment.
When a share_of_total multi-series table orders by an expression group-by, this guard disables the companion-column rewrite and leaves the source-dependent expression in the outer ORDER BY, where columns such as ResourceAttributes are unavailable, causing ClickHouse to reject the query and the tile to fail.
Knowledge Base Used:
E2E Test Results✅ All tests passed • 321 passed • 1 skipped • 1346s
Tests ran across 4 shards in parallel. |
github-actions
Bot
added
the
review/tier-4
🔴 Tier 4 — CriticalTouches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI. Why this tier:
Review process: Deep review from a domain expert. Synchronous walkthrough may be required. Stats
|
Deep ReviewThis PR fixes a real, well-scoped bug: multi-series metric tables grouped by an expression failed to render because 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (6): correctness, adversarial, testing, maintainability, previous-comments, kieran-typescript. Testing gaps: No coverage for |
1 participant
Summary
Multi-series metric tiles grouped by an expression (e.g.
ResourceAttributes['service.name'],concat(ResourceAttributes['host.name'], if(...))) failed to render with:Root cause: a multi-series metric chart renders as N per-series subqueries composed via
UNION ALL+ pivot. Group-by columns pass through the outer statement viaSELECT * EXCEPT (...)+GROUP BY ALL, deliberately un-renamed (consumers such as the Kubernetes dashboard look rows up by ClickHouse's derived names likearrayElement(ResourceAttributes, 'service.name'), which can't be reproduced node-side).convertToTableChartConfigdefaults a table'sorderByto the raw groupBy text, so the outerORDER BYre-referenced expressions over source columns that no longer exist in that scope.Fix (
renderMultiSeriesMetricChartConfig): ORDER BY items that repeat a group-by expression verbatim now sort through internal__hdx_sort_<n>companion columns:* EXCEPT, so output columns and meta are byte-for-byte unchangedORDER BYreferences them asany(_hdx_sort)(valid post-GROUP BY ALL; deterministic because the companion duplicates a grouping key)How to test on Vercel preview
N/A — non-UI change (query generation in common-utils).
Testing
concat/ifgroup-by, table defaultorderBy = groupBytext), structured sort items, alias rewrite, histogram padding, plain-column passthrough69176214128f1e5fc4c968ad) with the fix and executed the SQL against the private instance: 175 rows returned, ordered by service/pod, output column names unchangedmake ci-lint,make ci-unit,yarn ci:int(common-utils) all passReferences