From 3a72ed663689cfca54cc998d478afd656726c7de Mon Sep 17 00:00:00 2001 From: Karl Power <85935352+karl-power@users.noreply.github.com> Date: Thu, 11 Jun 2026 17:54:20 +0200 Subject: [PATCH] Revert "fix: unknown lucene field falls through in search (#2422)" This reverts commit 8aad6d6d6c66e2eb6b041f241268f82d675edfb1. --- .changeset/healthy-pans-grab.md | 5 - .../src/__tests__/queryParser.test.ts | 45 -- .../src/__tests__/renderChartConfig.test.ts | 395 ------------------ .../src/core/renderChartConfig.ts | 81 ---- packages/common-utils/src/queryParser.ts | 24 +- 5 files changed, 7 insertions(+), 543 deletions(-) delete mode 100644 .changeset/healthy-pans-grab.md diff --git a/.changeset/healthy-pans-grab.md b/.changeset/healthy-pans-grab.md deleted file mode 100644 index df79b08a65..0000000000 --- a/.changeset/healthy-pans-grab.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@hyperdx/common-utils": patch ---- - -fix: unknown lucene field falls through in search diff --git a/packages/common-utils/src/__tests__/queryParser.test.ts b/packages/common-utils/src/__tests__/queryParser.test.ts index d735d52f2a..a2afc2118f 100644 --- a/packages/common-utils/src/__tests__/queryParser.test.ts +++ b/packages/common-utils/src/__tests__/queryParser.test.ts @@ -35,8 +35,6 @@ 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 { @@ -496,49 +494,6 @@ 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 8acf84b011..96e26dc805 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -31,7 +31,6 @@ describe('renderChartConfig', () => { { name: 'value', type: 'Float64' }, { name: 'TraceId', type: 'String' }, { name: 'ServiceName', type: 'String' }, - { name: 'severity', type: 'String' }, ]; mockMetadata = { getColumns: jest.fn().mockResolvedValue([ @@ -2253,400 +2252,6 @@ 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'); - }); - - // 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/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index 75cc63fc33..c25e985890 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -5,7 +5,6 @@ import SqlString from 'sqlstring'; import { ChSql, chSql, - chSqlToAliasMap, concatChSql, convertCHDataTypeToJSType, JSDataType, @@ -690,11 +689,6 @@ async function renderSelectList( const isRatio = isRatioChartConfig(selectList, chartConfig); - const selectAliases = extractSelectAliases({ - selectLists: [chartConfig.select, chartConfig.groupBy], - withClauses: chartConfig.with, - }); - const selectsSQL = await Promise.all( selectList.map(async select => { const whereClause = await renderWhereExpression({ @@ -708,7 +702,6 @@ async function renderSelectList( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases, }); let expr: ChSql; @@ -726,7 +719,6 @@ async function renderSelectList( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases, }) : chSql`${{ UNSAFE_RAW_SQL: select.valueExpression }}`; } else if ( @@ -986,64 +978,6 @@ 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). - */ -function extractSelectAliases({ - selectLists, - withClauses, -}: { - selectLists: (SelectList | undefined)[]; - withClauses?: BuilderChartConfigWithDateRange['with']; -}): Set { - const aliases: string[] = []; - - 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 withClauses ?? []) { - if (clause.isSubquery === false && clause.name.trim() !== '') { - aliases.push(clause.name); - } - } - - return new Set(aliases); -} - async function renderWhereExpressionStr({ condition, language, @@ -1054,7 +988,6 @@ async function renderWhereExpressionStr({ useTextIndexForImplicitColumn, connectionId, with: withClauses, - selectAliases, }: { condition: SearchCondition; language: SearchConditionLanguage; @@ -1065,7 +998,6 @@ async function renderWhereExpressionStr({ useTextIndexForImplicitColumn?: BuilderChartConfigWithDateRange['useTextIndexForImplicitColumn']; connectionId: string; with?: BuilderChartConfigWithDateRange['with']; - selectAliases?: Set; }): Promise { let _condition = condition; if (language === 'lucene') { @@ -1077,7 +1009,6 @@ async function renderWhereExpressionStr({ bodyExpression, useTextIndexForImplicitColumn, connectionId: connectionId, - selectAliases, }); const builder = new SearchQueryBuilder(condition, serializer); _condition = await builder.build(); @@ -1127,11 +1058,6 @@ async function renderWhere( chartConfig: BuilderChartConfigWithOptDateRangeEx, metadata: Metadata, ): Promise { - const selectAliases = extractSelectAliases({ - selectLists: [chartConfig.select, chartConfig.groupBy], - withClauses: chartConfig.with, - }); - let whereSearchCondition: ChSql | [] = []; if (isNonEmptyWhereExpr(chartConfig.where)) { whereSearchCondition = wrapChSqlIfNotEmpty( @@ -1146,7 +1072,6 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases, }), '(', ')', @@ -1175,7 +1100,6 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases, }); } return null; @@ -1224,7 +1148,6 @@ async function renderWhere( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases, }), '(', ')', @@ -1393,10 +1316,6 @@ async function renderHaving( metadata, connectionId: chartConfig.connection, with: chartConfig.with, - selectAliases: extractSelectAliases({ - selectLists: [chartConfig.select, chartConfig.groupBy], - withClauses: chartConfig.with, - }), }); } diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index 64c9a7cca3..2a500c6b1c 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -762,7 +762,6 @@ export type CustomSchemaConfig = { * metadata; otherwise, it forces the chosen behavior. */ useTextIndexForImplicitColumn?: UseTextIndex; - selectAliases?: Set; }; function renderArrayFieldExpression({ @@ -1167,7 +1166,6 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { private skipIndicesPromise?: Promise; private enableTextIndexPromise?: Promise; private kvItemsLookupPromise?: Promise; - private selectAliases: Set | undefined; constructor({ metadata, @@ -1177,7 +1175,6 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { implicitColumnExpression, bodyExpression, useTextIndexForImplicitColumn, - selectAliases, }: { metadata: Metadata } & CustomSchemaConfig) { super(); this.metadata = metadata; @@ -1188,7 +1185,6 @@ 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 @@ -1477,14 +1473,13 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { }; let materializedColumns: Map; try { - // This won't work for CTEs. Treat a missing lookup (null/undefined) the - // same as the catch path below: proceed with no materialized columns. + // This won't work for CTEs 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(); @@ -1591,19 +1586,14 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { throw new Error('Unsupported column type for prefix match'); } - if (this.selectAliases?.has(field)) { - return { - found: true, - columnExpression: field, - columnType: 'Unknown', - }; - } - + // It might be an alias, let's just try the column + // TODO: Verify aliases return { - found: false, + found: true, columnExpression: field, columnType: 'Unknown', }; + // throw new Error(`Column not found: ${field}`); } private isLowerExpression(expr: string): boolean {