From 3c005f3c14660a4191d38e0ea7692a33c6b735b6 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 20:08:14 +1000 Subject: [PATCH 01/10] fix: allow grouping gauge and sum metric charts by materialized columns 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. --- .../metric-materialized-column-group-by.md | 9 ++ .../__tests__/renderChartConfig.int.test.ts | 90 +++++++++++++++++++ .../src/__tests__/renderChartConfig.test.ts | 24 +++++ .../src/core/renderChartConfig.ts | 69 ++++++++++++-- 4 files changed, 187 insertions(+), 5 deletions(-) create mode 100644 .changeset/metric-materialized-column-group-by.md diff --git a/.changeset/metric-materialized-column-group-by.md b/.changeset/metric-materialized-column-group-by.md new file mode 100644 index 0000000000..0e8eb97900 --- /dev/null +++ b/.changeset/metric-materialized-column-group-by.md @@ -0,0 +1,9 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/api': patch +'@hyperdx/app': patch +--- + +fix: allow grouping gauge and sum metric charts by materialized and alias columns + +Grouping or selecting a MATERIALIZED or ALIAS column on a gauge or sum metric chart failed with `Unknown expression identifier`, because the intermediate query didn't carry those columns through. diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts index 740f1c0871..b52d92b84f 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts @@ -519,6 +519,51 @@ describe('renderChartConfig', () => { ); expect(await queryData(query)).toMatchSnapshot(); }); + + describe('with a dotted materialized column', () => { + const table = `${DEFAULT_DATABASE}.${DEFAULT_METRICS_TABLE.GAUGE}`; + + beforeEach(async () => { + await executeSqlCommand( + `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS \`host.name\` String MATERIALIZED ResourceAttributes['host']`, + ); + }); + + afterEach(async () => { + await executeSqlCommand( + `ALTER TABLE ${table} DROP COLUMN IF EXISTS \`host.name\``, + ); + }); + + it('filters and groups on the column', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'max', + metricName: 'test.cpu', + metricType: MetricsDataType.Gauge, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: 'host.name:"host1"', + whereLanguage: 'lucene', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('10m'))], + granularity: '5 minute', + groupBy: '`host.name`', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + const rows = await queryData(query); + expect(rows).toHaveLength(2); + expect(rows.every(r => r['host.name'] === 'host1')).toBe(true); + }); + }); }); describe('Query Metrics - Sum', () => { @@ -693,6 +738,51 @@ describe('renderChartConfig', () => { expect(await queryData(query)).toMatchSnapshot(); }); + describe('with a dotted materialized column', () => { + const table = `${DEFAULT_DATABASE}.${DEFAULT_METRICS_TABLE.SUM}`; + + beforeEach(async () => { + await executeSqlCommand( + `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS \`host.name\` String MATERIALIZED ResourceAttributes['host']`, + ); + }); + + afterEach(async () => { + await executeSqlCommand( + `ALTER TABLE ${table} DROP COLUMN IF EXISTS \`host.name\``, + ); + }); + + it('filters and groups on the column', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'sum', + metricName: 'test.users', + metricType: MetricsDataType.Sum, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: 'host.name:"host1"', + whereLanguage: 'lucene', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('20m'))], + granularity: '5 minute', + groupBy: '`host.name`', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + const rows = await queryData(query); + expect(rows).toHaveLength(4); + expect(rows.every(r => r['host.name'] === 'host1')).toBe(true); + }); + }); + it('sum values as without rate computation', async () => { const query = await renderChartConfig( { diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index b213722f69..6bc1b4f341 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -105,6 +105,30 @@ describe('renderChartConfig', () => { expect(actual).toMatchSnapshot(); }); + it('carries computed columns through the gauge CTEs, skipping CTE alias names', async () => { + mockMetadata.getColumns = jest.fn().mockResolvedValue([ + { name: 'Value', type: 'Float64', default_type: '' }, + { + name: 'host.name', + type: 'String', + default_type: 'MATERIALIZED', + }, + { name: 'region', type: 'String', default_type: 'ALIAS' }, + { name: 'LastValue', type: 'Float64', default_type: 'MATERIALIZED' }, + ]); + const rendered = await renderChartConfig( + gaugeConfiguration, + mockMetadata, + querySettings, + ); + const sql = rendered.sql.replace(/\s+/g, ' '); + expect(sql).toContain('AS AttributesHash, `host.name`, `region` FROM'); + expect(sql).toContain( + 'any(`host.name`) AS `host.name`, any(`region`) AS `region`', + ); + expect(sql).not.toContain('`LastValue`'); + }); + it('should generate sql for a single gauge metric with a delta() function applied', async () => { const generatedSql = await renderChartConfig( { diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index d64f342ce6..481910f302 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1852,6 +1852,52 @@ function renderDeltaExpression( return `IF(${timeDiffInSeconds} > 0, ${valueDiff} * ${intervalInSeconds} / ${timeDiffInSeconds}, 0)`; } +/** + * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select + * them explicitly to keep them usable in the outer select and group-by. + */ +/** + * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select + * them explicitly to keep them usable in the outer select and group-by. + * Names the CTE already defines are skipped to avoid a clash. + */ +async function getComputedMetricColumns( + metadata: Metadata, + { + databaseName, + tableName, + connectionId, + reservedNames, + }: { + databaseName: string; + tableName: string; + connectionId: string; + reservedNames: string[]; + }, +): Promise<{ select: string; aggregate: string }> { + let names: string[] = []; + try { + const columns = await metadata.getColumns({ + databaseName, + tableName, + connectionId, + }); + names = columns + .filter( + c => + (c.default_type === 'MATERIALIZED' || c.default_type === 'ALIAS') && + !reservedNames.includes(c.name), + ) + .map(c => SqlString.escapeId(c.name, true)); + } catch { + // fall back to the fixed projection + } + return { + select: names.map(c => `, ${c}`).join(''), + aggregate: names.map(c => `, any(${c}) AS ${c}`).join(''), + }; +} + async function translateMetricChartConfig( chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, @@ -1932,6 +1978,12 @@ async function translateMetricChartConfig( const bucketValueExpr = _select.isDelta ? renderDeltaExpression(chartConfig, 'Value') : `last_value(Value)`; + const computedColumns = await getComputedMetricColumns(metadata, { + databaseName: from.databaseName, + tableName: metricTables[MetricsDataType.Gauge], + connectionId: chartConfig.connection, + reservedNames: ['AttributesHash', 'LastValue', timeBucketCol], + }); return { ...restChartConfig, @@ -1941,7 +1993,7 @@ async function translateMetricChartConfig( sql: chSql` SELECT *, - cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash${{ UNSAFE_RAW_SQL: computedColumns.select }} FROM ${renderFrom({ from: { ...from, tableName: metricTables[MetricsDataType.Gauge] }, isRenderingRawSqlTemplate: chartConfig.isRenderingRawSqlTemplate, metricType: MetricsDataType.Gauge })} WHERE ${where} `, @@ -1965,7 +2017,7 @@ async function translateMetricChartConfig( any(MetricDescription) AS MetricDescription, any(MetricUnit) AS MetricUnit, any(StartTimeUnix) AS StartTimeUnix, - any(Flags) AS Flags + any(Flags) AS Flags${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source GROUP BY AttributesHash, ${timeBucketCol} ORDER BY AttributesHash, ${timeBucketCol} @@ -2036,6 +2088,13 @@ async function translateMetricChartConfig( metadata, ); + const computedColumns = await getComputedMetricColumns(metadata, { + databaseName: from.databaseName, + tableName: metricTables[MetricsDataType.Sum], + connectionId: chartConfig.connection, + reservedNames: ['AttributesHash', 'Rate', 'Sum', timeBucketCol], + }); + /** * See: https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto * AGGREGATION_TEMPORALITY_DELTA = 1; @@ -2071,7 +2130,7 @@ async function translateMetricChartConfig( AggregationTemporality = 1, SUM(Value) OVER (PARTITION BY AttributesHash ORDER BY TimeUnix ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW), Value - ) AS Sum + ) AS Sum${{ UNSAFE_RAW_SQL: computedColumns.select }} FROM ${renderFrom({ from: { ...from, tableName: metricTables[MetricsDataType.Sum] }, isRenderingRawSqlTemplate: chartConfig.isRenderingRawSqlTemplate, metricType: MetricsDataType.Sum })} WHERE ${where}`, }, @@ -2102,7 +2161,7 @@ async function translateMetricChartConfig( StartTimeUnix, Flags, AggregationTemporality, - IsMonotonic + IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.select }} FROM ( SELECT ${timeExpr}, @@ -2130,7 +2189,7 @@ async function translateMetricChartConfig( any(StartTimeUnix) AS StartTimeUnix, any(Flags) AS Flags, any(AggregationTemporality) AS AggregationTemporality, - any(IsMonotonic) AS IsMonotonic + any(IsMonotonic) AS IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source GROUP BY AttributesHash, \`${timeBucketCol}\` ORDER BY AttributesHash, \`${timeBucketCol}\` From c2b609bf6c8e8dcae07c824d6a9a8b12d1f1db30 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 20:27:00 +1000 Subject: [PATCH 02/10] fix: address review feedback on metric computed columns Fix the int test type error, cover increase + group-by on a computed column, and log when listing computed columns fails. --- .../__tests__/renderChartConfig.int.test.ts | 43 ++++++++++++++++--- .../src/core/renderChartConfig.ts | 9 ++-- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts index b52d92b84f..e8a6c6245d 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts @@ -559,9 +559,9 @@ describe('renderChartConfig', () => { metadata, querySettings, ); - const rows = await queryData(query); - expect(rows).toHaveLength(2); - expect(rows.every(r => r['host.name'] === 'host1')).toBe(true); + expect(await queryData(query)).toEqual( + Array(2).fill(expect.objectContaining({ 'host.name': 'host1' })), + ); }); }); }); @@ -777,9 +777,40 @@ describe('renderChartConfig', () => { metadata, querySettings, ); - const rows = await queryData(query); - expect(rows).toHaveLength(4); - expect(rows.every(r => r['host.name'] === 'host1')).toBe(true); + expect(await queryData(query)).toEqual( + Array(4).fill(expect.objectContaining({ 'host.name': 'host1' })), + ); + }); + + it('ranks increase groups by the column', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'increase', + metricName: 'test.users', + metricType: MetricsDataType.Sum, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: '', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('20m'))], + granularity: '5 minute', + groupBy: '`host.name`', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + expect(await queryData(query)).toEqual( + expect.arrayContaining([ + expect.objectContaining({ 'host.name': 'host1' }), + expect.objectContaining({ 'host.name': 'host2' }), + ]), + ); }); }); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 481910f302..a4a41768f1 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1852,10 +1852,6 @@ function renderDeltaExpression( return `IF(${timeDiffInSeconds} > 0, ${valueDiff} * ${intervalInSeconds} / ${timeDiffInSeconds}, 0)`; } -/** - * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select - * them explicitly to keep them usable in the outer select and group-by. - */ /** * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select * them explicitly to keep them usable in the outer select and group-by. @@ -1889,8 +1885,9 @@ async function getComputedMetricColumns( !reservedNames.includes(c.name), ) .map(c => SqlString.escapeId(c.name, true)); - } catch { - // fall back to the fixed projection + } catch (e) { + // Charts still render, but group-by on a computed column will fail. + console.warn('Failed to list computed metric columns', e); } return { select: names.map(c => `, ${c}`).join(''), From 3f82c638a2eb8ff80dbb9d4dd596f2f849ce99dd Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 22:35:20 +1000 Subject: [PATCH 03/10] fix: keep metric groups separate when a grouped computed column varies 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. --- .../__tests__/renderChartConfig.int.test.ts | 109 ++++++++++++++++++ .../src/__tests__/renderChartConfig.test.ts | 19 ++- .../src/core/renderChartConfig.ts | 29 +++-- 3 files changed, 149 insertions(+), 8 deletions(-) diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts index e8a6c6245d..c1cd8abc61 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts @@ -527,12 +527,18 @@ describe('renderChartConfig', () => { await executeSqlCommand( `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS \`host.name\` String MATERIALIZED ResourceAttributes['host']`, ); + await executeSqlCommand( + `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS value_band String ALIAS if(Value > 20, 'high', 'low')`, + ); }); afterEach(async () => { await executeSqlCommand( `ALTER TABLE ${table} DROP COLUMN IF EXISTS \`host.name\``, ); + await executeSqlCommand( + `ALTER TABLE ${table} DROP COLUMN IF EXISTS value_band`, + ); }); it('filters and groups on the column', async () => { @@ -563,6 +569,69 @@ describe('renderChartConfig', () => { Array(2).fill(expect.objectContaining({ 'host.name': 'host1' })), ); }); + + it('keeps a group per value of a column that varies within a series', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'max', + metricName: 'test.cpu', + metricType: MetricsDataType.Gauge, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: 'host.name:"host1"', + whereLanguage: 'lucene', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('10m'))], + granularity: '5 minute', + groupBy: 'value_band', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + const rows = await queryData(query); + expect(rows.map(r => r.value_band).sort()).toEqual([ + 'high', + 'high', + 'low', + 'low', + ]); + }); + + it('does not split series by a computed column the chart does not group by', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'max', + metricName: 'test.cpu', + metricType: MetricsDataType.Gauge, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: 'host.name:"host1"', + whereLanguage: 'lucene', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('10m'))], + granularity: '5 minute', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + expect( + (await queryData(query)).map( + r => r['max(toFloat64OrDefault(toString(LastValue)))'], + ), + ).toEqual([6.25, 80]); + }); }); }); @@ -745,12 +814,52 @@ describe('renderChartConfig', () => { await executeSqlCommand( `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS \`host.name\` String MATERIALIZED ResourceAttributes['host']`, ); + await executeSqlCommand( + `ALTER TABLE ${table} ADD COLUMN IF NOT EXISTS value_band String ALIAS if(Value > 8, 'high', 'low')`, + ); }); afterEach(async () => { await executeSqlCommand( `ALTER TABLE ${table} DROP COLUMN IF EXISTS \`host.name\``, ); + await executeSqlCommand( + `ALTER TABLE ${table} DROP COLUMN IF EXISTS value_band`, + ); + }); + + it('keeps a group per value of a column that varies within a series', async () => { + const query = await renderChartConfig( + { + select: [ + { + aggFn: 'sum', + metricName: 'test.users', + metricType: MetricsDataType.Sum, + valueExpression: 'Value', + }, + ], + from: metricSource.from, + where: 'host.name:"host1"', + whereLanguage: 'lucene', + metricTables: TEST_METRIC_TABLES, + dateRange: [new Date(now), new Date(now + ms('20m'))], + granularity: '5 minute', + groupBy: 'value_band', + timestampValueExpression: metricSource.timestampValueExpression, + connection: connection.id, + }, + metadata, + querySettings, + ); + const rows = await queryData(query); + const secondBucket = rows.filter( + r => new Date(r.__hdx_time_bucket).getTime() === now + ms('5m'), + ); + expect(secondBucket.map(r => r.value_band).sort()).toEqual([ + 'high', + 'low', + ]); }); it('filters and groups on the column', async () => { diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 6bc1b4f341..ba2743f8b6 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -122,13 +122,30 @@ describe('renderChartConfig', () => { querySettings, ); const sql = rendered.sql.replace(/\s+/g, ' '); - expect(sql).toContain('AS AttributesHash, `host.name`, `region` FROM'); + expect(sql).toContain( + 'cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash, `host.name`, `region` FROM', + ); expect(sql).toContain( 'any(`host.name`) AS `host.name`, any(`region`) AS `region`', ); expect(sql).not.toContain('`LastValue`'); }); + it('hashes grouped-by computed columns into AttributesHash', async () => { + mockMetadata.getColumns = jest.fn().mockResolvedValue([ + { name: 'host.name', type: 'String', default_type: 'MATERIALIZED' }, + { name: 'region', type: 'String', default_type: 'ALIAS' }, + ]); + const rendered = await renderChartConfig( + { ...gaugeConfiguration, groupBy: '`host.name`' }, + mockMetadata, + querySettings, + ); + expect(rendered.sql.replace(/\s+/g, ' ')).toContain( + 'cityHash64(ScopeAttributes, ResourceAttributes, Attributes, tuple(`host.name`)) AS AttributesHash', + ); + }); + it('should generate sql for a single gauge metric with a delta() function applied', async () => { const generatedSql = await renderChartConfig( { diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index a4a41768f1..cc2d2aebfb 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1855,7 +1855,9 @@ function renderDeltaExpression( /** * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select * them explicitly to keep them usable in the outer select and group-by. - * Names the CTE already defines are skipped to avoid a clash. + * Names the CTE already defines are skipped to avoid a clash. Grouped-by ones + * also feed AttributesHash (via tuple, so NULLs hash) so the per-bucket `any()` + * can't merge rows whose values differ. */ async function getComputedMetricColumns( metadata: Metadata, @@ -1864,34 +1866,45 @@ async function getComputedMetricColumns( tableName, connectionId, reservedNames, + groupBy, }: { databaseName: string; tableName: string; connectionId: string; reservedNames: string[]; + groupBy: BuilderChartConfigWithOptDateRangeEx['groupBy']; }, -): Promise<{ select: string; aggregate: string }> { - let names: string[] = []; +): Promise<{ select: string; aggregate: string; hash: string }> { + let columnNames: string[] = []; try { const columns = await metadata.getColumns({ databaseName, tableName, connectionId, }); - names = columns + columnNames = columns .filter( c => (c.default_type === 'MATERIALIZED' || c.default_type === 'ALIAS') && !reservedNames.includes(c.name), ) - .map(c => SqlString.escapeId(c.name, true)); + .map(c => c.name); } catch (e) { // Charts still render, but group-by on a computed column will fail. console.warn('Failed to list computed metric columns', e); } + const groupByText = + typeof groupBy === 'string' + ? groupBy + : (groupBy ?? []).map(g => g.valueExpression).join(','); + const escape = (c: string) => SqlString.escapeId(c, true); + const names = columnNames.map(escape); + // A loose substring match; an extra column only splits series further. + const hashed = columnNames.filter(c => groupByText.includes(c)).map(escape); return { select: names.map(c => `, ${c}`).join(''), aggregate: names.map(c => `, any(${c}) AS ${c}`).join(''), + hash: hashed.length ? `, tuple(${hashed.join(', ')})` : '', }; } @@ -1980,6 +1993,7 @@ async function translateMetricChartConfig( tableName: metricTables[MetricsDataType.Gauge], connectionId: chartConfig.connection, reservedNames: ['AttributesHash', 'LastValue', timeBucketCol], + groupBy: chartConfig.groupBy, }); return { @@ -1990,7 +2004,7 @@ async function translateMetricChartConfig( sql: chSql` SELECT *, - cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash${{ UNSAFE_RAW_SQL: computedColumns.select }} + cityHash64(ScopeAttributes, ResourceAttributes, Attributes${{ UNSAFE_RAW_SQL: computedColumns.hash }}) AS AttributesHash${{ UNSAFE_RAW_SQL: computedColumns.select }} FROM ${renderFrom({ from: { ...from, tableName: metricTables[MetricsDataType.Gauge] }, isRenderingRawSqlTemplate: chartConfig.isRenderingRawSqlTemplate, metricType: MetricsDataType.Gauge })} WHERE ${where} `, @@ -2090,6 +2104,7 @@ async function translateMetricChartConfig( tableName: metricTables[MetricsDataType.Sum], connectionId: chartConfig.connection, reservedNames: ['AttributesHash', 'Rate', 'Sum', timeBucketCol], + groupBy: chartConfig.groupBy, }); /** @@ -2117,7 +2132,7 @@ async function translateMetricChartConfig( sql: chSql` SELECT *, - cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes${{ UNSAFE_RAW_SQL: computedColumns.hash }}) AS AttributesHash, IF( AggregationTemporality = 1, Value, -- DELTA: Value is already the per-interval increase From eacff52bbd1b259d5734ea0c71364047cad88986 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 22:43:13 +1000 Subject: [PATCH 04/10] test: type rows in computed column metric tests --- .../clickhouse/__tests__/renderChartConfig.int.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts index c1cd8abc61..2e8ab5ef08 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts @@ -56,14 +56,14 @@ describe('renderChartConfig', () => { const toClickHouseISOString = (date: Date) => date.toISOString().replace('.000Z', 'Z'); - const queryData = async (chsql: ChSql) => { + const queryData = async (chsql: ChSql) => { try { const res = await clickhouseClient.query<'JSON'>({ query: chsql.sql, query_params: chsql.params, format: 'JSON', }); - const json = await res.json(); + const json = await res.json(); return json.data; } catch (err) { console.error('[ClickhouseClient] Error:', err); @@ -594,7 +594,7 @@ describe('renderChartConfig', () => { metadata, querySettings, ); - const rows = await queryData(query); + const rows = await queryData>(query); expect(rows.map(r => r.value_band).sort()).toEqual([ 'high', 'high', @@ -627,7 +627,7 @@ describe('renderChartConfig', () => { querySettings, ); expect( - (await queryData(query)).map( + (await queryData>(query)).map( r => r['max(toFloat64OrDefault(toString(LastValue)))'], ), ).toEqual([6.25, 80]); @@ -852,7 +852,7 @@ describe('renderChartConfig', () => { metadata, querySettings, ); - const rows = await queryData(query); + const rows = await queryData>(query); const secondBucket = rows.filter( r => new Date(r.__hdx_time_bucket).getTime() === now + ms('5m'), ); From 74d7a8b64bd4bd9b8438d80309a3f5066583e146 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 22:47:41 +1000 Subject: [PATCH 05/10] fix: match computed group-by columns by whole identifier A substring match hashed `service` when grouping by `serviceName`, splitting series the chart didn't ask to split. --- .../src/__tests__/renderChartConfig.test.ts | 42 +++++++++++++++++++ .../src/core/renderChartConfig.ts | 15 +++++-- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index ba2743f8b6..9188db441a 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -146,6 +146,48 @@ describe('renderChartConfig', () => { ); }); + it.each([ + ['a column whose name is a prefix of the grouped one', 'regionName'], + [ + 'a column only named inside a string literal', + "concat('region', ServiceName)", + ], + ])('does not hash %s', async (_, groupBy) => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([ + { name: 'region', type: 'String', default_type: 'ALIAS' }, + ]); + const rendered = await renderChartConfig( + { ...gaugeConfiguration, groupBy }, + mockMetadata, + querySettings, + ); + expect(rendered.sql.replace(/\s+/g, ' ')).toContain( + 'cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash', + ); + }); + + it.each([ + 'lower(region), ServiceName', + "region ILIKE '%a%'", + [{ valueExpression: 'ServiceName' }, { valueExpression: '`region`' }], + ])('hashes a computed column referenced by group-by %p', async groupBy => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([ + { name: 'region', type: 'String', default_type: 'ALIAS' }, + ]); + const rendered = await renderChartConfig( + { ...gaugeConfiguration, groupBy }, + mockMetadata, + querySettings, + ); + expect(rendered.sql.replace(/\s+/g, ' ')).toContain( + 'Attributes, tuple(`region`)) AS AttributesHash', + ); + }); + it('should generate sql for a single gauge metric with a delta() function applied', async () => { const generatedSql = await renderChartConfig( { diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index cc2d2aebfb..79b1ee7cae 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1,3 +1,4 @@ +import escapeRegExp from 'lodash/escapeRegExp'; import isPlainObject from 'lodash/isPlainObject'; import * as SQLParser from 'node-sql-parser'; import SqlString from 'sqlstring'; @@ -1893,14 +1894,20 @@ async function getComputedMetricColumns( // Charts still render, but group-by on a computed column will fail. console.warn('Failed to list computed metric columns', e); } - const groupByText = + // Whole-identifier match with string literals removed; the SQL parser + // rejects valid group-bys such as ILIKE and would drop their columns. + const groupByText = ( typeof groupBy === 'string' ? groupBy - : (groupBy ?? []).map(g => g.valueExpression).join(','); + : (groupBy ?? []).map(g => g.valueExpression).join(',') + ).replace(/'(?:[^'\\]|\\.)*'/g, "''"); const escape = (c: string) => SqlString.escapeId(c, true); const names = columnNames.map(escape); - // A loose substring match; an extra column only splits series further. - const hashed = columnNames.filter(c => groupByText.includes(c)).map(escape); + const hashed = columnNames + .filter(c => + new RegExp(`(? `, ${c}`).join(''), aggregate: names.map(c => `, any(${c}) AS ${c}`).join(''), From 06013797f8ee8df9f6fe9aa09713f3fc2abc1d53 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 23:10:43 +1000 Subject: [PATCH 06/10] fix: count qualified and subcolumn group-by references as computed columns Match Bucketed.region and region.1 against region, and tokenise without a dynamic RegExp so lint stays under the warning cap. --- .../src/__tests__/renderChartConfig.test.ts | 3 ++ .../src/core/renderChartConfig.ts | 28 +++++++++++-------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 9188db441a..1028c95471 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -171,6 +171,9 @@ describe('renderChartConfig', () => { it.each([ 'lower(region), ServiceName', "region ILIKE '%a%'", + 'Bucketed.region', + 'region.1', + '"region"', [{ valueExpression: 'ServiceName' }, { valueExpression: '`region`' }], ])('hashes a computed column referenced by group-by %p', async groupBy => { mockMetadata.getColumns = jest diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 79b1ee7cae..efc8bc5358 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1,4 +1,3 @@ -import escapeRegExp from 'lodash/escapeRegExp'; import isPlainObject from 'lodash/isPlainObject'; import * as SQLParser from 'node-sql-parser'; import SqlString from 'sqlstring'; @@ -1894,20 +1893,27 @@ async function getComputedMetricColumns( // Charts still render, but group-by on a computed column will fail. console.warn('Failed to list computed metric columns', e); } - // Whole-identifier match with string literals removed; the SQL parser - // rejects valid group-bys such as ILIKE and would drop their columns. - const groupByText = ( + // Whole identifiers outside string literals, plus every dotted slice so + // `Bucketed.region` and `region.1` count. Over-matching only splits series. + const groupByText = typeof groupBy === 'string' ? groupBy - : (groupBy ?? []).map(g => g.valueExpression).join(',') - ).replace(/'(?:[^'\\]|\\.)*'/g, "''"); + : (groupBy ?? []).map(g => g.valueExpression).join(','); + const referenced = new Set( + ( + groupByText + .replace(/'(?:[^'\\]|\\.)*'/g, "''") + .match(/`[^`]+`|"[^"]+"|[\p{L}\p{N}_$.]+/gu) ?? [] + ).flatMap(token => { + const parts = token.replace(/^[`"]|[`"]$/g, '').split('.'); + return parts.flatMap((_, i) => + parts.slice(i).map((__, j) => parts.slice(i, i + j + 1).join('.')), + ); + }), + ); const escape = (c: string) => SqlString.escapeId(c, true); const names = columnNames.map(escape); - const hashed = columnNames - .filter(c => - new RegExp(`(? referenced.has(c)).map(escape); return { select: names.map(c => `, ${c}`).join(''), aggregate: names.map(c => `, any(${c}) AS ${c}`).join(''), From 731a67e782f6f75756cc54586aa0f5eeaf0cdba8 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 23:17:42 +1000 Subject: [PATCH 07/10] fix: group metric buckets by computed columns instead of hashing them 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. --- .../__tests__/renderChartConfig.int.test.ts | 14 +++++-- .../src/__tests__/renderChartConfig.test.ts | 41 ++++++++++--------- .../src/core/renderChartConfig.ts | 28 ++++++++----- 3 files changed, 50 insertions(+), 33 deletions(-) diff --git a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts index 2e8ab5ef08..4f93e19282 100644 --- a/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts +++ b/packages/api/src/clickhouse/__tests__/renderChartConfig.int.test.ts @@ -852,13 +852,19 @@ describe('renderChartConfig', () => { metadata, querySettings, ); - const rows = await queryData>(query); + const rows = await queryData<{ + __hdx_time_bucket: string; + value_band: string; + Value: number; + }>(query); const secondBucket = rows.filter( r => new Date(r.__hdx_time_bucket).getTime() === now + ms('5m'), ); - expect(secondBucket.map(r => r.value_band).sort()).toEqual([ - 'high', - 'low', + // The 8 -> 9 step lands in `high`; the rate window must not reset + // when the label changes. + expect(secondBucket.map(r => [r.value_band, r.Value]).sort()).toEqual([ + ['high', 1], + ['low', 0], ]); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 1028c95471..964de7dd86 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -131,7 +131,7 @@ describe('renderChartConfig', () => { expect(sql).not.toContain('`LastValue`'); }); - it('hashes grouped-by computed columns into AttributesHash', async () => { + it('groups buckets by grouped-by computed columns', async () => { mockMetadata.getColumns = jest.fn().mockResolvedValue([ { name: 'host.name', type: 'String', default_type: 'MATERIALIZED' }, { name: 'region', type: 'String', default_type: 'ALIAS' }, @@ -142,7 +142,7 @@ describe('renderChartConfig', () => { querySettings, ); expect(rendered.sql.replace(/\s+/g, ' ')).toContain( - 'cityHash64(ScopeAttributes, ResourceAttributes, Attributes, tuple(`host.name`)) AS AttributesHash', + '`host.name`, any(`region`) AS `region` FROM Source GROUP BY AttributesHash, __hdx_time_bucket2, `host.name` ORDER BY', ); }); @@ -152,7 +152,7 @@ describe('renderChartConfig', () => { 'a column only named inside a string literal', "concat('region', ServiceName)", ], - ])('does not hash %s', async (_, groupBy) => { + ])('does not group buckets by %s', async (_, groupBy) => { mockMetadata.getColumns = jest .fn() .mockResolvedValue([ @@ -164,7 +164,7 @@ describe('renderChartConfig', () => { querySettings, ); expect(rendered.sql.replace(/\s+/g, ' ')).toContain( - 'cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash', + 'GROUP BY AttributesHash, __hdx_time_bucket2 ORDER BY', ); }); @@ -175,21 +175,24 @@ describe('renderChartConfig', () => { 'region.1', '"region"', [{ valueExpression: 'ServiceName' }, { valueExpression: '`region`' }], - ])('hashes a computed column referenced by group-by %p', async groupBy => { - mockMetadata.getColumns = jest - .fn() - .mockResolvedValue([ - { name: 'region', type: 'String', default_type: 'ALIAS' }, - ]); - const rendered = await renderChartConfig( - { ...gaugeConfiguration, groupBy }, - mockMetadata, - querySettings, - ); - expect(rendered.sql.replace(/\s+/g, ' ')).toContain( - 'Attributes, tuple(`region`)) AS AttributesHash', - ); - }); + ])( + 'groups buckets by a computed column referenced by group-by %p', + async groupBy => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([ + { name: 'region', type: 'String', default_type: 'ALIAS' }, + ]); + const rendered = await renderChartConfig( + { ...gaugeConfiguration, groupBy }, + mockMetadata, + querySettings, + ); + expect(rendered.sql.replace(/\s+/g, ' ')).toContain( + 'GROUP BY AttributesHash, __hdx_time_bucket2, `region`', + ); + }, + ); it('should generate sql for a single gauge metric with a delta() function applied', async () => { const generatedSql = await renderChartConfig( diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index efc8bc5358..e753e0c2a1 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1856,8 +1856,8 @@ function renderDeltaExpression( * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select * them explicitly to keep them usable in the outer select and group-by. * Names the CTE already defines are skipped to avoid a clash. Grouped-by ones - * also feed AttributesHash (via tuple, so NULLs hash) so the per-bucket `any()` - * can't merge rows whose values differ. + * join the bucket GROUP BY instead of `any()`, so rows with different values + * stay apart; AttributesHash is left alone because the sum windows use it. */ async function getComputedMetricColumns( metadata: Metadata, @@ -1874,7 +1874,7 @@ async function getComputedMetricColumns( reservedNames: string[]; groupBy: BuilderChartConfigWithOptDateRangeEx['groupBy']; }, -): Promise<{ select: string; aggregate: string; hash: string }> { +): Promise<{ select: string; aggregate: string; groupBy: string }> { let columnNames: string[] = []; try { const columns = await metadata.getColumns({ @@ -1913,11 +1913,19 @@ async function getComputedMetricColumns( ); const escape = (c: string) => SqlString.escapeId(c, true); const names = columnNames.map(escape); - const hashed = columnNames.filter(c => referenced.has(c)).map(escape); return { select: names.map(c => `, ${c}`).join(''), - aggregate: names.map(c => `, any(${c}) AS ${c}`).join(''), - hash: hashed.length ? `, tuple(${hashed.join(', ')})` : '', + aggregate: columnNames + .map(c => + referenced.has(c) + ? `, ${escape(c)}` + : `, any(${escape(c)}) AS ${escape(c)}`, + ) + .join(''), + groupBy: columnNames + .filter(c => referenced.has(c)) + .map(c => `, ${escape(c)}`) + .join(''), }; } @@ -2017,7 +2025,7 @@ async function translateMetricChartConfig( sql: chSql` SELECT *, - cityHash64(ScopeAttributes, ResourceAttributes, Attributes${{ UNSAFE_RAW_SQL: computedColumns.hash }}) AS AttributesHash${{ UNSAFE_RAW_SQL: computedColumns.select }} + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash${{ UNSAFE_RAW_SQL: computedColumns.select }} FROM ${renderFrom({ from: { ...from, tableName: metricTables[MetricsDataType.Gauge] }, isRenderingRawSqlTemplate: chartConfig.isRenderingRawSqlTemplate, metricType: MetricsDataType.Gauge })} WHERE ${where} `, @@ -2043,7 +2051,7 @@ async function translateMetricChartConfig( any(StartTimeUnix) AS StartTimeUnix, any(Flags) AS Flags${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source - GROUP BY AttributesHash, ${timeBucketCol} + GROUP BY AttributesHash, ${timeBucketCol}${{ UNSAFE_RAW_SQL: computedColumns.groupBy }} ORDER BY AttributesHash, ${timeBucketCol} `, }, @@ -2145,7 +2153,7 @@ async function translateMetricChartConfig( sql: chSql` SELECT *, - cityHash64(ScopeAttributes, ResourceAttributes, Attributes${{ UNSAFE_RAW_SQL: computedColumns.hash }}) AS AttributesHash, + cityHash64(ScopeAttributes, ResourceAttributes, Attributes) AS AttributesHash, IF( AggregationTemporality = 1, Value, -- DELTA: Value is already the per-interval increase @@ -2216,7 +2224,7 @@ async function translateMetricChartConfig( any(AggregationTemporality) AS AggregationTemporality, any(IsMonotonic) AS IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source - GROUP BY AttributesHash, \`${timeBucketCol}\` + GROUP BY AttributesHash, \`${timeBucketCol}\`${{ UNSAFE_RAW_SQL: computedColumns.groupBy }} ORDER BY AttributesHash, \`${timeBucketCol}\` ) `, From 2c5d5f58e37c7456dc04c77966def91544b8ecf0 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 23:26:40 +1000 Subject: [PATCH 08/10] fix: keep quoted dotted group-by identifiers whole `host.name` no longer also counts as a reference to host and name. --- .../common-utils/src/__tests__/renderChartConfig.test.ts | 2 ++ packages/common-utils/src/core/renderChartConfig.ts | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 964de7dd86..47aa677ce1 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -152,6 +152,7 @@ describe('renderChartConfig', () => { 'a column only named inside a string literal', "concat('region', ServiceName)", ], + ['a segment of a quoted dotted name', '`host.region`'], ])('does not group buckets by %s', async (_, groupBy) => { mockMetadata.getColumns = jest .fn() @@ -172,6 +173,7 @@ describe('renderChartConfig', () => { 'lower(region), ServiceName', "region ILIKE '%a%'", 'Bucketed.region', + 'Bucketed.`region`', 'region.1', '"region"', [{ valueExpression: 'ServiceName' }, { valueExpression: '`region`' }], diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index e753e0c2a1..b51da3bcdb 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1893,8 +1893,8 @@ async function getComputedMetricColumns( // Charts still render, but group-by on a computed column will fail. console.warn('Failed to list computed metric columns', e); } - // Whole identifiers outside string literals, plus every dotted slice so - // `Bucketed.region` and `region.1` count. Over-matching only splits series. + // Whole identifiers outside string literals. Unquoted dotted names also + // count by slice, so `Bucketed.region` and `region.1` match; quoted stay whole. const groupByText = typeof groupBy === 'string' ? groupBy @@ -1905,7 +1905,8 @@ async function getComputedMetricColumns( .replace(/'(?:[^'\\]|\\.)*'/g, "''") .match(/`[^`]+`|"[^"]+"|[\p{L}\p{N}_$.]+/gu) ?? [] ).flatMap(token => { - const parts = token.replace(/^[`"]|[`"]$/g, '').split('.'); + if (/^[`"]/.test(token)) return [token.slice(1, -1)]; + const parts = token.split('.'); return parts.flatMap((_, i) => parts.slice(i).map((__, j) => parts.slice(i, i + j + 1).join('.')), ); From 41f3c78fd9c6be87502bc6f6f857bcd4ec6c4e95 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 23:47:17 +1000 Subject: [PATCH 09/10] fix: handle escaped identifiers and OTel-named computed metric columns 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. --- .../src/__tests__/renderChartConfig.test.ts | 55 +++++++++++++++++++ .../src/core/renderChartConfig.ts | 52 ++++++++++++++++-- 2 files changed, 101 insertions(+), 6 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 47aa677ce1..fd53af29f7 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -169,6 +169,61 @@ describe('renderChartConfig', () => { ); }); + it.each([ + ['a`b', '`a``b`', '`a``b`'], + ['a"b', '"a""b"', '`a"b`'], + ['a""b', '`a""b`', '`a""b`'], + ])( + 'groups buckets by computed column %s written as %s', + async (name, groupBy, rendered) => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([{ name, type: 'String', default_type: 'ALIAS' }]); + const { sql } = await renderChartConfig( + { ...gaugeConfiguration, groupBy }, + mockMetadata, + querySettings, + ); + expect(sql.replace(/\s+/g, ' ')).toContain( + `GROUP BY AttributesHash, __hdx_time_bucket2, ${rendered}`, + ); + }, + ); + + it('does not re-project a computed column Bucketed already carries', async () => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([ + { name: 'ServiceName', type: 'String', default_type: 'MATERIALIZED' }, + ]); + const rendered = await renderChartConfig( + { ...gaugeConfiguration, groupBy: 'ServiceName' }, + mockMetadata, + querySettings, + ); + const sql = rendered.sql.replace(/\s+/g, ' '); + expect(sql).toContain('AS AttributesHash, `ServiceName` FROM'); + expect(sql).toContain( + 'any(Flags) AS Flags FROM Source GROUP BY AttributesHash, __hdx_time_bucket2 ORDER BY', + ); + }); + + it('carries a computed column the gauge Bucketed CTE does not project', async () => { + mockMetadata.getColumns = jest + .fn() + .mockResolvedValue([ + { name: 'IsMonotonic', type: 'Bool', default_type: 'ALIAS' }, + ]); + const { sql } = await renderChartConfig( + gaugeConfiguration, + mockMetadata, + querySettings, + ); + expect(sql.replace(/\s+/g, ' ')).toContain( + 'any(`IsMonotonic`) AS `IsMonotonic` FROM Source', + ); + }); + it.each([ 'lower(region), ServiceName', "region ILIKE '%a%'", diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index b51da3bcdb..08634234f1 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1852,6 +1852,29 @@ function renderDeltaExpression( return `IF(${timeDiffInSeconds} > 0, ${valueDiff} * ${intervalInSeconds} / ${timeDiffInSeconds}, 0)`; } +// OTel columns each Bucketed CTE already projects with any(). +const GAUGE_BUCKETED_COLUMNS = [ + 'ScopeAttributes', + 'ResourceAttributes', + 'Attributes', + 'ResourceSchemaUrl', + 'ScopeName', + 'ScopeVersion', + 'ScopeDroppedAttrCount', + 'ScopeSchemaUrl', + 'ServiceName', + 'MetricDescription', + 'MetricUnit', + 'StartTimeUnix', + 'Flags', +]; +const SUM_BUCKETED_COLUMNS = [ + ...GAUGE_BUCKETED_COLUMNS, + 'MetricName', + 'AggregationTemporality', + 'IsMonotonic', +]; + /** * `SELECT *` skips MATERIALIZED and ALIAS columns, so the metric CTEs select * them explicitly to keep them usable in the outer select and group-by. @@ -1866,12 +1889,14 @@ async function getComputedMetricColumns( tableName, connectionId, reservedNames, + bucketedColumns, groupBy, }: { databaseName: string; tableName: string; connectionId: string; reservedNames: string[]; + bucketedColumns: string[]; groupBy: BuilderChartConfigWithOptDateRangeEx['groupBy']; }, ): Promise<{ select: string; aggregate: string; groupBy: string }> { @@ -1903,9 +1928,20 @@ async function getComputedMetricColumns( ( groupByText .replace(/'(?:[^'\\]|\\.)*'/g, "''") - .match(/`[^`]+`|"[^"]+"|[\p{L}\p{N}_$.]+/gu) ?? [] + .match( + /`(?:[^`\\]|``|\\.)+`|"(?:[^"\\]|""|\\.)+"|[\p{L}\p{N}_$.]+/gu, + ) ?? [] ).flatMap(token => { - if (/^[`"]/.test(token)) return [token.slice(1, -1)]; + if (/^[`"]/.test(token)) { + // Undo ClickHouse's doubled-quote and backslash escapes. + const quote = token[0]; + return [ + token + .slice(1, -1) + .replaceAll(quote + quote, quote) + .replace(/\\(.)/g, '$1'), + ]; + } const parts = token.split('.'); return parts.flatMap((_, i) => parts.slice(i).map((__, j) => parts.slice(i, i + j + 1).join('.')), @@ -1913,17 +1949,19 @@ async function getComputedMetricColumns( }), ); const escape = (c: string) => SqlString.escapeId(c, true); - const names = columnNames.map(escape); + // A second projection of a Bucketed column would clash with its any() + // alias in GROUP BY. + const bucketed = columnNames.filter(c => !bucketedColumns.includes(c)); return { - select: names.map(c => `, ${c}`).join(''), - aggregate: columnNames + select: columnNames.map(c => `, ${escape(c)}`).join(''), + aggregate: bucketed .map(c => referenced.has(c) ? `, ${escape(c)}` : `, any(${escape(c)}) AS ${escape(c)}`, ) .join(''), - groupBy: columnNames + groupBy: bucketed .filter(c => referenced.has(c)) .map(c => `, ${escape(c)}`) .join(''), @@ -2015,6 +2053,7 @@ async function translateMetricChartConfig( tableName: metricTables[MetricsDataType.Gauge], connectionId: chartConfig.connection, reservedNames: ['AttributesHash', 'LastValue', timeBucketCol], + bucketedColumns: GAUGE_BUCKETED_COLUMNS, groupBy: chartConfig.groupBy, }); @@ -2126,6 +2165,7 @@ async function translateMetricChartConfig( tableName: metricTables[MetricsDataType.Sum], connectionId: chartConfig.connection, reservedNames: ['AttributesHash', 'Rate', 'Sum', timeBucketCol], + bucketedColumns: SUM_BUCKETED_COLUMNS, groupBy: chartConfig.groupBy, }); From 902a0a9cd963e58eb1296b0956d44ce12108b078 Mon Sep 17 00:00:00 2001 From: Jordan Simonovski Date: Wed, 23 Sep 2026 23:58:37 +1000 Subject: [PATCH 10/10] fix: group by OTel-named computed columns instead of any() 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. --- .../src/__tests__/renderChartConfig.test.ts | 5 +- .../src/core/renderChartConfig.ts | 90 ++++++++++--------- 2 files changed, 49 insertions(+), 46 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index fd53af29f7..2264838536 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -190,7 +190,7 @@ describe('renderChartConfig', () => { }, ); - it('does not re-project a computed column Bucketed already carries', async () => { + it('groups by a computed column Bucketed already carries, without projecting it twice', async () => { mockMetadata.getColumns = jest .fn() .mockResolvedValue([ @@ -203,8 +203,9 @@ describe('renderChartConfig', () => { ); const sql = rendered.sql.replace(/\s+/g, ' '); expect(sql).toContain('AS AttributesHash, `ServiceName` FROM'); + expect(sql).not.toContain('any(ServiceName)'); expect(sql).toContain( - 'any(Flags) AS Flags FROM Source GROUP BY AttributesHash, __hdx_time_bucket2 ORDER BY', + 'any(Flags) AS Flags FROM Source GROUP BY AttributesHash, __hdx_time_bucket2, `ServiceName` ORDER BY', ); }); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 08634234f1..70dab887c4 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1899,7 +1899,12 @@ async function getComputedMetricColumns( bucketedColumns: string[]; groupBy: BuilderChartConfigWithOptDateRangeEx['groupBy']; }, -): Promise<{ select: string; aggregate: string; groupBy: string }> { +): Promise<{ + select: string; + aggregate: string; + groupBy: string; + project: (c: string) => { UNSAFE_RAW_SQL: string }; +}> { let columnNames: string[] = []; try { const columns = await metadata.getColumns({ @@ -1949,22 +1954,19 @@ async function getComputedMetricColumns( }), ); const escape = (c: string) => SqlString.escapeId(c, true); - // A second projection of a Bucketed column would clash with its any() - // alias in GROUP BY. - const bucketed = columnNames.filter(c => !bucketedColumns.includes(c)); + const grouped = new Set(columnNames.filter(c => referenced.has(c))); + // Bucketed projects its own columns via `project`, so they aren't repeated. + const extra = columnNames.filter(c => !bucketedColumns.includes(c)); + const project = (c: string) => + grouped.has(c) ? escape(c) : `any(${escape(c)}) AS ${escape(c)}`; return { select: columnNames.map(c => `, ${escape(c)}`).join(''), - aggregate: bucketed - .map(c => - referenced.has(c) - ? `, ${escape(c)}` - : `, any(${escape(c)}) AS ${escape(c)}`, - ) - .join(''), - groupBy: bucketed - .filter(c => referenced.has(c)) - .map(c => `, ${escape(c)}`) - .join(''), + aggregate: extra.map(c => `, ${project(c)}`).join(''), + groupBy: [...grouped].map(c => `, ${escape(c)}`).join(''), + // Names come from the fixed OTel lists, so they need no quoting. + project: (c: string) => ({ + UNSAFE_RAW_SQL: grouped.has(c) ? c : `any(${c}) AS ${c}`, + }), }; } @@ -2077,19 +2079,19 @@ async function translateMetricChartConfig( ${timeExpr}, AttributesHash, ${bucketValueExpr} AS LastValue, - any(ScopeAttributes) AS ScopeAttributes, - any(ResourceAttributes) AS ResourceAttributes, - any(Attributes) AS Attributes, - any(ResourceSchemaUrl) AS ResourceSchemaUrl, - any(ScopeName) AS ScopeName, - any(ScopeVersion) AS ScopeVersion, - any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, - any(ScopeSchemaUrl) AS ScopeSchemaUrl, - any(ServiceName) AS ServiceName, - any(MetricDescription) AS MetricDescription, - any(MetricUnit) AS MetricUnit, - any(StartTimeUnix) AS StartTimeUnix, - any(Flags) AS Flags${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} + ${computedColumns.project('ScopeAttributes')}, + ${computedColumns.project('ResourceAttributes')}, + ${computedColumns.project('Attributes')}, + ${computedColumns.project('ResourceSchemaUrl')}, + ${computedColumns.project('ScopeName')}, + ${computedColumns.project('ScopeVersion')}, + ${computedColumns.project('ScopeDroppedAttrCount')}, + ${computedColumns.project('ScopeSchemaUrl')}, + ${computedColumns.project('ServiceName')}, + ${computedColumns.project('MetricDescription')}, + ${computedColumns.project('MetricUnit')}, + ${computedColumns.project('StartTimeUnix')}, + ${computedColumns.project('Flags')}${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source GROUP BY AttributesHash, ${timeBucketCol}${{ UNSAFE_RAW_SQL: computedColumns.groupBy }} ORDER BY AttributesHash, ${timeBucketCol} @@ -2248,22 +2250,22 @@ async function translateMetricChartConfig( -- deterministic w.r.t. TimeUnix ordering unlike last_value -- which in a GROUP BY context is anyLast (order-dependent). argMax(Source.Sum, TimeUnix) AS Sum, - any(ResourceAttributes) AS ResourceAttributes, - any(ResourceSchemaUrl) AS ResourceSchemaUrl, - any(ScopeName) AS ScopeName, - any(ScopeVersion) AS ScopeVersion, - any(ScopeAttributes) AS ScopeAttributes, - any(ScopeDroppedAttrCount) AS ScopeDroppedAttrCount, - any(ScopeSchemaUrl) AS ScopeSchemaUrl, - any(ServiceName) AS ServiceName, - any(MetricName) AS MetricName, - any(MetricDescription) AS MetricDescription, - any(MetricUnit) AS MetricUnit, - any(Attributes) AS Attributes, - any(StartTimeUnix) AS StartTimeUnix, - any(Flags) AS Flags, - any(AggregationTemporality) AS AggregationTemporality, - any(IsMonotonic) AS IsMonotonic${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} + ${computedColumns.project('ResourceAttributes')}, + ${computedColumns.project('ResourceSchemaUrl')}, + ${computedColumns.project('ScopeName')}, + ${computedColumns.project('ScopeVersion')}, + ${computedColumns.project('ScopeAttributes')}, + ${computedColumns.project('ScopeDroppedAttrCount')}, + ${computedColumns.project('ScopeSchemaUrl')}, + ${computedColumns.project('ServiceName')}, + ${computedColumns.project('MetricName')}, + ${computedColumns.project('MetricDescription')}, + ${computedColumns.project('MetricUnit')}, + ${computedColumns.project('Attributes')}, + ${computedColumns.project('StartTimeUnix')}, + ${computedColumns.project('Flags')}, + ${computedColumns.project('AggregationTemporality')}, + ${computedColumns.project('IsMonotonic')}${{ UNSAFE_RAW_SQL: computedColumns.aggregate }} FROM Source GROUP BY AttributesHash, \`${timeBucketCol}\`${{ UNSAFE_RAW_SQL: computedColumns.groupBy }} ORDER BY AttributesHash, \`${timeBucketCol}\`