diff --git a/.changeset/lucene-unknown-field-falls-through.md b/.changeset/lucene-unknown-field-falls-through.md new file mode 100644 index 0000000000..25afae6bf3 --- /dev/null +++ b/.changeset/lucene-unknown-field-falls-through.md @@ -0,0 +1,7 @@ +--- +"@hyperdx/common-utils": patch +--- + +fix: unknown Lucene field no longer falls through as a raw SQL identifier + +A Lucene field that doesn't resolve to a real column is now gated on the set of known SELECT aliases — a known alias renders as a bare identifier (ClickHouse resolves SELECT aliases in WHERE) while anything genuinely unknown renders the no-match predicate `(1 = 0)` instead of being injected as raw SQL that ClickHouse rejects with "Unknown identifier" (which previously killed both the histogram and the row table at once). As part of this, `chSqlToAliasMap` now parses selects containing ClickHouse parametric aggregate functions — 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" 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__/queryParser.test.ts b/packages/common-utils/src/__tests__/queryParser.test.ts index b6b0594914..b2f2eefa0e 100644 --- a/packages/common-utils/src/__tests__/queryParser.test.ts +++ b/packages/common-utils/src/__tests__/queryParser.test.ts @@ -35,6 +35,8 @@ describe('CustomSchemaSQLSerializerV2 - json', () => { return { name: 'SeverityNumber', type: 'UInt8' }; } else if (column === 'foo') { return { name: 'foo', type: 'String' }; + } else if (column === 'bar') { + return { name: 'bar', type: 'String' }; } else if (column === 'MaterializedExample') { return { name: 'MaterializedExample', type: 'String' }; } else { @@ -494,6 +496,49 @@ describe('CustomSchemaSQLSerializerV2 - json', () => { expect(actualSql).toBe(expectedSql); }); + describe('CustomSchemaSQLSerializerV2: select alias resolution', () => { + // ClickHouse resolves SELECT aliases in WHERE, so a Lucene field matching a + // known alias must resolve to a bare identifier. A field that is neither a + // column nor an alias is genuinely unknown and must not be injected as raw + // SQL (which ClickHouse would reject as an unknown identifier). + const aliasSerializer = new CustomSchemaSQLSerializerV2({ + metadata, + databaseName, + tableName, + connectionId, + implicitColumnExpression: 'Body', + selectAliases: new Set(['Content']), + }); + + it('resolves a known select alias as a column reference', async () => { + const res = await aliasSerializer.getColumnForField('Content', {}); + expect(res).toEqual( + expect.objectContaining({ found: true, column: 'Content' }), + ); + }); + + it('renders a select alias reference as a bare identifier', async () => { + const sql = await new SearchQueryBuilder( + 'Content:foo', + aliasSerializer, + ).build(); + expect(sql).toBe("((Content ILIKE '%foo%'))"); + }); + + it('returns not-found for an unknown, non-alias field', async () => { + const res = await serializer.getColumnForField('NotAColumn', {}); + expect(res).toEqual(expect.objectContaining({ found: false })); + }); + + it('renders an unknown, non-alias field as the no-match predicate', async () => { + const sql = await new SearchQueryBuilder( + 'NotAColumn:foo', + serializer, + ).build(); + expect(sql).toBe('((1 = 0))'); + }); + }); + describe('CustomSchemaSQLSerializerV2: implicit column falls back to bodyExpression', () => { // Symmetric counterpart to the one-way fallback in // `getEventBody` (packages/app/src/source.ts): a log source admin who diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 35a198d7ea..8a34a9e0e1 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -31,6 +31,7 @@ describe('renderChartConfig', () => { { name: 'value', type: 'Float64' }, { name: 'TraceId', type: 'String' }, { name: 'ServiceName', type: 'String' }, + { name: 'severity', type: 'String' }, ]; mockMetadata = { getColumns: jest.fn().mockResolvedValue([ @@ -2324,6 +2325,430 @@ describe('renderChartConfig', () => { }); }); + describe('SELECT alias references in a Lucene WHERE', () => { + // ClickHouse resolves SELECT aliases in WHERE. A Lucene WHERE that + // references a SELECT alias (here `Body AS Content`) must resolve the alias + // to a bare identifier rather than rendering the no-match predicate. + it('resolves a Lucene WHERE reference to a SELECT alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: 'Content:"swagger"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + }); + + it('renders an unknown, non-alias field in a Lucene WHERE as the no-match predicate', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: 'NotAColumn:"swagger"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + expect(actual).not.toContain('NotAColumn'); + }); + + // String-form select lists (e.g. a source's defaultTableSelectExpression) + // declare aliases as raw SQL. These are parsed to recover the aliases so + // alias references in a Lucene WHERE still resolve. + it('resolves a Lucene WHERE reference to an alias in a string-form select', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: 'Body AS Content', + where: 'Content:"swagger"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + }); + + it('resolves an alias from a realistic default-view string select', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: 'Timestamp, ServiceName as service, Body', + where: 'service:"prod"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("service = 'prod'"); + }); + + it('renders an unknown field as the no-match predicate when a string select declares no matching alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: 'Timestamp, Body', + where: 'NotAColumn:"x"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + expect(actual).not.toContain('NotAColumn'); + }); + + // Combined-fix regression guard. Extracting the alias from a string-form + // select that contains a ClickHouse parametric aggregate (the double-paren + // `func(params)(args)` form) requires chSqlToAliasMap to parse that form + // without throwing — the parser gap that forced the original revert of the + // unknown-field fix. With both fixes in place, the alias is collected and a + // Lucene WHERE referencing it resolves to a bare identifier rather than the + // no-match predicate. + it('resolves a Lucene WHERE reference to an alias on a parametric aggregate in a string select', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: 'quantile(0.9)(value) AS p90, Body', + where: 'p90:"5"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("p90 = '5'"); + expect(actual).not.toContain('(1 = 0)'); + }); + + // SAVED_SEARCH alerts select count() while injecting the saved search's + // select aliases as expression-form WITH clauses (isSubquery: false). A + // Lucene WHERE referencing such an alias must resolve to the bare + // identifier, not the no-match predicate. + it('resolves a Lucene WHERE reference to an expression-form WITH alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + with: [ + { + name: 'Content', + sql: chSql`toString(Body)`, + isSubquery: false, + }, + ], + select: [{ aggFn: 'count', valueExpression: '' }], + where: 'Content:"swagger"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + expect(actual).not.toContain('(1 = 0)'); + }); + + // Subquery CTEs (isSubquery: true) name a table-like source, not a column, + // so an alias reference to one in a Lucene WHERE is still unknown and must + // render the no-match predicate. + it('does not treat a subquery WITH alias as a column reference', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + with: [ + { + name: 'sub', + sql: chSql`SELECT 1`, + isSubquery: true, + }, + ], + select: [{ aggFn: 'count', valueExpression: '' }], + where: 'sub:"x"', + whereLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + }); + }); + + describe('SELECT alias references in other Lucene call sites', () => { + // The main WHERE clause is covered above; these exercise the remaining + // Lucene-capable call sites in renderChartConfig (filters array, + // per-aggregate conditions, and HAVING), all of which must resolve SELECT + // aliases identically to the main WHERE. + + it('resolves a Lucene filter reference to a SELECT alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: '', + filters: [{ type: 'lucene', condition: 'Content:"swagger"' }], + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + }); + + it('renders an unknown, non-alias field in a Lucene filter as the no-match predicate', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: '', + filters: [{ type: 'lucene', condition: 'NotAColumn:"swagger"' }], + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + expect(actual).not.toContain('NotAColumn'); + }); + + // A per-aggregate Lucene condition is pushed into the WHERE clause (when + // every select has one) and also rendered as the aggregate's `...If(...)` + // predicate. Both paths must resolve the alias. The alias here is an + // expression-form WITH clause to avoid a select referencing its own alias. + it('resolves a Lucene per-aggregate condition to a SELECT alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + with: [ + { + name: 'Content', + sql: chSql`toString(Body)`, + isSubquery: false, + }, + ], + select: [ + { + aggFn: 'count', + valueExpression: '', + aggCondition: 'Content:"swagger"', + aggConditionLanguage: 'lucene', + }, + ], + where: '', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + expect(actual).not.toContain('(1 = 0)'); + }); + + it('renders an unknown per-aggregate Lucene field as the no-match predicate', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [ + { + aggFn: 'count', + valueExpression: '', + aggCondition: 'NotAColumn:"swagger"', + aggConditionLanguage: 'lucene', + }, + ], + where: '', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + expect(actual).not.toContain('NotAColumn'); + }); + + // A Lucene value expression (aggFn omitted, valueExpressionLanguage: lucene) + // is rendered as a SELECT column expression and must resolve aliases too. + it('resolves a Lucene value expression reference to a SELECT alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + with: [ + { + name: 'Content', + sql: chSql`toString(Body)`, + isSubquery: false, + }, + ], + select: [ + { + valueExpression: 'Content:"swagger"', + valueExpressionLanguage: 'lucene', + alias: 'matched', + }, + ], + where: '', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + expect(actual).not.toContain('(1 = 0)'); + }); + + // ClickHouse resolves SELECT aliases in HAVING, so a Lucene HAVING that + // references an alias must resolve it rather than render the no-match + // predicate. + it('resolves a Lucene HAVING reference to a SELECT alias', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: '', + having: 'Content:"swagger"', + havingLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain("Content = 'swagger'"); + }); + + it('renders an unknown, non-alias field in a Lucene HAVING as the no-match predicate', async () => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Table, + connection: 'test-connection', + from: { + databaseName: 'default', + tableName: 'logs', + }, + select: [{ valueExpression: 'Body', alias: 'Content' }], + where: '', + having: 'NotAColumn:"swagger"', + havingLanguage: 'lucene', + }; + + const generatedSql = await renderChartConfig( + config, + mockMetadata, + querySettings, + ); + const actual = parameterizedQueryToSql(generatedSql); + expect(actual).toContain('(1 = 0)'); + expect(actual).not.toContain('NotAColumn'); + }); + }); + describe('SETTINGS clause', () => { const config: ChartConfigWithOptDateRangeEx = { displayType: DisplayType.Table, 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/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 4f5c8a48d5..63fd8dc517 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -2,7 +2,13 @@ import isPlainObject from 'lodash/isPlainObject'; import * as SQLParser from 'node-sql-parser'; import SqlString from 'sqlstring'; -import { ChSql, chSql, concatChSql, wrapChSqlIfNotEmpty } from '@/clickhouse'; +import { + ChSql, + chSql, + chSqlToAliasMap, + concatChSql, + wrapChSqlIfNotEmpty, +} from '@/clickhouse'; import { translateHistogram } from '@/core/histogram'; import { Metadata } from '@/core/metadata'; import { @@ -682,6 +688,8 @@ async function renderSelectList( const isRatio = isRatioChartConfig(selectList, chartConfig); + const selectAliases = extractSelectAliases(chartConfig); + const selectsSQL = await Promise.all( selectList.map(async select => { const whereClause = isNonEmptyWhereExpr(select.aggCondition) @@ -696,6 +704,7 @@ async function renderSelectList( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases, }) : chSql``; @@ -714,6 +723,7 @@ async function renderSelectList( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases, }) : chSql`${{ UNSAFE_RAW_SQL: select.valueExpression }}`; } else if ( @@ -973,6 +983,77 @@ function renderFrom({ ); } +/** + * Collects the aliases a Lucene WHERE may legally reference as bare + * identifiers: SELECT-list aliases plus expression-form WITH clauses. + * + * Array-form select lists expose `alias` directly. Raw-string lists (e.g. a + * source's `defaultTableSelectExpression` like + * 'Timestamp, ServiceName as service, Body') are parsed via `chSqlToAliasMap` + * to recover their declared aliases. + * + * WITH clauses only contribute when they declare an expression alias + * (`isSubquery === false`, i.e. `WITH (expr) AS ident`). Subquery CTEs + * (`WITH ident AS (subquery)`) name a table-like source, not a column usable in + * WHERE, so they're excluded. SAVED_SEARCH alerts depend on this: the alert + * query selects count() while the saved search's select aliases are injected as + * expression WITH clauses (see `computeAliasWithClauses` in the alert task). + */ +// Memoizes the alias set per chart config. `renderSelectList`, `renderWhere`, +// and `renderHaving` each need it from the same config, so without this a single +// render reparses any string-form select list (via `chSqlToAliasMap`) up to +// three times. Keyed on config identity; entries are GC'd with the config. +const selectAliasesByChartConfig = new WeakMap>(); + +function extractSelectAliases( + chartConfig: BuilderChartConfigWithOptDateRangeEx, +): Set { + const cached = selectAliasesByChartConfig.get(chartConfig); + if (cached != null) { + return cached; + } + + const aliases: string[] = []; + const selectLists: (SelectList | undefined)[] = [ + chartConfig.select, + chartConfig.groupBy, + ]; + + for (const list of selectLists) { + if (list == null) { + continue; + } + + if (typeof list === 'string') { + // Wrap the select-list string in a parse scaffold (mirrors the + // `SELECT * FROM \`t\`` scaffold used below). chSqlToAliasMap swallows + // parse failures and returns {}, so an unparseable list contributes no + // aliases rather than throwing. + const aliasMap = chSqlToAliasMap( + chSql`SELECT ${{ UNSAFE_RAW_SQL: list }} FROM t`, + ); + aliases.push(...Object.keys(aliasMap)); + continue; + } + + for (const col of list) { + if (col.alias && col.alias.trim() !== '') { + aliases.push(col.alias); + } + } + } + + for (const clause of chartConfig.with ?? []) { + if (clause.isSubquery === false && clause.name.trim() !== '') { + aliases.push(clause.name); + } + } + + const result = new Set(aliases); + selectAliasesByChartConfig.set(chartConfig, result); + return result; +} + async function renderWhereExpressionStr({ condition, language, @@ -983,6 +1064,7 @@ async function renderWhereExpressionStr({ useTextIndexForImplicitColumn, connectionId, with: withClauses, + selectAliases, }: { condition: SearchCondition; language: SearchConditionLanguage; @@ -993,6 +1075,7 @@ async function renderWhereExpressionStr({ useTextIndexForImplicitColumn?: BuilderChartConfigWithDateRange['useTextIndexForImplicitColumn']; connectionId: string; with?: BuilderChartConfigWithDateRange['with']; + selectAliases?: Set; }): Promise { let _condition = condition; if (language === 'lucene') { @@ -1004,6 +1087,7 @@ async function renderWhereExpressionStr({ bodyExpression, useTextIndexForImplicitColumn, connectionId: connectionId, + selectAliases, }); const builder = new SearchQueryBuilder(condition, serializer); _condition = await builder.build(); @@ -1053,6 +1137,8 @@ async function renderWhere( chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, ): Promise { + const selectAliases = extractSelectAliases(chartConfig); + let whereSearchCondition: ChSql | [] = []; if (isNonEmptyWhereExpr(chartConfig.where)) { whereSearchCondition = wrapChSqlIfNotEmpty( @@ -1067,6 +1153,7 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases, }), '(', ')', @@ -1095,6 +1182,7 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases, }); } return null; @@ -1143,6 +1231,7 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases, }), '(', ')', @@ -1333,6 +1422,7 @@ async function renderHaving( metadata, connectionId: chartConfig.connection, with: chartConfig.with, + selectAliases: extractSelectAliases(chartConfig), }); } 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. diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index 81445b2588..de015328c8 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -762,6 +762,7 @@ export type CustomSchemaConfig = { * metadata; otherwise, it forces the chosen behavior. */ useTextIndexForImplicitColumn?: UseTextIndex; + selectAliases?: Set; }; function renderArrayFieldExpression({ @@ -1166,6 +1167,7 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { private skipIndicesPromise?: Promise; private enableTextIndexPromise?: Promise; private kvItemsLookupPromise?: Promise; + private selectAliases: Set | undefined; constructor({ metadata, @@ -1175,6 +1177,7 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { implicitColumnExpression, bodyExpression, useTextIndexForImplicitColumn, + selectAliases, }: { metadata: Metadata } & CustomSchemaConfig) { super(); this.metadata = metadata; @@ -1185,6 +1188,7 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { this.connectionId = connectionId; this.useTextIndexForImplicitColumn = useTextIndexForImplicitColumn ?? UseTextIndex.Auto; + this.selectAliases = selectAliases; // Pre-fetch skip indices for potential bloom filter optimization this.skipIndicesPromise = this.metadata @@ -1473,13 +1477,14 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { }; let materializedColumns: Map; try { - // This won't work for CTEs + // This won't work for CTEs. Treat a missing lookup (null/undefined) the + // same as the catch path below: proceed with no materialized columns. materializedColumns = - await this.metadata.getMaterializedColumnsLookupTable({ + (await this.metadata.getMaterializedColumnsLookupTable({ databaseName: this.databaseName, tableName: this.tableName, connectionId: this.connectionId, - }); + })) ?? new Map(); } catch (e) { console.debug('Error in getMaterializedColumnsLookupTable', e); materializedColumns = new Map(); @@ -1586,14 +1591,19 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { throw new Error('Unsupported column type for prefix match'); } - // It might be an alias, let's just try the column - // TODO: Verify aliases + if (this.selectAliases?.has(field)) { + return { + found: true, + columnExpression: field, + columnType: 'Unknown', + }; + } + return { - found: true, + found: false, columnExpression: field, columnType: 'Unknown', }; - // throw new Error(`Column not found: ${field}`); } private isLowerExpression(expr: string): boolean {