From c6523ed37d64408680ac99f068d03a483a272006 Mon Sep 17 00:00:00 2001 From: Karl Power Date: Thu, 11 Jun 2026 16:44:28 +0200 Subject: [PATCH] fix: handle ClickHouse parametric aggregate functions in alias extraction --- .../fix-alias-extraction-parametric-agg.md | 5 + .../src/__tests__/clickhouse.test.ts | 67 ++++++++++ .../common-utils/src/__tests__/utils.test.ts | 90 +++++++++++++ packages/common-utils/src/clickhouse/index.ts | 28 +++- packages/common-utils/src/core/utils.ts | 124 ++++++++++++++++++ 5 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 .changeset/fix-alias-extraction-parametric-agg.md diff --git a/.changeset/fix-alias-extraction-parametric-agg.md b/.changeset/fix-alias-extraction-parametric-agg.md new file mode 100644 index 0000000000..593df9745b --- /dev/null +++ b/.changeset/fix-alias-extraction-parametric-agg.md @@ -0,0 +1,5 @@ +--- +"@hyperdx/common-utils": patch +--- + +fix: handle ClickHouse parametric aggregate functions in alias extraction. `chSqlToAliasMap` now parses selects containing the double-paren `func(params)(args)` form (e.g. `groupUniqArray(20)(col)`, `quantile(0.9)(col)`) instead of throwing and logging "Error parsing alias map" — which surfaced on every value-autocomplete fetch. diff --git a/packages/common-utils/src/__tests__/clickhouse.test.ts b/packages/common-utils/src/__tests__/clickhouse.test.ts index 336f9f9a9e..348b7780a9 100644 --- a/packages/common-utils/src/__tests__/clickhouse.test.ts +++ b/packages/common-utils/src/__tests__/clickhouse.test.ts @@ -223,6 +223,73 @@ describe('chSqlToAliasMap - alias unit test', () => { }; expect(res).toEqual(aliasMap); }); + + it('Parametric aggregate functions (getKeyValues repro)', () => { + // node-sql-parser can't parse the double-paren `func(params)(args)` form, so + // these were silently dropped and spammed "Error parsing alias map". The + // aliases must now resolve to the full original expression. + const chSqlInput: ChSql = { + sql: 'SELECT groupUniqArray(20)(param0) AS param0, groupUniqArray(20)(param1) AS param1 FROM t', + params: {}, + }; + const res = chSqlToAliasMap(chSqlInput); + expect(res).toEqual({ + param0: 'groupUniqArray(20)(param0)', + param1: 'groupUniqArray(20)(param1)', + }); + }); + + it('Parametric aggregate mixed with normal and bracketed aliases', () => { + const chSqlInput: ChSql = { + sql: "SELECT Timestamp as ts, groupUniqArray(20)(ServiceName) AS svc, ResourceAttributes['x'] as rx FROM t", + params: {}, + }; + const res = chSqlToAliasMap(chSqlInput); + expect(res).toEqual({ + ts: 'Timestamp', + svc: 'groupUniqArray(20)(ServiceName)', + rx: "ResourceAttributes['x']", + }); + }); + + it('Parametric aggregate over a dotted/JSON column', () => { + // Parametric replacement runs before JSON replacement, so the dotted inner + // expression must survive intact inside the restored alias value. + const chSqlInput: ChSql = { + sql: 'SELECT groupUniqArray(20)(ResourceAttributes.service.name) AS svc FROM t', + params: {}, + }; + const res = chSqlToAliasMap(chSqlInput); + expect(res).toEqual({ + svc: 'groupUniqArray(20)(ResourceAttributes.service.name)', + }); + }); + + it('Parametric aggregate with numeric params', () => { + const chSqlInput: ChSql = { + sql: 'SELECT quantiles(0.5, 0.9)(Duration) AS q FROM t', + params: {}, + }; + const res = chSqlToAliasMap(chSqlInput); + expect(res).toEqual({ + q: 'quantiles(0.5, 0.9)(Duration)', + }); + }); + + it('Single-paren aggregates are not treated as parametric', () => { + // count() has a single group and must parse natively; the parametric + // aggregates alongside it must still resolve. + const chSqlInput: ChSql = { + sql: 'SELECT count() AS c, groupUniqArray(20)(a) AS ga, quantile(0.9)(b) AS p90 FROM t', + params: {}, + }; + const res = chSqlToAliasMap(chSqlInput); + expect(res).toEqual({ + c: 'count()', + ga: 'groupUniqArray(20)(a)', + p90: 'quantile(0.9)(b)', + }); + }); }); describe('computeRatio', () => { diff --git a/packages/common-utils/src/__tests__/utils.test.ts b/packages/common-utils/src/__tests__/utils.test.ts index d70aa40882..c162b1a1d1 100644 --- a/packages/common-utils/src/__tests__/utils.test.ts +++ b/packages/common-utils/src/__tests__/utils.test.ts @@ -31,6 +31,7 @@ import { parseToStartOfFunction, pickBucketTimestampColumn, replaceJsonExpressions, + replaceParametricAggregates, splitAndTrimCSV, splitAndTrimWithBracket, } from '../core/utils'; @@ -1885,6 +1886,95 @@ describe('utils', () => { }); }); + describe('replaceParametricAggregates', () => { + it('should leave input without parametric aggregates unchanged', () => { + const sql = 'SELECT a, count() AS c, sum(if(x, 1, 0)) AS s FROM table'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map(), + sqlWithReplacements: sql, + }); + }); + + it('should replace a single parametric aggregate', () => { + const sql = 'SELECT groupUniqArray(20)(col) AS param0 FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map([ + ['__hdx_paramagg_replacement_0', 'groupUniqArray(20)(col)'], + ]), + sqlWithReplacements: + 'SELECT __hdx_paramagg_replacement_0 AS param0 FROM t', + }); + }); + + it('should replace multiple parametric aggregates', () => { + const sql = + 'SELECT groupUniqArray(20)(a) AS p0, quantile(0.9)(b) AS p1 FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map([ + ['__hdx_paramagg_replacement_0', 'groupUniqArray(20)(a)'], + ['__hdx_paramagg_replacement_1', 'quantile(0.9)(b)'], + ]), + sqlWithReplacements: + 'SELECT __hdx_paramagg_replacement_0 AS p0, __hdx_paramagg_replacement_1 AS p1 FROM t', + }); + }); + + it('should balance nested parens within the argument groups', () => { + const sql = 'SELECT quantiles(0.5, 0.9)(toFloat64(Duration)) AS q FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map([ + [ + '__hdx_paramagg_replacement_0', + 'quantiles(0.5, 0.9)(toFloat64(Duration))', + ], + ]), + sqlWithReplacements: 'SELECT __hdx_paramagg_replacement_0 AS q FROM t', + }); + }); + + it('should not count parens inside quoted strings', () => { + const sql = "SELECT col, ')(' AS literal FROM t"; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map(), + sqlWithReplacements: sql, + }); + }); + + it('should tolerate whitespace between the identifier and groups', () => { + const sql = 'SELECT groupUniqArray (20) (col) AS p0 FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map([ + ['__hdx_paramagg_replacement_0', 'groupUniqArray (20) (col)'], + ]), + sqlWithReplacements: 'SELECT __hdx_paramagg_replacement_0 AS p0 FROM t', + }); + }); + + it('should not match an unbalanced double-paren form', () => { + const sql = 'SELECT f(a)(b FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map(), + sqlWithReplacements: sql, + }); + }); + + it('should not match a single-paren call', () => { + const sql = 'SELECT count() AS c, max(value) AS m FROM t'; + const actual = replaceParametricAggregates(sql); + expect(actual).toEqual({ + replacements: new Map(), + sqlWithReplacements: sql, + }); + }); + }); + describe('parseToStartOfFunction', () => { it.each([ { diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index dfb28d0a6e..b2416ed02b 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -21,6 +21,7 @@ import { extractSettingsClauseFromEnd, hashCode, replaceJsonExpressions, + replaceParametricAggregates, splitAndTrimWithBracket, } from '@/core/utils'; import { isBuilderChartConfig } from '@/guards'; @@ -833,9 +834,18 @@ export function chSqlToAliasMap( // Remove the SETTINGS clause because `SQLParser` doesn't understand it. const [sqlWithoutSettingsClause] = extractSettingsClauseFromEnd(sql); + // Replace ClickHouse parametric aggregates (eg. groupUniqArray(20)(col)) with + // tokens first: node-sql-parser can't parse the double-paren form, and doing + // this before replaceJsonExpressions keeps any dotted/JSON args inside the + // saved span intact (the dotless token is inert to JSON detection). + const { + sqlWithReplacements: sqlWithoutParametricAggregates, + replacements: parametricAggregateReplacements, + } = replaceParametricAggregates(sqlWithoutSettingsClause); + // Replace JSON expressions with replacement tokens so that node-sql-parser can parse the SQL const { sqlWithReplacements, replacements: jsonReplacementsToExpressions } = - replaceJsonExpressions(sqlWithoutSettingsClause); + replaceJsonExpressions(sqlWithoutParametricAggregates); const parser = new SQLParser.Parser(); // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- astify returns union type @@ -866,11 +876,17 @@ export function chSqlToAliasMap( }); } - // Replace the JSON replacement tokens with the original JSON expressions - for (const [alias, aliasExpression] of Object.entries(aliasMap)) { - for (const [replacement, original] of jsonReplacementsToExpressions) { - if (aliasExpression.includes(replacement)) { - aliasMap[alias] = aliasExpression.replaceAll(replacement, original); + // Restore the JSON and parametric-aggregate replacement tokens to their + // original expressions. Re-read aliasMap[alias] each pass so multiple tokens + // in one value compound correctly. + const allReplacements = new Map([ + ...jsonReplacementsToExpressions, + ...parametricAggregateReplacements, + ]); + for (const alias of Object.keys(aliasMap)) { + for (const [replacement, original] of allReplacements) { + if (aliasMap[alias].includes(replacement)) { + aliasMap[alias] = aliasMap[alias].replaceAll(replacement, original); } } } diff --git a/packages/common-utils/src/core/utils.ts b/packages/common-utils/src/core/utils.ts index 2095771aca..6cadc33fb2 100644 --- a/packages/common-utils/src/core/utils.ts +++ b/packages/common-utils/src/core/utils.ts @@ -344,6 +344,130 @@ export function replaceJsonExpressions(sql: string) { return { sqlWithReplacements, replacements }; } +/** + * Replaces ClickHouse parametric aggregate function calls — the double-paren + * `func(params)(args)` form (eg. `groupUniqArray(20)(col)`, `quantile(0.9)(col)`) + * — with placeholders like `__hdx_paramagg_replacement_0`. + */ +export function replaceParametricAggregates(sql: string) { + const replacements = new Map(); + + // Returns the index just past the matching ')' for an opening '(' at + // `openIndex`, or null if the parens are unbalanced. Quoted regions are + // skipped so parens inside literals/identifiers don't affect the depth. + const matchBalancedParens = (openIndex: number): number | null => { + let depth = 0; + let isInSingleQuote = false; + let isInDoubleQuote = false; + let isInBacktick = false; + for (let i = openIndex; i < sql.length; i++) { + const c = sql.charAt(i); + if (c === "'" && !isInDoubleQuote && !isInBacktick) { + isInSingleQuote = !isInSingleQuote; + } else if (c === '"' && !isInSingleQuote && !isInBacktick) { + isInDoubleQuote = !isInDoubleQuote; + } else if (c === '`' && !isInSingleQuote && !isInDoubleQuote) { + isInBacktick = !isInBacktick; + } else if (!isInSingleQuote && !isInDoubleQuote && !isInBacktick) { + if (c === '(') { + depth++; + } else if (c === ')') { + depth--; + if (depth === 0) { + return i + 1; + } + } + } + } + return null; + }; + + const skipWhitespace = (index: number): number => { + let i = index; + while (i < sql.length && /\s/.test(sql.charAt(i))) { + i++; + } + return i; + }; + + // Identifier classes mirror node-sql-parser's grammar: [A-Za-z_一-龥]. + const isIdentifierStart = (c: string) => /[A-Za-z_一-龥]/.test(c); + const isIdentifierChar = (c: string) => /[A-Za-z0-9_一-龥]/.test(c); + + let result = ''; + let counter = 0; + let i = 0; + let isInSingleQuote = false; + let isInDoubleQuote = false; + let isInBacktick = false; + + while (i < sql.length) { + const c = sql.charAt(i); + + // Copy quoted regions through verbatim. + if (c === "'" && !isInDoubleQuote && !isInBacktick) { + isInSingleQuote = !isInSingleQuote; + result += c; + i++; + continue; + } + if (c === '"' && !isInSingleQuote && !isInBacktick) { + isInDoubleQuote = !isInDoubleQuote; + result += c; + i++; + continue; + } + if (c === '`' && !isInSingleQuote && !isInDoubleQuote) { + isInBacktick = !isInBacktick; + result += c; + i++; + continue; + } + if (isInSingleQuote || isInDoubleQuote || isInBacktick) { + result += c; + i++; + continue; + } + + if (isIdentifierStart(c)) { + // Consume the identifier. + let idEnd = i + 1; + while (idEnd < sql.length && isIdentifierChar(sql.charAt(idEnd))) { + idEnd++; + } + + const afterId = skipWhitespace(idEnd); + if (sql.charAt(afterId) === '(') { + const group1End = matchBalancedParens(afterId); + if (group1End != null) { + const afterGroup1 = skipWhitespace(group1End); + if (sql.charAt(afterGroup1) === '(') { + const group2End = matchBalancedParens(afterGroup1); + if (group2End != null) { + const span = sql.slice(i, group2End); + const replacement = `__hdx_paramagg_replacement_${counter++}`; + replacements.set(replacement, span); + result += replacement; + i = group2End; + continue; + } + } + } + } + + // Not a parametric aggregate: copy the identifier as-is. + result += sql.slice(i, idEnd); + i = idEnd; + continue; + } + + result += c; + i++; + } + + return { sqlWithReplacements: result, replacements }; +} + /** * To best support Pre-aggregation in Materialized Views, any new * granularities should be multiples of all smaller granularities.