fix: allow grouping gauge and sum metric charts by materialized columns - #3191
jordan-simonovski wants to merge 10 commits into
Conversation
The gauge and sum CTEs select * (which skips MATERIALIZED and ALIAS columns) and then project a fixed column list, so an outer group-by on a computed column failed with Unknown expression identifier. Carry those columns through both CTEs.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 902a0a9 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 |
github-actions
Bot
added
the
review/tier-2
🔴 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 ReviewScope: Gauge/sum metric chart rendering — ✅ No critical issues found. No P0/P1 ship-blockers surfaced. The prior-review P1 threads all appear resolved in the current code: 🟡 P2 — recommended
🔵 P3 nitpicks (5)
Reviewers (10): correctness, adversarial, security, performance, testing, maintainability, project-standards, kieran-typescript, previous-comments, learnings. Testing gaps:
Coverage note: correctness, adversarial, previous-comments, TypeScript, and learnings reviewers were still completing at synthesis time; the correctness and previous-comments conclusions above were verified directly against the diff, the surrounding CTE code, and the full prior-comment thread. No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| databaseName: from.databaseName, | ||
| tableName: metricTables[MetricsDataType.Gauge], | ||
| connectionId: chartConfig.connection, | ||
| reservedNames: ['AttributesHash', 'LastValue', timeBucketCol], |
There was a problem hiding this comment.
🔵 minor — reservedNames is a hand-maintained copy of the CTE projection and covers only 3 of ~17 aliases
Derive the reserved set from the projection (or from the filtered column list) rather than restating it. The gauge list is ['AttributesHash','LastValue',timeBucketCol] but the same CTE also defines ScopeAttributes, ResourceAttributes, Attributes, ResourceSchemaUrl, ScopeName, ScopeVersion, ScopeDroppedAttrCount, ScopeSchemaUrl, ServiceName, MetricDescription, MetricUnit, StartTimeUnix, Flags (lines 2008-2020); the sum list at line 2095 omits those plus MetricName, AggregationTemporality, IsMonotonic. A table that declares one of them computed — e.g. ServiceName String MATERIALIZED ResourceAttributes['service.name'], the pattern ClickHouse's own logs schemas use — emits any(ServiceName) AS ServiceName twice in Bucketed and projects the name twice in the sum outer select (line 2164), so the CTE result has a duplicate column and the invariant the doc comment claims ("names the CTE already defines are skipped") does not hold.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
PR Review
4 finding(s): 🔴 0 critical · 🟠 1 major · 🔵 3 minor 4 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
E2E Test Results✅ All tests passed • 382 passed • 1 skipped • 1285s
Tests ran across 4 shards in parallel. |
Fix the int test type error, cover increase + group-by on a computed column, and log when listing computed columns fails.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Grouped-by MATERIALIZED/ALIAS columns now feed AttributesHash, so the per-bucket any() can't merge rows with different values. They go in via tuple() because cityHash64 returns NULL for a NULL argument.
This comment has been minimized.
This comment has been minimized.
A substring match hashed `service` when grouping by `serviceName`, splitting series the chart didn't ask to split.
…lumns Match Bucketed.region and region.1 against region, and tokenise without a dynamic RegExp so lint stays under the warning cap.
AttributesHash also partitions the sum rate window, so folding group-by columns into it reset the window on a label change and dropped the step.
`host.name` no longer also counts as a reference to host and name.
| groupByText | ||
| .replace(/'(?:[^'\\]|\\.)*'/g, "''") | ||
| .match(/`[^`]+`|"[^"]+"|[\p{L}\p{N}_$.]+/gu) ?? [] | ||
| ).flatMap(token => { |
There was a problem hiding this comment.
🔵 minor — Slicing unqualified dotted tokens makes a JSON sub-column path false-match a same-named computed column
Only treat the whole token as a reference (plus an explicit Bucketed.-qualified prefix if needed), or resolve the leading segment against the table's columns first. With a JSON ResourceAttributes column and a MATERIALIZED region, a group-by of ResourceAttributes.`region (the form mergePath builds, packages/app/src/utils.ts:1160-1178) adds region to the bucket GROUP BY, splitting each series-bucket into several LastValue rows and silently changing gauge avg/sum/quantile output.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| typeof groupBy === 'string' | ||
| ? groupBy | ||
| : (groupBy ?? []).map(g => g.valueExpression).join(','); | ||
| const referenced = new Set( |
There was a problem hiding this comment.
🔵 minor — Hand-rolled column-reference tokenizer duplicates extractColumnReferencesFromKey
Reuse extractColumnReferencesFromKey (packages/common-utils/src/clickhouse/index.ts:342 — same input shape, already used by core/metadata.ts:141, app/src/source.ts:336, app/src/utils/materializedViews.ts:434, DBRowTable.tsx:1418), extending it if backtick-quoted dotted names need handling; otherwise add the doc comment the repo conventions require naming the twin and why both must exist. Same for escape on line 1915 — the file already emits identifiers via chSql's Identifier binding (line 1221-1224) and core/metadata.ts exports quoteIdentifierIfNeeded.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Unescape doubled quotes and backslashes in quoted group-by names. Skip computed columns the Bucketed CTE already projects with any(), which otherwise made ClickHouse resolve GROUP BY to that alias.
| Flags, | ||
| AggregationTemporality, | ||
| IsMonotonic | ||
| IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.select }} |
There was a problem hiding this comment.
🟠 major — Sum Bucketed outer SELECT re-projects computed columns it already lists literally
The sum path appends computedColumns.select (the unfiltered columnNames) to the outer Bucketed projection, but that projection already lists all 16 SUM_BUCKETED_COLUMNS by name. On a custom sum table where one of those names is computed — e.g. ServiceName String MATERIALIZED ResourceAttributes['service.name'], exactly the shape this PR targets — the inner subquery emits any(ServiceName) AS ServiceName once (correctly, since bucketed filters it out), and the outer CTE then projects ServiceName twice, making the reference ambiguous/duplicated for the outer query that reads Bucketed. The gauge path is immune only because its select string is used solely in the SELECT * CTE. Return a fourth field built from bucketed (the already-filtered list) and use it here instead of select, and add a sum unit test mirroring 'does not re-project a computed column Bucketed already carries' — the new unit tests only exercise the gauge path, so nothing covers the sum CTE's third insertion point.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| // alias in GROUP BY. | ||
| const bucketed = columnNames.filter(c => !bucketedColumns.includes(c)); | ||
| return { | ||
| select: columnNames.map(c => `, ${escape(c)}`).join(''), |
There was a problem hiding this comment.
🟠 major — Every ALIAS column on the metric table is now evaluated by every gauge/sum query
select/aggregate carry all MATERIALIZED and ALIAS columns, not just the ones the chart references, so each ALIAS expression is computed for every row in range and then any()-aggregated per bucket even for charts that never mention it. One badly-behaved ALIAS on the table (e.g. ALIAS toFloat64(Attributes['pct']), which throws on a non-numeric attribute, or an Array(String) items column like ResourceAttributeItems in docker/clickhouse/local/init-db-e2e.sh:115) then breaks or slows all gauge and sum charts, not just the one using it — a regression for tables that render fine today. The referenced set is already computed here; restrict the carry-through to columns the chart actually references (group-by plus orderBy/having, which restChartConfig forwards to the outer query).
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
|
|
||
| // OTel columns each Bucketed CTE already projects with any(). | ||
| const GAUGE_BUCKETED_COLUMNS = [ |
There was a problem hiding this comment.
🔵 minor — GAUGE_BUCKETED_COLUMNS/SUM_BUCKETED_COLUMNS restate the CTE projections as a second source of truth
These two constants must stay in exact sync with the literal any(...) lists 200-400 lines below (renderChartConfig.ts:2080-2092 and :2251-2266) with nothing tying them together; the sum finding above is what that drift already costs. Derive the list from a single array that both the constant and the generated any(...) projection are built from, so adding or removing a column in the CTE can't silently leave the carry-through logic wrong.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| typeof groupBy === 'string' | ||
| ? groupBy | ||
| : (groupBy ?? []).map(g => g.valueExpression).join(','); | ||
| const referenced = new Set( |
There was a problem hiding this comment.
🔵 minor — Hand-rolled identifier tokenizer/unquoter duplicates existing helpers
Extracting column references from an expression already exists as extractColumnReferencesFromKey (packages/common-utils/src/clickhouse/index.ts:342, which handles map access and JSON paths), and stripping identifier quoting exists as unquoteIdentifier (packages/common-utils/src/core/metadata.ts:120). The repo's DRY rule is marked REQUIRED. Either build referenced on those (parameterizing the quoted-dotted-name behaviour you need) or, if the twin genuinely has to coexist, doc-comment why and name the existing symbol, per the conventions' "two genuinely must coexist" row.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
A grouped computed column named like a fixed Bucketed column (ServiceName, MetricName, ...) now replaces that column's any() projection and joins the GROUP BY, so differing values stay apart.
github-actions
Bot
added
review/tier-4
| Flags, | ||
| AggregationTemporality, | ||
| IsMonotonic | ||
| IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.select }} |
There was a problem hiding this comment.
🟠 major — Sum Bucketed wrapper projects an OTel-named computed column twice
computedColumns.select is every computed column, but this outer wrapper already lists the 16 OTel columns by name (lines 2225–2239). If the sum table declares any of them MATERIALIZED/ALIAS — e.g. the common ServiceName MATERIALIZED ResourceAttributes['service.name'] — the CTE emits ServiceName, … , \ServiceName`, so ClickHouse rejects/ambiguates the reference from the outer query. The inner subquery already re-exposes those under their own names via project(), so only extrabelongs here: return a second string (e.g.selectExtra: extra.map(c => ", " + escape(c)).join("")) and use it at line 2240, keeping selectfor theSourceCTE at line 2209. Note the gauge equivalent is covered by the unit test atpackages/common-utils/src/tests/renderChartConfig.test.ts:380, but there is no sum-path unit test at all — add one with ServiceName` MATERIALIZED against the sum config.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
| } | ||
|
|
||
| // OTel columns each Bucketed CTE already projects with any(). | ||
| const GAUGE_BUCKETED_COLUMNS = [ |
There was a problem hiding this comment.
🔵 minor — GAUGE_BUCKETED_COLUMNS/SUM_BUCKETED_COLUMNS are a hand-synced copy of the CTE projection lists
These arrays must stay identical to the literal computedColumns.project('…') lists at lines 2082–2094 and 2253–2268 and the wrapper list at 2225–2239; drift silently produces either a duplicate projection (the bug above) or a missing column. Drive the projections from the constants instead — e.g. GAUGE_BUCKETED_COLUMNS.map(c => computedColumns.project(c)) joined with , — so there is one source of truth.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const project = (c: string) => | ||
| grouped.has(c) ? escape(c) : `any(${escape(c)}) AS ${escape(c)}`; | ||
| return { | ||
| select: columnNames.map(c => `, ${escape(c)}`).join(''), |
There was a problem hiding this comment.
🔵 minor — Every computed column is carried through both CTEs even when the chart references none
select/aggregate are built from all of columnNames, so a metrics table with N MATERIALIZED/ALIAS columns adds N projections to Source and N any() aggregates to the bucket GROUP BY on every gauge/sum query, including charts that reference none of them (ALIAS columns are evaluated per row at query time). It also makes all metric charts fail after a computed column is dropped, since MetadataCache (packages/common-utils/src/core/metadata.ts:224) never evicts, so the stale name stays in the SQL until the process restarts. Filter columnNames down to the ones actually referenced — the referenced set is already computed here; extend it with chartConfig.orderBy — and fall back to nothing when the chart references none.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| typeof groupBy === 'string' | ||
| ? groupBy | ||
| : (groupBy ?? []).map(g => g.valueExpression).join(','); | ||
| const referenced = new Set( |
There was a problem hiding this comment.
🔵 minor — Identifier extraction, unquoting and quoting are re-implemented instead of reusing existing helpers
Per the REQUIRED DRY rule, three existing implementations of these operations already exist: extractColumnReferencesFromKey (packages/common-utils/src/clickhouse/index.ts:342) pulls column references out of a comma-separated expression list, unquoteIdentifier (packages/common-utils/src/core/metadata.ts:120) — already imported at line 13 of this file — strips one level of `/" quoting, and quoteIdentifierIfNeeded/quoteJsonPathSegment (packages/common-utils/src/core/metadata.ts:148-162) backtick-quote a name. Reuse them, or if the doubled-quote/dotted-name handling genuinely requires a twin, add the doc comment naming the existing symbol and why it does not fit.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
This branch was successfully deployed
3 participants
Summary
Gauge and sum metric charts failed with
Code: 47 Unknown expression identifierwhen grouped by a MATERIALIZED or ALIAS column, such as adeployment.environment.namecolumn pulled out ofResourceAttributes. They now work.AttributesHash,LastValue,Rate,Sum) is skipped, so adding one can't break charts that work today.Implementation detail
The gauge and sum paths build a
SourceCTE withSELECT *, which leaves out MATERIALIZED and ALIAS columns. ABucketedCTE then picks a fixed list of columns, and the chart's group-by runs againstBucketed, so a computed column was never there to find. Both CTEs now pick up the table's computed columns fromDESCRIBE. If that lookup fails, the chart renders the same SQL as before.Tested with integration tests against ClickHouse that add a dotted MATERIALIZED column to the gauge and sum tables, then filter and group by it. Both fail on
mainwith the same error and pass here. A unit test covers passing the columns through and skipping alias names.