From bb876b0f91aaede8d57d9bd861ed586cd9b17cf0 Mon Sep 17 00:00:00 2001 From: Mohammed Shoaib Choudry Date: Sat, 19 Sep 2026 16:08:38 +0530 Subject: [PATCH 1/6] fix: label Map attribute columns by their key instead of arrayElement(...) Selecting LogAttributes['service.name'] in the search results left the column named by ClickHouse's derived arrayElement(LogAttributes, 'service.name'), which is wider than the value under it and shows up wherever the result column name is read. renderSelectList now aliases each bare Map subscript in a string select after its key, so the column reads "service.name". The alias is emitted through quotedColumnName, which now escapes backslashes as well as double quotes, and is skipped when it would shadow a name ClickHouse could otherwise resolve (a table column, an alias CTE, or anything else named in the list), since ClickHouse resolves an identifier to an alias before a column. The same key selected from two maps is prefixed with the map. groupBy lists are untouched: GROUP BY x AS y is invalid, and the Kubernetes dashboard reads those rows by their derived names. Fixes #3095 --- .changeset/map-attribute-column-alias.md | 9 + .../src/__tests__/renderChartConfig.test.ts | 208 ++++++++++++++++++ .../src/core/renderChartConfig.ts | 173 ++++++++++++++- 3 files changed, 383 insertions(+), 7 deletions(-) create mode 100644 .changeset/map-attribute-column-alias.md diff --git a/.changeset/map-attribute-column-alias.md b/.changeset/map-attribute-column-alias.md new file mode 100644 index 0000000000..300b1ac825 --- /dev/null +++ b/.changeset/map-attribute-column-alias.md @@ -0,0 +1,9 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +'@hyperdx/api': patch +--- + +fix: label Map attribute columns in search results by their key + +Selecting `LogAttributes['service.name']` as a column labelled the results-table header, and everything else that reads the result column name such as exports, with ClickHouse's derived name `arrayElement(LogAttributes, 'service.name')`. Bare Map subscripts in the select are now aliased after their key, so the column reads `service.name`. A key that would shadow a table column or another selected name keeps the derived name, and the same key selected from two maps is prefixed with the map (`LogAttributes.http.method`). diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index b213722f69..8981636ae7 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -4,6 +4,7 @@ import { ChartConfigWithOptDateRangeEx, renderChartConfig, timeFilterExpr, + withMapSubscriptAliases, } from '@/core/renderChartConfig'; import { convertToCategoricalChartConfig } from '@/core/utils'; import { @@ -5211,4 +5212,211 @@ describe('renderChartConfig', () => { ); }); }); + + describe('Map subscript aliases in a string select (#3095)', () => { + describe('withMapSubscriptAliases', () => { + it('aliases a bare Map subscript after its key', () => { + expect( + withMapSubscriptAliases( + "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", + ), + ).toBe( + 'Timestamp, ' + + 'LogAttributes[\'constructor.index_builder_ac_res_id\'] AS "constructor.index_builder_ac_res_id", ' + + 'ResourceAttributes[\'service.name\'] AS "service.name"', + ); + }); + + it('escapes a backslash in the key', () => { + expect( + withMapSubscriptAliases(String.raw`LogAttributes['dir\\name']`), + ).toBe(String.raw`LogAttributes['dir\\name'] AS "dir\\name"`); + }); + + it('keeps the derived name for a key containing a double quote', () => { + // node-sql-parser, which builds the app's alias map from the rendered + // SQL, rejects a doubled quote in an alias and drops every alias in + // the statement with it. + const select = `LogAttributes['say "hi"'], LogAttributes['other']`; + expect(withMapSubscriptAliases(select)).toBe( + `LogAttributes['say "hi"'], LogAttributes['other'] AS "other"`, + ); + }); + + it('unescapes the single-quote forms of the key literal', () => { + expect( + withMapSubscriptAliases( + String.raw`LogAttributes['it\'s'], SpanAttributes['won''t']`, + ), + ).toBe( + String.raw`LogAttributes['it\'s'] AS "it's", SpanAttributes['won''t'] AS "won't"`, + ); + }); + + it('leaves an already aliased subscript alone', () => { + const select = + "LogAttributes['a'] AS \"a\", LogAttributes['b'] as b, LogAttributes['c'] AS `c d`"; + expect(withMapSubscriptAliases(select)).toBe(select); + }); + + it('leaves plain columns and other expressions alone, byte for byte', () => { + const select = + "Timestamp,ServiceName , toString(LogAttributes['x']), LogAttributes['a']['b'], *"; + expect(withMapSubscriptAliases(select)).toBe(select); + }); + + it('handles a backtick-quoted map column', () => { + expect(withMapSubscriptAliases("`Log Attributes`['k']")).toBe( + '`Log Attributes`[\'k\'] AS "k"', + ); + }); + + it('prefixes the map when the same key is selected from two maps', () => { + expect( + withMapSubscriptAliases( + "LogAttributes['http.method'], SpanAttributes['http.method'], LogAttributes['other']", + ), + ).toBe( + 'LogAttributes[\'http.method\'] AS "LogAttributes.http.method", ' + + 'SpanAttributes[\'http.method\'] AS "SpanAttributes.http.method", ' + + 'LogAttributes[\'other\'] AS "other"', + ); + }); + + it('keeps the derived name when the key is a reserved column', () => { + // ClickHouse resolves an identifier to an alias before a column, so + // `AS "ServiceName"` would hijack a `WHERE ServiceName = ...`. + const select = "Timestamp, ResourceAttributes['ServiceName']"; + expect(withMapSubscriptAliases(select, ['ServiceName'])).toBe(select); + }); + + it('keeps the derived name when the key is referenced elsewhere in the list', () => { + for (const select of [ + "ServiceName, ResourceAttributes['ServiceName']", + "LogAttributes['x'] AS y, SpanAttributes['y']", + "LogAttributes['x'] AS \"y\", SpanAttributes['y']", + "concat(host, '-'), ResourceAttributes['host']", + ]) { + expect(withMapSubscriptAliases(select)).toBe(select); + } + }); + + it('still aliases a key that only appears inside a string literal', () => { + expect( + withMapSubscriptAliases( + "concat(ServiceName, 'host'), ResourceAttributes['host']", + ), + ).toBe( + "concat(ServiceName, 'host'), ResourceAttributes['host'] AS \"host\"", + ); + }); + }); + + describe('renderChartConfig', () => { + const searchConfig = ( + select: string, + overrides: Partial = {}, + ): ChartConfigWithOptDateRange => ({ + connection: 'test-connection', + from: { databaseName: 'default', tableName: 'otel_logs' }, + select, + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'timestamp', + dateRange: [new Date('2025-01-01'), new Date('2025-01-02')], + ...overrides, + }); + + it('names the result columns after the selected keys', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig( + "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", + ), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain( + 'SELECT Timestamp, ' + + 'LogAttributes[\'constructor.index_builder_ac_res_id\'] AS "constructor.index_builder_ac_res_id", ' + + 'ResourceAttributes[\'service.name\'] AS "service.name" ' + + 'FROM default.otel_logs', + ); + expect(mockMetadata.getColumns).toHaveBeenCalledWith({ + databaseName: 'default', + tableName: 'otel_logs', + connectionId: 'test-connection', + }); + }); + + it('does not alias a key that names a column of the source table', async () => { + // mockMetadata.getColumns lists a `value` column. + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig("LogAttributes['value'], LogAttributes['other']"), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain( + "SELECT LogAttributes['value'], LogAttributes['other'] AS \"other\" FROM", + ); + }); + + it('does not alias a key that names an alias CTE', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig("ResourceAttributes['service']", { + with: [ + { name: 'service', sql: chSql`ServiceName`, isSubquery: false }, + ], + }), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain("SELECT ResourceAttributes['service'] FROM"); + }); + + it('never aliases a string groupBy', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig('count()', { + groupBy: "ResourceAttributes['service.name']", + }), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain( + "SELECT count(),ResourceAttributes['service.name'] FROM", + ); + expect(sql).toContain("GROUP BY ResourceAttributes['service.name']"); + expect(sql).not.toContain('AS "service.name"'); + }); + + it('renders the list as written when the table columns are unknown', async () => { + mockMetadata.getColumns.mockRejectedValueOnce(new Error('no access')); + const select = "LogAttributes['k']"; + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig(select), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain(`SELECT ${select} FROM`); + }); + + it('skips the column lookup when nothing needs an alias', async () => { + await renderChartConfig( + searchConfig('Timestamp, ServiceName, Body'), + mockMetadata, + querySettings, + ); + expect(mockMetadata.getColumns).not.toHaveBeenCalled(); + }); + }); + }); }); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index eb5544ad66..b7efec4116 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -169,12 +169,119 @@ const MULTI_SERIES_SORT_ALIAS_PREFIX = '__hdx_sort_'; /** * Render a user-facing output column name as a ClickHouse double-quoted * identifier. User aliases (and metric names, which flow into the default - * aliases) can contain double quotes; ClickHouse escapes them by doubling, - * so `bad"name` becomes "bad""name" instead of terminating the identifier - * early. Escaping happens only at SQL-emission time — collision dedup and - * the meta column names consumers see keep the raw name. + * aliases) can contain double quotes and backslashes. ClickHouse reads a + * quoted identifier like a string literal, so `"` is doubled and `\` is + * escaped: `bad"name` becomes "bad""name" instead of terminating the + * identifier early, and `a\b` becomes "a\\b" instead of dropping to "ab". + * Escaping happens only at SQL-emission time — collision dedup and the meta + * column names consumers see keep the raw name. */ -const quotedColumnName = (name: string) => `"${name.replace(/"/g, '""')}"`; +const quotedColumnName = (name: string) => + `"${name.replace(/\\/g, '\\\\').replace(/"/g, '""')}"`; + +// A bare Map subscript: a plain or backtick-quoted column followed by one +// single-quoted key and nothing else, e.g. `LogAttributes['service.name']`. +// Captures the column as written and the key still SQL-escaped. A subscript +// wrapped in a function, nested, or carrying an alias does not match. +// Stricter than parseKeyPath (metadata.ts), which slices on the first `['` +// and expects input already known to be a key path. +const BARE_MAP_SUBSCRIPT_RE = + /^(`(?:[^`]|``)+`|[A-Za-z_][A-Za-z0-9_]*)\s*\[\s*'((?:[^'\\]|\\.|'')*)'\s*\]$/; + +// Undo the escapes a single-quoted ClickHouse literal carries for `'` and +// `\` — the two the app emits when it builds a subscript from a row's key +// (mergePath). Other C-style escapes stay as typed. +const unescapeSqlStringLiteral = (literal: string) => + literal.replace(/\\(['\\])|''/g, (_, escaped?: string) => escaped ?? "'"); + +/** + * Identifier-like tokens of a select entry outside string literals: bare + * words plus the contents of backtick- and double-quoted identifiers. A + * superset of the names the entry binds (keywords and function names come + * along), which only makes the alias collision check more conservative. + */ +function identifierTokens(entry: string): string[] { + const tokens: string[] = []; + const re = + /'(?:[^'\\]|\\.|'')*'|`((?:[^`]|``)+)`|"((?:[^"]|"")+)"|[A-Za-z_][A-Za-z0-9_.]*/g; + for (const match of entry.matchAll(re)) { + if (match[1] != null) { + tokens.push(match[1].replace(/``/g, '`')); + } else if (match[2] != null) { + tokens.push(match[2].replace(/""/g, '"')); + } else if (!match[0].startsWith("'")) { + tokens.push(match[0]); + } + } + return tokens; +} + +/** + * Alias each bare Map subscript in a string select list after its key, so + * the result column is named `service.name` rather than ClickHouse's derived + * `arrayElement(ResourceAttributes, 'service.name')`. Entries that are plain + * columns, other expressions, or already aliased are left as written. + * + * ClickHouse resolves an identifier to a query alias before a table column, + * so an alias must never shadow a name the rest of the query can reference. + * A key is skipped when it is one of `reservedNames` (the source table's + * columns, alias CTEs) or appears as an identifier anywhere in the list (a + * selected column, an explicit alias, a function argument). The same key + * selected from two maps is aliased with the map as prefix + * (`LogAttributes.http.method`, `SpanAttributes.http.method`), since + * ClickHouse rejects two expressions under one alias; an entry whose alias + * is still taken keeps its derived name. + * + * A key containing a double quote also keeps its derived name: the app's + * alias map (chSqlToAliasMap) parses the rendered statement with + * node-sql-parser, which rejects a doubled quote inside an alias, and an + * unparseable statement drops every alias in the query, not just that one. + * + * Returns the input untouched when nothing is aliased; otherwise the entries + * are re-joined with `, `. + */ +export function withMapSubscriptAliases( + select: string, + reservedNames: Iterable = [], +): string { + const entries = splitAndTrimWithBracket(select).map(entry => { + const match = entry.match(BARE_MAP_SUBSCRIPT_RE); + const key = match == null ? undefined : unescapeSqlStringLiteral(match[2]); + return match == null || key == null || key.includes('"') + ? { entry } + : { + entry, + subscript: { + column: unquoteIdentifier(match[1]).replace(/``/g, '`'), + key, + }, + }; + }); + if (!entries.some(e => e.subscript != null)) { + return select; + } + + const taken = new Set(reservedNames); + const keyCount = new Map(); + for (const { entry, subscript } of entries) { + for (const token of identifierTokens(entry)) taken.add(token); + if (subscript != null) { + keyCount.set(subscript.key, (keyCount.get(subscript.key) ?? 0) + 1); + } + } + + let aliased = false; + const out = entries.map(({ entry, subscript: s }) => { + if (s == null) return entry; + const alias = + (keyCount.get(s.key) ?? 0) > 1 ? `${s.column}.${s.key}` : s.key; + if (taken.has(alias)) return entry; + taken.add(alias); + aliased = true; + return `${entry} AS ${quotedColumnName(alias)}`; + }); + return aliased ? out.join(', ') : select; +} // Histogram translations bake the group-by dimensions into a single Array // column named GROUP_ALIAS instead of projecting them as individual columns @@ -726,16 +833,67 @@ type RenderSelectListOptions = { * otherwise render `divide(ServiceName, Region)` into the GROUP BY clause. */ mergeRatio: boolean; + /** + * Whether bare Map subscripts in a string list get an alias after their key + * (see withMapSubscriptAliases). + * + * True only for the chart's *select* list, where it names the output + * columns. A `groupBy` list must pass `false`: `GROUP BY x AS y` is a + * syntax error, and consumers such as the Kubernetes dashboard look + * group-by rows up by ClickHouse's derived names. + */ + aliasMapSubscripts?: boolean; }; +/** + * withMapSubscriptAliases over a chart's string select list, reserving the + * source table's columns and any alias CTE names. Without the column list (a + * CTE source, or DESCRIBE failing) the list renders as written: an alias that + * shadowed a column referenced in WHERE would silently change what the query + * filters on. + */ +async function aliasStringSelectList( + selectList: string, + chartConfig: BuilderChartConfigWithOptDateRangeEx, + metadata: Metadata, +): Promise { + const { from, with: withClauses } = chartConfig; + if ( + !from.databaseName || + hasSubqueryCte(withClauses) || + !splitAndTrimWithBracket(selectList).some(entry => + BARE_MAP_SUBSCRIPT_RE.test(entry), + ) + ) { + return selectList; + } + try { + const columns = await metadata.getColumns({ + databaseName: from.databaseName, + tableName: from.tableName, + connectionId: chartConfig.connection, + }); + return withMapSubscriptAliases(selectList, [ + ...columns.map(c => c.name), + ...(withClauses ?? []).map(w => w.name), + ]); + } catch { + return selectList; + } +} + async function renderSelectList( selectList: SelectList, chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, - { mergeRatio }: RenderSelectListOptions, + { mergeRatio, aliasMapSubscripts = false }: RenderSelectListOptions, ) { if (typeof selectList === 'string') { - return chSql`${{ UNSAFE_RAW_SQL: selectList }}`; + return chSql`${{ + UNSAFE_RAW_SQL: aliasMapSubscripts + ? await aliasStringSelectList(selectList, chartConfig, metadata) + : selectList, + }}`; } // This metadata query is executed in an attempt tp optimize the selects by favoring materialized fields @@ -1169,6 +1327,7 @@ async function renderSelect( ) : await renderSelectList(chartConfig.select, chartConfig, metadata, { mergeRatio: true, + aliasMapSubscripts: true, }), isIncludingGroupBy && chartConfig.selectGroupBy !== false ? await renderSelectList(chartConfig.groupBy, chartConfig, metadata, { From 19292b50759f81f195a53ae8243988040da085be Mon Sep 17 00:00:00 2001 From: Mohammed Shoaib Choudry Date: Sun, 20 Sep 2026 16:05:15 +0530 Subject: [PATCH 2/6] fix: skip Map keys a group-by alias or a backslash would break Review round 1. Reserve the aliases a structured group-by binds before aliasing Map subscripts. renderGroupBy projects those aliases into the same SELECT, so a key equal to one of them produced two expressions under a single alias, which ClickHouse rejects. Skip a key containing a backslash as well as one containing a double quote, for the same reason: neither survives the round trip back through chSqlToAliasMap, so the recovered name never matches the column name ClickHouse reports and the alias cannot be addressed. Add a test feeding the rendered SQL through chSqlToAliasMap, which pins the round trip the feature rests on, and keep the backslash escaping in quotedColumnName covered by a formula-column test now that the Map path no longer reaches it. --- .../src/__tests__/renderChartConfig.test.ts | 73 +++++++++++++++++-- .../src/core/renderChartConfig.ts | 51 +++++++++++-- 2 files changed, 113 insertions(+), 11 deletions(-) diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 8981636ae7..560a05e583 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -1,4 +1,9 @@ -import { chSql, ColumnMeta, parameterizedQueryToSql } from '@/clickhouse'; +import { + chSql, + chSqlToAliasMap, + ColumnMeta, + parameterizedQueryToSql, +} from '@/clickhouse'; import { Metadata } from '@/core/metadata'; import { ChartConfigWithOptDateRangeEx, @@ -5141,6 +5146,21 @@ describe('renderChartConfig', () => { expect(sql).toContain('AS "errors__2"'); }); + it('escapes backslashes in the formula column name', async () => { + const generatedSql = await renderChartConfig( + { + ...baseEventFormulaConfig, + formulas: [{ expression: 'A / B', alias: String.raw`dir\name` }], + }, + mockMetadata, + querySettings, + ); + const sql = parameterizedQueryToSql(generatedSql); + // ClickHouse reads a double-quoted identifier like a string literal, so + // a lone backslash would escape the character after it. + expect(sql).toContain(String.raw`AS "dir\\name"`); + }); + it('escapes double quotes in the formula column name', async () => { const generatedSql = await renderChartConfig( { @@ -5227,10 +5247,14 @@ describe('renderChartConfig', () => { ); }); - it('escapes a backslash in the key', () => { - expect( - withMapSubscriptAliases(String.raw`LogAttributes['dir\\name']`), - ).toBe(String.raw`LogAttributes['dir\\name'] AS "dir\\name"`); + it('keeps the derived name for a key containing a backslash', () => { + // Same round trip as a double quote: node-sql-parser hands the alias + // back with its escape still in place, so the app's alias map never + // matches the name ClickHouse reports for the column. + const select = String.raw`LogAttributes['dir\\name'], LogAttributes['other']`; + expect(withMapSubscriptAliases(select)).toBe( + String.raw`LogAttributes['dir\\name'], LogAttributes['other'] AS "other"`, + ); }); it('keeps the derived name for a key containing a double quote', () => { @@ -5409,6 +5433,45 @@ describe('renderChartConfig', () => { expect(sql).toContain(`SELECT ${select} FROM`); }); + it('does not alias a key that a group-by projection already names', async () => { + const sql = parameterizedQueryToSql( + await renderChartConfig( + searchConfig("LogAttributes['service'], LogAttributes['other']", { + groupBy: [ + { + aggCondition: '', + valueExpression: 'Region', + alias: 'service', + }, + ], + }), + mockMetadata, + querySettings, + ), + ); + expect(sql).toContain( + "SELECT LogAttributes['service'], LogAttributes['other'] AS \"other\"", + ); + expect(sql).toContain('Region AS "service"'); + }); + + it('renders aliases the app can map back to their expressions', async () => { + // The feature needs no app change only because chSqlToAliasMap + // recovers the expression behind each alias from the rendered SQL. + const query = await renderChartConfig( + searchConfig( + "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", + ), + mockMetadata, + querySettings, + ); + expect(chSqlToAliasMap(query)).toEqual({ + 'constructor.index_builder_ac_res_id': + "LogAttributes['constructor.index_builder_ac_res_id']", + 'service.name': "ResourceAttributes['service.name']", + }); + }); + it('skips the column lookup when nothing needs an alias', async () => { await renderChartConfig( searchConfig('Timestamp, ServiceName, Body'), diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index b7efec4116..8728c2c759 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -232,10 +232,13 @@ function identifierTokens(entry: string): string[] { * ClickHouse rejects two expressions under one alias; an entry whose alias * is still taken keeps its derived name. * - * A key containing a double quote also keeps its derived name: the app's + * A key containing a double quote or a backslash also keeps its derived + * name, because neither survives the round trip back to the key. The app's * alias map (chSqlToAliasMap) parses the rendered statement with - * node-sql-parser, which rejects a doubled quote inside an alias, and an - * unparseable statement drops every alias in the query, not just that one. + * node-sql-parser, which rejects a doubled quote inside an alias — and an + * unparseable statement drops every alias in the query, not just that one — + * and leaves a backslash escape as written rather than unescaping it, so the + * recovered name never matches the column name ClickHouse reports. * * Returns the input untouched when nothing is aliased; otherwise the entries * are re-joined with `, `. @@ -247,7 +250,7 @@ export function withMapSubscriptAliases( const entries = splitAndTrimWithBracket(select).map(entry => { const match = entry.match(BARE_MAP_SUBSCRIPT_RE); const key = match == null ? undefined : unescapeSqlStringLiteral(match[2]); - return match == null || key == null || key.includes('"') + return match == null || key == null || /["\\]/.test(key) ? { entry } : { entry, @@ -843,8 +846,30 @@ type RenderSelectListOptions = { * group-by rows up by ClickHouse's derived names. */ aliasMapSubscripts?: boolean; + /** + * Extra names a Map-subscript alias must not take, on top of the source + * table's columns and the alias CTEs. Used for the aliases the structured + * group-by binds, which are not visible in the select list itself. + */ + reservedNames?: Iterable; }; +/** + * Output names a structured select list binds. A group-by list is projected + * into the same SELECT as the select list, so its aliases are names a + * Map-subscript alias must not reuse: ClickHouse rejects one alias standing + * for two different expressions. + */ +function selectListAliases(selectList: SelectList | undefined): string[] { + return typeof selectList === 'string' || selectList == null + ? [] + : selectList.flatMap(select => + select.alias != null && select.alias.trim() !== '' + ? [select.alias] + : [], + ); +} + /** * withMapSubscriptAliases over a chart's string select list, reserving the * source table's columns and any alias CTE names. Without the column list (a @@ -856,6 +881,7 @@ async function aliasStringSelectList( selectList: string, chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, + reservedNames: Iterable, ): Promise { const { from, with: withClauses } = chartConfig; if ( @@ -876,6 +902,7 @@ async function aliasStringSelectList( return withMapSubscriptAliases(selectList, [ ...columns.map(c => c.name), ...(withClauses ?? []).map(w => w.name), + ...reservedNames, ]); } catch { return selectList; @@ -886,12 +913,21 @@ async function renderSelectList( selectList: SelectList, chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, - { mergeRatio, aliasMapSubscripts = false }: RenderSelectListOptions, + { + mergeRatio, + aliasMapSubscripts = false, + reservedNames = [], + }: RenderSelectListOptions, ) { if (typeof selectList === 'string') { return chSql`${{ UNSAFE_RAW_SQL: aliasMapSubscripts - ? await aliasStringSelectList(selectList, chartConfig, metadata) + ? await aliasStringSelectList( + selectList, + chartConfig, + metadata, + reservedNames, + ) : selectList, }}`; } @@ -1328,6 +1364,9 @@ async function renderSelect( : await renderSelectList(chartConfig.select, chartConfig, metadata, { mergeRatio: true, aliasMapSubscripts: true, + reservedNames: isIncludingGroupBy + ? selectListAliases(chartConfig.groupBy) + : [], }), isIncludingGroupBy && chartConfig.selectGroupBy !== false ? await renderSelectList(chartConfig.groupBy, chartConfig, metadata, { From e27c5a0818016dd60416441b350748a2e1cb364e Mon Sep 17 00:00:00 2001 From: Mohammed Shoaib Choudry Date: Wed, 23 Sep 2026 19:00:21 +0530 Subject: [PATCH 3/6] revert: drop the automatic Map subscript aliasing in renderChartConfig Per review, aliasing every rendered query has too wide a blast radius (saved searches, alerts, dashboards all go through renderChartConfig). The alias moves to the search page instead, where it is only added when a column is picked from the UI and stays visible and editable in the SELECT. --- .changeset/map-attribute-column-alias.md | 9 - .../src/__tests__/renderChartConfig.test.ts | 273 +----------------- .../src/core/renderChartConfig.ts | 212 +------------- 3 files changed, 8 insertions(+), 486 deletions(-) delete mode 100644 .changeset/map-attribute-column-alias.md diff --git a/.changeset/map-attribute-column-alias.md b/.changeset/map-attribute-column-alias.md deleted file mode 100644 index 300b1ac825..0000000000 --- a/.changeset/map-attribute-column-alias.md +++ /dev/null @@ -1,9 +0,0 @@ ---- -'@hyperdx/common-utils': patch -'@hyperdx/app': patch -'@hyperdx/api': patch ---- - -fix: label Map attribute columns in search results by their key - -Selecting `LogAttributes['service.name']` as a column labelled the results-table header, and everything else that reads the result column name such as exports, with ClickHouse's derived name `arrayElement(LogAttributes, 'service.name')`. Bare Map subscripts in the select are now aliased after their key, so the column reads `service.name`. A key that would shadow a table column or another selected name keeps the derived name, and the same key selected from two maps is prefixed with the map (`LogAttributes.http.method`). diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 560a05e583..b213722f69 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -1,15 +1,9 @@ -import { - chSql, - chSqlToAliasMap, - ColumnMeta, - parameterizedQueryToSql, -} from '@/clickhouse'; +import { chSql, ColumnMeta, parameterizedQueryToSql } from '@/clickhouse'; import { Metadata } from '@/core/metadata'; import { ChartConfigWithOptDateRangeEx, renderChartConfig, timeFilterExpr, - withMapSubscriptAliases, } from '@/core/renderChartConfig'; import { convertToCategoricalChartConfig } from '@/core/utils'; import { @@ -5146,21 +5140,6 @@ describe('renderChartConfig', () => { expect(sql).toContain('AS "errors__2"'); }); - it('escapes backslashes in the formula column name', async () => { - const generatedSql = await renderChartConfig( - { - ...baseEventFormulaConfig, - formulas: [{ expression: 'A / B', alias: String.raw`dir\name` }], - }, - mockMetadata, - querySettings, - ); - const sql = parameterizedQueryToSql(generatedSql); - // ClickHouse reads a double-quoted identifier like a string literal, so - // a lone backslash would escape the character after it. - expect(sql).toContain(String.raw`AS "dir\\name"`); - }); - it('escapes double quotes in the formula column name', async () => { const generatedSql = await renderChartConfig( { @@ -5232,254 +5211,4 @@ describe('renderChartConfig', () => { ); }); }); - - describe('Map subscript aliases in a string select (#3095)', () => { - describe('withMapSubscriptAliases', () => { - it('aliases a bare Map subscript after its key', () => { - expect( - withMapSubscriptAliases( - "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", - ), - ).toBe( - 'Timestamp, ' + - 'LogAttributes[\'constructor.index_builder_ac_res_id\'] AS "constructor.index_builder_ac_res_id", ' + - 'ResourceAttributes[\'service.name\'] AS "service.name"', - ); - }); - - it('keeps the derived name for a key containing a backslash', () => { - // Same round trip as a double quote: node-sql-parser hands the alias - // back with its escape still in place, so the app's alias map never - // matches the name ClickHouse reports for the column. - const select = String.raw`LogAttributes['dir\\name'], LogAttributes['other']`; - expect(withMapSubscriptAliases(select)).toBe( - String.raw`LogAttributes['dir\\name'], LogAttributes['other'] AS "other"`, - ); - }); - - it('keeps the derived name for a key containing a double quote', () => { - // node-sql-parser, which builds the app's alias map from the rendered - // SQL, rejects a doubled quote in an alias and drops every alias in - // the statement with it. - const select = `LogAttributes['say "hi"'], LogAttributes['other']`; - expect(withMapSubscriptAliases(select)).toBe( - `LogAttributes['say "hi"'], LogAttributes['other'] AS "other"`, - ); - }); - - it('unescapes the single-quote forms of the key literal', () => { - expect( - withMapSubscriptAliases( - String.raw`LogAttributes['it\'s'], SpanAttributes['won''t']`, - ), - ).toBe( - String.raw`LogAttributes['it\'s'] AS "it's", SpanAttributes['won''t'] AS "won't"`, - ); - }); - - it('leaves an already aliased subscript alone', () => { - const select = - "LogAttributes['a'] AS \"a\", LogAttributes['b'] as b, LogAttributes['c'] AS `c d`"; - expect(withMapSubscriptAliases(select)).toBe(select); - }); - - it('leaves plain columns and other expressions alone, byte for byte', () => { - const select = - "Timestamp,ServiceName , toString(LogAttributes['x']), LogAttributes['a']['b'], *"; - expect(withMapSubscriptAliases(select)).toBe(select); - }); - - it('handles a backtick-quoted map column', () => { - expect(withMapSubscriptAliases("`Log Attributes`['k']")).toBe( - '`Log Attributes`[\'k\'] AS "k"', - ); - }); - - it('prefixes the map when the same key is selected from two maps', () => { - expect( - withMapSubscriptAliases( - "LogAttributes['http.method'], SpanAttributes['http.method'], LogAttributes['other']", - ), - ).toBe( - 'LogAttributes[\'http.method\'] AS "LogAttributes.http.method", ' + - 'SpanAttributes[\'http.method\'] AS "SpanAttributes.http.method", ' + - 'LogAttributes[\'other\'] AS "other"', - ); - }); - - it('keeps the derived name when the key is a reserved column', () => { - // ClickHouse resolves an identifier to an alias before a column, so - // `AS "ServiceName"` would hijack a `WHERE ServiceName = ...`. - const select = "Timestamp, ResourceAttributes['ServiceName']"; - expect(withMapSubscriptAliases(select, ['ServiceName'])).toBe(select); - }); - - it('keeps the derived name when the key is referenced elsewhere in the list', () => { - for (const select of [ - "ServiceName, ResourceAttributes['ServiceName']", - "LogAttributes['x'] AS y, SpanAttributes['y']", - "LogAttributes['x'] AS \"y\", SpanAttributes['y']", - "concat(host, '-'), ResourceAttributes['host']", - ]) { - expect(withMapSubscriptAliases(select)).toBe(select); - } - }); - - it('still aliases a key that only appears inside a string literal', () => { - expect( - withMapSubscriptAliases( - "concat(ServiceName, 'host'), ResourceAttributes['host']", - ), - ).toBe( - "concat(ServiceName, 'host'), ResourceAttributes['host'] AS \"host\"", - ); - }); - }); - - describe('renderChartConfig', () => { - const searchConfig = ( - select: string, - overrides: Partial = {}, - ): ChartConfigWithOptDateRange => ({ - connection: 'test-connection', - from: { databaseName: 'default', tableName: 'otel_logs' }, - select, - where: '', - whereLanguage: 'sql', - timestampValueExpression: 'timestamp', - dateRange: [new Date('2025-01-01'), new Date('2025-01-02')], - ...overrides, - }); - - it('names the result columns after the selected keys', async () => { - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig( - "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", - ), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain( - 'SELECT Timestamp, ' + - 'LogAttributes[\'constructor.index_builder_ac_res_id\'] AS "constructor.index_builder_ac_res_id", ' + - 'ResourceAttributes[\'service.name\'] AS "service.name" ' + - 'FROM default.otel_logs', - ); - expect(mockMetadata.getColumns).toHaveBeenCalledWith({ - databaseName: 'default', - tableName: 'otel_logs', - connectionId: 'test-connection', - }); - }); - - it('does not alias a key that names a column of the source table', async () => { - // mockMetadata.getColumns lists a `value` column. - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig("LogAttributes['value'], LogAttributes['other']"), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain( - "SELECT LogAttributes['value'], LogAttributes['other'] AS \"other\" FROM", - ); - }); - - it('does not alias a key that names an alias CTE', async () => { - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig("ResourceAttributes['service']", { - with: [ - { name: 'service', sql: chSql`ServiceName`, isSubquery: false }, - ], - }), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain("SELECT ResourceAttributes['service'] FROM"); - }); - - it('never aliases a string groupBy', async () => { - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig('count()', { - groupBy: "ResourceAttributes['service.name']", - }), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain( - "SELECT count(),ResourceAttributes['service.name'] FROM", - ); - expect(sql).toContain("GROUP BY ResourceAttributes['service.name']"); - expect(sql).not.toContain('AS "service.name"'); - }); - - it('renders the list as written when the table columns are unknown', async () => { - mockMetadata.getColumns.mockRejectedValueOnce(new Error('no access')); - const select = "LogAttributes['k']"; - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig(select), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain(`SELECT ${select} FROM`); - }); - - it('does not alias a key that a group-by projection already names', async () => { - const sql = parameterizedQueryToSql( - await renderChartConfig( - searchConfig("LogAttributes['service'], LogAttributes['other']", { - groupBy: [ - { - aggCondition: '', - valueExpression: 'Region', - alias: 'service', - }, - ], - }), - mockMetadata, - querySettings, - ), - ); - expect(sql).toContain( - "SELECT LogAttributes['service'], LogAttributes['other'] AS \"other\"", - ); - expect(sql).toContain('Region AS "service"'); - }); - - it('renders aliases the app can map back to their expressions', async () => { - // The feature needs no app change only because chSqlToAliasMap - // recovers the expression behind each alias from the rendered SQL. - const query = await renderChartConfig( - searchConfig( - "Timestamp, LogAttributes['constructor.index_builder_ac_res_id'], ResourceAttributes['service.name']", - ), - mockMetadata, - querySettings, - ); - expect(chSqlToAliasMap(query)).toEqual({ - 'constructor.index_builder_ac_res_id': - "LogAttributes['constructor.index_builder_ac_res_id']", - 'service.name': "ResourceAttributes['service.name']", - }); - }); - - it('skips the column lookup when nothing needs an alias', async () => { - await renderChartConfig( - searchConfig('Timestamp, ServiceName, Body'), - mockMetadata, - querySettings, - ); - expect(mockMetadata.getColumns).not.toHaveBeenCalled(); - }); - }); - }); }); diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index fcc9292e39..d64f342ce6 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -169,122 +169,12 @@ const MULTI_SERIES_SORT_ALIAS_PREFIX = '__hdx_sort_'; /** * Render a user-facing output column name as a ClickHouse double-quoted * identifier. User aliases (and metric names, which flow into the default - * aliases) can contain double quotes and backslashes. ClickHouse reads a - * quoted identifier like a string literal, so `"` is doubled and `\` is - * escaped: `bad"name` becomes "bad""name" instead of terminating the - * identifier early, and `a\b` becomes "a\\b" instead of dropping to "ab". - * Escaping happens only at SQL-emission time — collision dedup and the meta - * column names consumers see keep the raw name. + * aliases) can contain double quotes; ClickHouse escapes them by doubling, + * so `bad"name` becomes "bad""name" instead of terminating the identifier + * early. Escaping happens only at SQL-emission time — collision dedup and + * the meta column names consumers see keep the raw name. */ -const quotedColumnName = (name: string) => - `"${name.replace(/\\/g, '\\\\').replace(/"/g, '""')}"`; - -// A bare Map subscript: a plain or backtick-quoted column followed by one -// single-quoted key and nothing else, e.g. `LogAttributes['service.name']`. -// Captures the column as written and the key still SQL-escaped. A subscript -// wrapped in a function, nested, or carrying an alias does not match. -// Stricter than parseKeyPath (metadata.ts), which slices on the first `['` -// and expects input already known to be a key path. -const BARE_MAP_SUBSCRIPT_RE = - /^(`(?:[^`]|``)+`|[A-Za-z_][A-Za-z0-9_]*)\s*\[\s*'((?:[^'\\]|\\.|'')*)'\s*\]$/; - -// Undo the escapes a single-quoted ClickHouse literal carries for `'` and -// `\` — the two the app emits when it builds a subscript from a row's key -// (mergePath). Other C-style escapes stay as typed. -const unescapeSqlStringLiteral = (literal: string) => - literal.replace(/\\(['\\])|''/g, (_, escaped?: string) => escaped ?? "'"); - -/** - * Identifier-like tokens of a select entry outside string literals: bare - * words plus the contents of backtick- and double-quoted identifiers. A - * superset of the names the entry binds (keywords and function names come - * along), which only makes the alias collision check more conservative. - */ -function identifierTokens(entry: string): string[] { - const tokens: string[] = []; - const re = - /'(?:[^'\\]|\\.|'')*'|`((?:[^`]|``)+)`|"((?:[^"]|"")+)"|[A-Za-z_][A-Za-z0-9_.]*/g; - for (const match of entry.matchAll(re)) { - if (match[1] != null) { - tokens.push(match[1].replace(/``/g, '`')); - } else if (match[2] != null) { - tokens.push(match[2].replace(/""/g, '"')); - } else if (!match[0].startsWith("'")) { - tokens.push(match[0]); - } - } - return tokens; -} - -/** - * Alias each bare Map subscript in a string select list after its key, so - * the result column is named `service.name` rather than ClickHouse's derived - * `arrayElement(ResourceAttributes, 'service.name')`. Entries that are plain - * columns, other expressions, or already aliased are left as written. - * - * ClickHouse resolves an identifier to a query alias before a table column, - * so an alias must never shadow a name the rest of the query can reference. - * A key is skipped when it is one of `reservedNames` (the source table's - * columns, alias CTEs) or appears as an identifier anywhere in the list (a - * selected column, an explicit alias, a function argument). The same key - * selected from two maps is aliased with the map as prefix - * (`LogAttributes.http.method`, `SpanAttributes.http.method`), since - * ClickHouse rejects two expressions under one alias; an entry whose alias - * is still taken keeps its derived name. - * - * A key containing a double quote or a backslash also keeps its derived - * name, because neither survives the round trip back to the key. The app's - * alias map (chSqlToAliasMap) parses the rendered statement with - * node-sql-parser, which rejects a doubled quote inside an alias — and an - * unparseable statement drops every alias in the query, not just that one — - * and leaves a backslash escape as written rather than unescaping it, so the - * recovered name never matches the column name ClickHouse reports. - * - * Returns the input untouched when nothing is aliased; otherwise the entries - * are re-joined with `, `. - */ -export function withMapSubscriptAliases( - select: string, - reservedNames: Iterable = [], -): string { - const entries = splitAndTrimWithBracket(select).map(entry => { - const match = entry.match(BARE_MAP_SUBSCRIPT_RE); - const key = match == null ? undefined : unescapeSqlStringLiteral(match[2]); - return match == null || key == null || /["\\]/.test(key) - ? { entry } - : { - entry, - subscript: { - column: unquoteIdentifier(match[1]).replace(/``/g, '`'), - key, - }, - }; - }); - if (!entries.some(e => e.subscript != null)) { - return select; - } - - const taken = new Set(reservedNames); - const keyCount = new Map(); - for (const { entry, subscript } of entries) { - for (const token of identifierTokens(entry)) taken.add(token); - if (subscript != null) { - keyCount.set(subscript.key, (keyCount.get(subscript.key) ?? 0) + 1); - } - } - - let aliased = false; - const out = entries.map(({ entry, subscript: s }) => { - if (s == null) return entry; - const alias = - (keyCount.get(s.key) ?? 0) > 1 ? `${s.column}.${s.key}` : s.key; - if (taken.has(alias)) return entry; - taken.add(alias); - aliased = true; - return `${entry} AS ${quotedColumnName(alias)}`; - }); - return aliased ? out.join(', ') : select; -} +const quotedColumnName = (name: string) => `"${name.replace(/"/g, '""')}"`; // Histogram translations bake the group-by dimensions into a single Array // column named GROUP_ALIAS instead of projecting them as individual columns @@ -836,100 +726,16 @@ type RenderSelectListOptions = { * otherwise render `divide(ServiceName, Region)` into the GROUP BY clause. */ mergeRatio: boolean; - /** - * Whether bare Map subscripts in a string list get an alias after their key - * (see withMapSubscriptAliases). - * - * True only for the chart's *select* list, where it names the output - * columns. A `groupBy` list must pass `false`: `GROUP BY x AS y` is a - * syntax error, and consumers such as the Kubernetes dashboard look - * group-by rows up by ClickHouse's derived names. - */ - aliasMapSubscripts?: boolean; - /** - * Extra names a Map-subscript alias must not take, on top of the source - * table's columns and the alias CTEs. Used for the aliases the structured - * group-by binds, which are not visible in the select list itself. - */ - reservedNames?: Iterable; }; -/** - * Output names a structured select list binds. A group-by list is projected - * into the same SELECT as the select list, so its aliases are names a - * Map-subscript alias must not reuse: ClickHouse rejects one alias standing - * for two different expressions. - */ -function selectListAliases(selectList: SelectList | undefined): string[] { - return typeof selectList === 'string' || selectList == null - ? [] - : selectList.flatMap(select => - select.alias != null && select.alias.trim() !== '' - ? [select.alias] - : [], - ); -} - -/** - * withMapSubscriptAliases over a chart's string select list, reserving the - * source table's columns and any alias CTE names. Without the column list (a - * CTE source, or DESCRIBE failing) the list renders as written: an alias that - * shadowed a column referenced in WHERE would silently change what the query - * filters on. - */ -async function aliasStringSelectList( - selectList: string, - chartConfig: BuilderChartConfigWithOptDateRangeEx, - metadata: Metadata, - reservedNames: Iterable, -): Promise { - const { from, with: withClauses } = chartConfig; - if ( - !from.databaseName || - hasSubqueryCte(withClauses) || - !splitAndTrimWithBracket(selectList).some(entry => - BARE_MAP_SUBSCRIPT_RE.test(entry), - ) - ) { - return selectList; - } - try { - const columns = await metadata.getColumns({ - databaseName: from.databaseName, - tableName: from.tableName, - connectionId: chartConfig.connection, - }); - return withMapSubscriptAliases(selectList, [ - ...columns.map(c => c.name), - ...(withClauses ?? []).map(w => w.name), - ...reservedNames, - ]); - } catch { - return selectList; - } -} - async function renderSelectList( selectList: SelectList, chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, - { - mergeRatio, - aliasMapSubscripts = false, - reservedNames = [], - }: RenderSelectListOptions, + { mergeRatio }: RenderSelectListOptions, ) { if (typeof selectList === 'string') { - return chSql`${{ - UNSAFE_RAW_SQL: aliasMapSubscripts - ? await aliasStringSelectList( - selectList, - chartConfig, - metadata, - reservedNames, - ) - : selectList, - }}`; + return chSql`${{ UNSAFE_RAW_SQL: selectList }}`; } // This metadata query is executed in an attempt tp optimize the selects by favoring materialized fields @@ -1370,10 +1176,6 @@ async function renderSelect( ) : await renderSelectList(chartConfig.select, chartConfig, metadata, { mergeRatio: true, - aliasMapSubscripts: true, - reservedNames: isIncludingGroupBy - ? selectListAliases(chartConfig.groupBy) - : [], }), isIncludingGroupBy && chartConfig.selectGroupBy !== false ? await renderSelectList(chartConfig.groupBy, chartConfig, metadata, { From a7a5fa9678627b63657fba5aa8faad956a1e9335 Mon Sep 17 00:00:00 2001 From: Mohammed Shoaib Choudry Date: Wed, 23 Sep 2026 19:16:34 +0530 Subject: [PATCH 4/6] fix: alias Map attribute columns when they are added from the search sidebars Per review: instead of rewriting every rendered query, add the alias on the search page at the moment a column is picked from the UI. Both "add column" actions (filters sidebar, row side panel) go through toggleColumn, which now writes a bare Map subscript as `Map['key'] AS "key"`. The alias is visible and editable in the SELECT, and hand-written SQL is untouched. The alias is skipped when it could change the query: a key named like a table column, a name already in the SELECT, or a key that would need escaping. The sidebar "is this column shown" checks now match a column with or without its alias, so the buttons keep reading "Remove" for an aliased column. displayedColumns itself is unchanged, since DBRowTable relies on it being the literal SELECT items. --- .changeset/map-attribute-column-alias.md | 14 ++++ packages/app/src/DBSearchPage.tsx | 17 ++++- packages/app/src/__tests__/utils.test.ts | 75 +++++++++++++++++++ .../app/src/components/DBRowJsonViewer.tsx | 4 +- .../src/components/DBSearchPageFilters.tsx | 7 +- .../DBSearchPageFilters/NestedFilterGroup.tsx | 4 +- packages/app/src/utils.ts | 56 ++++++++++++++ 7 files changed, 168 insertions(+), 9 deletions(-) create mode 100644 .changeset/map-attribute-column-alias.md diff --git a/.changeset/map-attribute-column-alias.md b/.changeset/map-attribute-column-alias.md new file mode 100644 index 0000000000..fa6b21687c --- /dev/null +++ b/.changeset/map-attribute-column-alias.md @@ -0,0 +1,14 @@ +--- +'@hyperdx/app': patch +--- + +fix: label Map attribute columns added from the search sidebars by their key + +Adding a Map attribute such as `ResourceAttributes['service.name']` as a column +from the filters sidebar or the row side panel now writes it into the SELECT as +`ResourceAttributes['service.name'] AS "service.name"`, so the results column +reads `service.name` instead of +`arrayElement(ResourceAttributes, 'service.name')`. The alias is visible and +editable in the SELECT, and queries typed by hand are not changed. A key named +like a table column, or one already used as a name in the SELECT, is added +without an alias. diff --git a/packages/app/src/DBSearchPage.tsx b/packages/app/src/DBSearchPage.tsx index 087a2a1063..5a436023b7 100644 --- a/packages/app/src/DBSearchPage.tsx +++ b/packages/app/src/DBSearchPage.tsx @@ -131,8 +131,10 @@ import { import { formatDurationMs, QUERY_LOCAL_STORAGE, + selectItemExpression, useLocalStorage, usePrevious, + withMapKeyAlias, } from '@/utils'; import ChartSQLPreview, { SQLPreview } from './components/ChartSQLPreview'; @@ -1737,13 +1739,20 @@ export function DBSearchPage() { const toggleColumn = useCallback( (column: string) => { - const newSelectArray = displayedColumns.includes(column) - ? displayedColumns.filter(s => s !== column) - : [...displayedColumns, column]; + // A column added from the UI can carry an alias, so match the expression too + const selected = displayedColumns.find( + s => s === column || selectItemExpression(s) === column, + ); + const newSelectArray = selected + ? displayedColumns.filter(s => s !== selected) + : [ + ...displayedColumns, + withMapKeyAlias(column, displayedColumns, knownColumns), + ]; setValue('select', newSelectArray.join(', ')); onSubmit(); }, - [displayedColumns, setValue, onSubmit], + [displayedColumns, knownColumns, setValue, onSubmit], ); const generateSearchUrl = useCallback( diff --git a/packages/app/src/__tests__/utils.test.ts b/packages/app/src/__tests__/utils.test.ts index 27f7e8f238..315d2b04a4 100644 --- a/packages/app/src/__tests__/utils.test.ts +++ b/packages/app/src/__tests__/utils.test.ts @@ -21,6 +21,7 @@ import { getAllMetricTables, getColorFromCSSToken, getMetricTableName, + isColumnInSelect, mapKeyBy, mergePath, orderByStringToSortingState, @@ -29,6 +30,7 @@ import { sortingStateToOrderByString, stripTrailingSlash, useQueryHistory, + withMapKeyAlias, } from '@/utils'; describe('formatAttributeClause', () => { @@ -1420,6 +1422,79 @@ describe('mergePath', () => { }); }); +describe('withMapKeyAlias', () => { + const noColumns = new Set(); + + it('labels a Map subscript with its key', () => { + expect( + withMapKeyAlias("ResourceAttributes['service.name']", [], noColumns), + ).toBe(`ResourceAttributes['service.name'] AS "service.name"`); + expect(withMapKeyAlias("`LogAttributes`['k8s.pod']", [], noColumns)).toBe( + '`LogAttributes`[\'k8s.pod\'] AS "k8s.pod"', + ); + }); + + it('unescapes the key the way mergePath escaped it', () => { + expect(withMapKeyAlias("LogAttributes['it\\'s']", [], noColumns)).toBe( + `LogAttributes['it\\'s'] AS "it's"`, + ); + }); + + it('leaves anything that is not a bare Map subscript alone', () => { + [ + 'Body', + 'lower(Body)', + "LogAttributes['a']['b']", + "JSONExtractString(Body, 'a')", + "LogAttributes['a'] AS a", + ].forEach(column => { + expect(withMapKeyAlias(column, [], noColumns)).toBe(column); + }); + }); + + it('keeps the derived name when the alias could change the query', () => { + // An alias shadows a column of the same name in WHERE and ORDER BY + expect( + withMapKeyAlias( + "ResourceAttributes['ServiceName']", + [], + new Set(['ServiceName']), + ), + ).toBe("ResourceAttributes['ServiceName']"); + // Two selected columns cannot share a name + expect( + withMapKeyAlias( + "LogAttributes['http.method']", + [`SpanAttributes['http.method'] AS "http.method"`], + noColumns, + ), + ).toBe("LogAttributes['http.method']"); + // Keys that would need escaping inside the quoted alias + expect(withMapKeyAlias(`LogAttributes['a"b']`, [], noColumns)).toBe( + `LogAttributes['a"b']`, + ); + }); +}); + +describe('isColumnInSelect', () => { + it('matches a column whether or not it was aliased', () => { + const select = [ + 'Timestamp', + `ResourceAttributes['service.name'] AS "service.name"`, + ]; + expect(isColumnInSelect(select, 'Timestamp')).toBe(true); + expect(isColumnInSelect(select, "ResourceAttributes['service.name']")).toBe( + true, + ); + expect(isColumnInSelect(select, 'Body')).toBe(false); + expect(isColumnInSelect(undefined, 'Body')).toBe(false); + }); + + it('does not mistake a cast for an alias', () => { + expect(isColumnInSelect(['CAST(x AS String)'], 'CAST(x')).toBe(false); + }); +}); + describe('getColorFromCSSToken', () => { afterEach(() => { jest.restoreAllMocks(); diff --git a/packages/app/src/components/DBRowJsonViewer.tsx b/packages/app/src/components/DBRowJsonViewer.tsx index e5c5b6faa4..f8e2ec2eaf 100644 --- a/packages/app/src/components/DBRowJsonViewer.tsx +++ b/packages/app/src/components/DBRowJsonViewer.tsx @@ -33,7 +33,7 @@ import HyperJson, { LineAction, } from '@/components/HyperJson'; import { useFormatTime } from '@/useFormatTime'; -import { mergePath } from '@/utils'; +import { isColumnInSelect, mergePath } from '@/utils'; import { CLIPBOARD_ERROR_MESSAGE, copyTextToClipboard, @@ -584,7 +584,7 @@ export function DBRowJsonViewer({ } } - const isIncluded = displayedColumns?.includes(columnFieldPath); + const isIncluded = isColumnInSelect(displayedColumns, columnFieldPath); actions.push({ key: 'toggle-column', label: isIncluded ? : , diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index 67a154aae7..20d1d9edb0 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -63,7 +63,7 @@ import { usePinnedFilters, } from '@/searchFilters'; import { useSource } from '@/source'; -import { useLocalStorage } from '@/utils'; +import { isColumnInSelect, useLocalStorage } from '@/utils'; import { FilterSettingsPanel } from './DBSearchPageFilters/FilterSettingsPopover'; import { useFetchFacets } from './DBSearchPageFilters/hooks'; @@ -1554,7 +1554,10 @@ const DBSearchPageFiltersComponent = ({ onColumnToggle={ onColumnToggle ? () => onColumnToggle(facetSqlKey) : undefined } - isColumnDisplayed={displayedColumns?.includes(facetSqlKey)} + isColumnDisplayed={isColumnInSelect( + displayedColumns, + facetSqlKey, + )} onLoadMore={loadMoreFacetsForKey} loadMoreLoading={loadMoreLoadingKeys.has(facet.key)} hasLoadedMore={extraFacetKeys.has(facet.key)} diff --git a/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx b/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx index 4145d8e917..89720f035d 100644 --- a/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx +++ b/packages/app/src/components/DBSearchPageFilters/NestedFilterGroup.tsx @@ -4,6 +4,7 @@ import { Accordion, Group, Text, Tooltip, UnstyledButton } from '@mantine/core'; import { useVirtualizer } from '@tanstack/react-virtual'; import { FilterGroup } from '@/components/DBSearchPageFilters'; +import { isColumnInSelect } from '@/utils'; import classes from '@styles/SearchPage.module.scss'; @@ -243,7 +244,8 @@ export const NestedFilterGroup = ({ onColumnToggle(child.sqlKey ?? child.key) : undefined } - isColumnDisplayed={displayedColumns?.includes( + isColumnDisplayed={isColumnInSelect( + displayedColumns, child.sqlKey ?? child.key, )} onLoadMore={() => onLoadMore(child.key)} diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index 6315d561b2..434fedd80e 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -1195,6 +1195,62 @@ export const mergePath = ( .join('')}`; }; +// A bare Map subscript as the UI builds it: `LogAttributes['key']` or `` `LogAttributes`['key'] `` +const MAP_SUBSCRIPT_REGEX = + /^(`[^`]+`|[A-Za-z_][A-Za-z0-9_]*)\['((?:[^'\\]|\\.)*)'\]$/; +// A trailing `AS alias` on a SELECT item, with the alias quoted or bare +const SELECT_ALIAS_REGEX = + /\s+AS\s+("(?:[^"\\]|\\.)*"|`[^`]*`|[A-Za-z_][A-Za-z0-9_]*)\s*$/i; + +/** + * The expression of a SELECT item, without its `AS alias` + */ +export const selectItemExpression = (item: string): string => + item.replace(SELECT_ALIAS_REGEX, '').trim(); + +/** + * Whether a column picked in the UI is in the SELECT list, aliased or not + */ +export const isColumnInSelect = ( + displayedColumns: string[] | undefined, + column: string, +): boolean => + displayedColumns?.some( + item => item === column || selectItemExpression(item) === column, + ) ?? false; + +/** + * Label a Map subscript picked in the UI with its key, so the results column + * reads `service.name` instead of `arrayElement(LogAttributes, 'service.name')`. + * The alias is left off when it could change the query: a key named like a + * table column (ClickHouse resolves an alias before a column), a name already + * in the SELECT list, or a key that would need escaping as an identifier. + */ +export const withMapKeyAlias = ( + column: string, + selectItems: string[], + tableColumns: Set, +): string => { + const match = MAP_SUBSCRIPT_REGEX.exec(column); + if (!match) { + return column; + } + const key = match[2].replace(/\\(.)/g, '$1'); + const selectedNames = selectItems.map(item => { + const alias = SELECT_ALIAS_REGEX.exec(item)?.[1]; + return alias ? alias.replace(/^["`]|["`]$/g, '') : item.trim(); + }); + if ( + key === '' || + /["\\]/.test(key) || + tableColumns.has(key) || + selectedNames.includes(key) + ) { + return column; + } + return `${column} AS "${key}"`; +}; + const _useTry = (fn: () => T): [null | Error | unknown, null | T] => { let output = null; let error = null; From cdc3047a28f1a10c95eee098f081fbe0437700e1 Mon Sep 17 00:00:00 2001 From: Mohammed Shoaib Choudry Date: Thu, 24 Sep 2026 12:47:01 +0530 Subject: [PATCH 5/6] fix: keep the sort valid when an aliased column is removed, and wait for columns Review round on the sidebar alias: - Removing a SELECT item whose alias the current sort is on (a table header sort on an aliased column orders by `"alias"`) now resets the sort to the source default, instead of resubmitting an ORDER BY that no longer resolves. Also covers a hand-typed alias removed from the header. - No alias is added while the table's columns are unknown (still loading or failed to load), so a clash with a column name cannot slip through. - Aliases are unquoted with the shared unquoteIdentifier, and each skip rule is commented. --- packages/app/src/DBSearchPage.tsx | 23 ++++++++++- packages/app/src/__tests__/utils.test.ts | 42 +++++++++++++++++++- packages/app/src/utils.ts | 50 ++++++++++++++++++++---- 3 files changed, 104 insertions(+), 11 deletions(-) diff --git a/packages/app/src/DBSearchPage.tsx b/packages/app/src/DBSearchPage.tsx index 5a436023b7..732faa411f 100644 --- a/packages/app/src/DBSearchPage.tsx +++ b/packages/app/src/DBSearchPage.tsx @@ -130,6 +130,7 @@ import { } from '@/timeQuery'; import { formatDurationMs, + orderByAfterRemovingSelectItem, QUERY_LOCAL_STORAGE, selectItemExpression, useLocalStorage, @@ -1106,7 +1107,7 @@ export function DBSearchPage() { [sources, lastSelectedSourceId], ); - const { control, setValue, reset, handleSubmit, formState } = + const { control, setValue, getValues, reset, handleSubmit, formState } = useForm({ values: { select: searchedConfig.select || '', @@ -1750,9 +1751,27 @@ export function DBSearchPage() { withMapKeyAlias(column, displayedColumns, knownColumns), ]; setValue('select', newSelectArray.join(', ')); + if (selected) { + const orderBy = getValues('orderBy') ?? ''; + const nextOrderBy = orderByAfterRemovingSelectItem( + selected, + orderBy, + defaultSearchConfig.orderBy ?? '', + ); + if (nextOrderBy !== orderBy) { + setValue('orderBy', nextOrderBy); + } + } onSubmit(); }, - [displayedColumns, knownColumns, setValue, onSubmit], + [ + displayedColumns, + knownColumns, + setValue, + getValues, + defaultSearchConfig.orderBy, + onSubmit, + ], ); const generateSearchUrl = useCallback( diff --git a/packages/app/src/__tests__/utils.test.ts b/packages/app/src/__tests__/utils.test.ts index 315d2b04a4..e18af71cf5 100644 --- a/packages/app/src/__tests__/utils.test.ts +++ b/packages/app/src/__tests__/utils.test.ts @@ -24,6 +24,7 @@ import { isColumnInSelect, mapKeyBy, mergePath, + orderByAfterRemovingSelectItem, orderByStringToSortingState, parseTimestampToMs, resolveConditionalColor, @@ -1423,7 +1424,7 @@ describe('mergePath', () => { }); describe('withMapKeyAlias', () => { - const noColumns = new Set(); + const noColumns = new Set(['Timestamp', 'Body']); it('labels a Map subscript with its key', () => { expect( @@ -1473,6 +1474,10 @@ describe('withMapKeyAlias', () => { expect(withMapKeyAlias(`LogAttributes['a"b']`, [], noColumns)).toBe( `LogAttributes['a"b']`, ); + // Table columns not loaded yet: a clash with one cannot be ruled out + expect( + withMapKeyAlias("ResourceAttributes['ServiceName']", [], new Set()), + ).toBe("ResourceAttributes['ServiceName']"); }); }); @@ -1490,11 +1495,46 @@ describe('isColumnInSelect', () => { expect(isColumnInSelect(undefined, 'Body')).toBe(false); }); + it('matches a hand-typed alias too', () => { + expect(isColumnInSelect(['Body AS b'], 'Body')).toBe(true); + expect(isColumnInSelect(['lower(Body) AS `b`'], 'lower(Body)')).toBe(true); + }); + it('does not mistake a cast for an alias', () => { expect(isColumnInSelect(['CAST(x AS String)'], 'CAST(x')).toBe(false); }); }); +describe('orderByAfterRemovingSelectItem', () => { + const item = `ResourceAttributes['service.name'] AS "service.name"`; + + it('drops a sort on the alias of the removed column', () => { + expect( + orderByAfterRemovingSelectItem( + item, + '"service.name" DESC', + 'Timestamp DESC', + ), + ).toBe('Timestamp DESC'); + expect(orderByAfterRemovingSelectItem(item, '"service.name" ASC', '')).toBe( + '', + ); + }); + + it('keeps any other sort', () => { + expect( + orderByAfterRemovingSelectItem(item, 'Timestamp ASC', 'Timestamp DESC'), + ).toBe('Timestamp ASC'); + expect(orderByAfterRemovingSelectItem(item, '"http.method" DESC', '')).toBe( + '"http.method" DESC', + ); + // A column without an alias sorts by its own expression, which is not affected + expect( + orderByAfterRemovingSelectItem('Body', 'Body DESC', 'Timestamp DESC'), + ).toBe('Body DESC'); + }); +}); + describe('getColorFromCSSToken', () => { afterEach(() => { jest.restoreAllMocks(); diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index 434fedd80e..ff43e6b838 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -4,7 +4,10 @@ import { formatDistanceToNowStrict } from 'date-fns'; import numbro from 'numbro'; import type { SetStateAction } from 'react'; import TimestampNano from 'timestamp-nano'; -import { TableConnection } from '@hyperdx/common-utils/dist/core/metadata'; +import { + TableConnection, + unquoteIdentifier, +} from '@hyperdx/common-utils/dist/core/metadata'; import { CATEGORICAL_PALETTE_TOKENS, ChartPaletteToken, @@ -1208,6 +1211,14 @@ const SELECT_ALIAS_REGEX = export const selectItemExpression = (item: string): string => item.replace(SELECT_ALIAS_REGEX, '').trim(); +/** + * The alias of a SELECT item, unquoted, or undefined if it has none + */ +export const selectItemAlias = (item: string): string | undefined => { + const alias = SELECT_ALIAS_REGEX.exec(item)?.[1]; + return alias == null ? undefined : unquoteIdentifier(alias); +}; + /** * Whether a column picked in the UI is in the SELECT list, aliased or not */ @@ -1222,9 +1233,7 @@ export const isColumnInSelect = ( /** * Label a Map subscript picked in the UI with its key, so the results column * reads `service.name` instead of `arrayElement(LogAttributes, 'service.name')`. - * The alias is left off when it could change the query: a key named like a - * table column (ClickHouse resolves an alias before a column), a name already - * in the SELECT list, or a key that would need escaping as an identifier. + * The alias is left off whenever it could change the query. */ export const withMapKeyAlias = ( column: string, @@ -1235,15 +1244,21 @@ export const withMapKeyAlias = ( if (!match) { return column; } + // Inverse of escapeSqlSingleQuoted, which mergePath uses to build the subscript const key = match[2].replace(/\\(.)/g, '$1'); - const selectedNames = selectItems.map(item => { - const alias = SELECT_ALIAS_REGEX.exec(item)?.[1]; - return alias ? alias.replace(/^["`]|["`]$/g, '') : item.trim(); - }); + const selectedNames = selectItems.map( + item => selectItemAlias(item) ?? item.trim(), + ); if ( + // Nothing to label key === '' || + // Would need escaping inside the quoted alias /["\\]/.test(key) || + // Table columns not loaded yet, so a clash with one cannot be ruled out + tableColumns.size === 0 || + // ClickHouse resolves an alias before a column of the same name tableColumns.has(key) || + // Two selected columns cannot share a name selectedNames.includes(key) ) { return column; @@ -1251,6 +1266,25 @@ export const withMapKeyAlias = ( return `${column} AS "${key}"`; }; +/** + * The sort to keep once a SELECT item is removed. Sorting on an aliased column + * from the table header orders by the alias itself (`"service.name" DESC`), + * which stops resolving once the item is gone, so that falls back to the + * default sort. + */ +export const orderByAfterRemovingSelectItem = ( + item: string, + orderBy: string, + defaultOrderBy: string, +): string => { + const alias = selectItemAlias(item); + if (alias == null) { + return orderBy; + } + const sortedBy = orderBy.trim().replace(/\s+(ASC|DESC)$/i, ''); + return sortedBy === `"${alias}"` ? defaultOrderBy : orderBy; +}; + const _useTry = (fn: () => T): [null | Error | unknown, null | T] => { let output = null; let error = null; From eb8cc31d1a329529eb89e7122a6d5b960234a3e1 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Mon, 28 Sep 2026 12:11:50 -0400 Subject: [PATCH 6/6] fix: Fix knip --- packages/app/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/app/src/utils.ts b/packages/app/src/utils.ts index 26d1662135..e9644d36e8 100644 --- a/packages/app/src/utils.ts +++ b/packages/app/src/utils.ts @@ -1220,7 +1220,7 @@ export const selectItemExpression = (item: string): string => /** * The alias of a SELECT item, unquoted, or undefined if it has none */ -export const selectItemAlias = (item: string): string | undefined => { +const selectItemAlias = (item: string): string | undefined => { const alias = SELECT_ALIAS_REGEX.exec(item)?.[1]; return alias == null ? undefined : unquoteIdentifier(alias); };