From 7fae5c311e85d9edc44b04b8944567bbc9224f7e Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 18:02:35 -0500 Subject: [PATCH 1/6] feat: show the most frequent text index values with topKWeighted --- .changeset/topk-text-index-values.md | 11 ++++++++ .../src/__tests__/metadata.int.test.ts | 27 +++++++++++++++++++ .../src/__tests__/metadata.test.ts | 23 +++++++--------- packages/common-utils/src/core/metadata.ts | 6 +++-- 4 files changed, 51 insertions(+), 16 deletions(-) create mode 100644 .changeset/topk-text-index-values.md diff --git a/.changeset/topk-text-index-values.md b/.changeset/topk-text-index-values.md new file mode 100644 index 0000000000..332e05a55d --- /dev/null +++ b/.changeset/topk-text-index-values.md @@ -0,0 +1,11 @@ +--- +'@hyperdx/common-utils': patch +--- + +feat: show the most frequent filter values read from text indexes + +"All values" lookups that read a text index (Map key=value indexes and text +indexes on plain columns) used `groupUniqArray(limit)`, which keeps the +alphabetically first tokens it reads. They now use `topKWeighted(limit)` +weighted by each token's row count, so a capped list holds the most common +values, most common first. This costs more CPU per token than before. diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 55853a7421..10e8340391 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -907,6 +907,33 @@ describe('Metadata Integration Tests', () => { ); }); + it('keeps the most frequent text index values when maxValuesPerKey truncates the list', async () => { + if (!textIndexSupported) { + console.warn( + 'Skipping: ClickHouse < 26.3 does not support mergeTreeTextIndex()', + ); + return; + } + // The index dictionary is sorted, so 'a-rare' is read first. + const freqKey = `freq.${tag}`; + const timestamp = testTime.toISOString().replace('T', ' ').slice(0, 23); + await client.command({ + query: `INSERT INTO default.otel_logs (Timestamp, ResourceAttributes) + SELECT '${timestamp}', map('${freqKey}', if(number = 0, 'a-rare', 'z-common')) + FROM numbers(4)`, + }); + + const result = await metadata.getAllKeyValues({ + ...commonArgs, + keyExpressions: [`ResourceAttributes['${freqKey}']`], + maxValuesPerKey: 1, + }); + + expect(result).toEqual([ + { key: `ResourceAttributes['${freqKey}']`, value: ['z-common'] }, + ]); + }); + it('routes TraceId through getTextIndexKeyValues', async () => { if (!textIndexSupported) { console.warn( diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index f07e176c4a..5621ddb36a 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1375,7 +1375,7 @@ describe('Metadata', () => { const sql = calls[calls.length - 1][0].query as string; expect(sql).toContain('mergeTreeTextIndex('); - expect(sql).toContain('groupUniqArray('); + expect(sql).toContain('topKWeighted('); expect(sql).toContain('GROUP BY key'); expect(sql).not.toContain('startsWith(token,'); }); @@ -1718,9 +1718,8 @@ describe('Metadata', () => { // via text index or raw scan. describe('maxValuesPerKey threading', () => { // Pick a value that (a) isn't ambient in the test setup (like 20, - // dates, or granularity numbers) and (b) is easy to spot in a param - // dump when a test fails. Int32 chSql params render as numbers, so - // assertions compare against the numeric literal. + // dates, or granularity numbers) and (b) is easy to spot in the SQL or a + // param dump when a test fails. const MAX_VALUES = 7777; it('threads maxValuesPerKey into the map text-index query', async () => { @@ -1740,11 +1739,9 @@ describe('Metadata', () => { c[0].query.includes('startsWith(token,'), ); expect(mapTextIndexCall).toBeDefined(); - const params = mapTextIndexCall![0].query_params as Record< - string, - unknown - >; - expect(Object.values(params)).toContain(MAX_VALUES); + expect(mapTextIndexCall![0].query).toContain( + `topKWeighted(${MAX_VALUES})(`, + ); }); it('threads maxValuesPerKey into the native text-index query', async () => { @@ -1765,11 +1762,9 @@ describe('Metadata', () => { !c[0].query.includes('startsWith(token,'), ); expect(nativeTextIndexCall).toBeDefined(); - const params = nativeTextIndexCall![0].query_params as Record< - string, - unknown - >; - expect(Object.values(params)).toContain(MAX_VALUES); + expect(nativeTextIndexCall![0].query).toContain( + `topKWeighted(${MAX_VALUES})(token, cardinality)`, + ); }); it('threads maxValuesPerKey into the raw-table getKeyValues fallback', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 34425ac362..5da2f783f4 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1734,11 +1734,13 @@ export class Metadata { timestampValueExpression, }); const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; + // `cardinality` is the token's row count in the part, so this keeps + // each key's most frequent values rather than the first ones read. const sql = chSql` SELECT * FROM ( SELECT ${{ String: columnName }} as column, substring(token, 1, position(token, ${{ String: info.separator }}) - 1) AS key, - groupUniqArray(${{ Int32: info.limit }})(${valueSql}) AS value + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(${valueSql}, cardinality) AS value FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} AND (${orChain}) @@ -1815,7 +1817,7 @@ export class Metadata { const sql = chSql` SELECT * FROM ( SELECT ${{ String: columnName }} AS key, - groupUniqArray(${{ Int32: info.limit }})(token) AS value + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(token, cardinality) AS value FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} AND token != '' From e66388dafe66fd7008bcd1863f0104fdc9219874 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:31:49 -0500 Subject: [PATCH 2/6] perf: read text index values per key with a token cap --- .changeset/topk-text-index-values.md | 16 ++-- .../src/__tests__/metadata.int.test.ts | 16 +++- .../src/__tests__/metadata.test.ts | 30 +++++++- packages/common-utils/src/core/metadata.ts | 76 +++++++++---------- 4 files changed, 88 insertions(+), 50 deletions(-) diff --git a/.changeset/topk-text-index-values.md b/.changeset/topk-text-index-values.md index 332e05a55d..a25ccb0f78 100644 --- a/.changeset/topk-text-index-values.md +++ b/.changeset/topk-text-index-values.md @@ -2,10 +2,16 @@ '@hyperdx/common-utils': patch --- -feat: show the most frequent filter values read from text indexes +perf: read each key's text index values separately, and show the most +frequent ones "All values" lookups that read a text index (Map key=value indexes and text -indexes on plain columns) used `groupUniqArray(limit)`, which keeps the -alphabetically first tokens it reads. They now use `topKWeighted(limit)` -weighted by each token's row count, so a capped list holds the most common -values, most common first. This costs more CPU per token than before. +indexes on plain columns) read every token of every requested key, capped at +`max_rows_to_read` for the whole query. One key with a value per row (request +IDs, UUIDs) slowed the whole query down, and because the index is read in +sorted order, the cap left keys that sort after it with no values. Each key now +has its own query branch that reads at most 100,000 tokens, and there is no +whole-query row cap. Values come from `topKWeighted(limit)` weighted by each +token's row count, so a capped list holds the most common values, most common +first. For keys with more distinct values than the token cap, they are the most +common among the alphabetically first ones. diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 10e8340391..5b5acf2f9f 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -925,13 +925,21 @@ describe('Metadata Integration Tests', () => { const result = await metadata.getAllKeyValues({ ...commonArgs, - keyExpressions: [`ResourceAttributes['${freqKey}']`], + keyExpressions: [ + `ResourceAttributes['${freqKey}']`, + `ResourceAttributes['${mapKey}']`, + ], maxValuesPerKey: 1, }); - expect(result).toEqual([ - { key: `ResourceAttributes['${freqKey}']`, value: ['z-common'] }, - ]); + // UNION ALL branches can arrive in any order. + expect(result).toHaveLength(2); + expect(result).toEqual( + expect.arrayContaining([ + { key: `ResourceAttributes['${freqKey}']`, value: ['z-common'] }, + { key: `ResourceAttributes['${mapKey}']`, value: [podNameA] }, + ]), + ); }); it('routes TraceId through getTextIndexKeyValues', async () => { diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 5621ddb36a..0114d18e04 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1376,7 +1376,7 @@ describe('Metadata', () => { expect(sql).toContain('mergeTreeTextIndex('); expect(sql).toContain('topKWeighted('); - expect(sql).toContain('GROUP BY key'); + expect(sql).toContain('LIMIT 100000'); expect(sql).not.toContain('startsWith(token,'); }); @@ -1395,7 +1395,33 @@ describe('Metadata', () => { expect(sql).toContain('mergeTreeTextIndex('); expect(sql).toContain('startsWith(token,'); expect(sql).toContain('substring(token, position(token,'); - expect(sql).toContain('GROUP BY column, key'); + expect(sql).toContain("startsWith(token, 'requestId=')"); + }); + + it('reads a capped number of tokens per Map key, without a row cap that could starve later keys', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: [ + "LogAttributes['requestId']", + "LogAttributes['region']", + ], + }); + + const call = (mockClickhouseClient.query as jest.Mock).mock.calls.find( + ([opts]) => (opts.query as string).includes('startsWith(token,'), + )![0]; + const branches = (call.query as string).split('UNION ALL'); + expect(branches).toHaveLength(2); + expect(branches[0]).toContain("startsWith(token, 'requestId=')"); + expect(branches[0]).toContain("'requestId' AS key"); + expect(branches[1]).toContain("startsWith(token, 'region=')"); + for (const branch of branches) { + expect(branch).toContain('LIMIT 100000'); + } + expect(call.clickhouse_settings.max_rows_to_read).toBe('0'); + expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined(); }); it('routes ServiceName and SeverityText through getMetadataMVKeyValues (KV rollup MV path)', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 5da2f783f4..301abf62aa 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -69,6 +69,11 @@ const DEFAULT_MAX_KEYS = 1000; // Cap keys per dispatched query: each key is another operation for the db to fetch, and simply fetching all keys at once can be too much for the db to handle. export const GET_ALL_KEY_VALUES_CHUNK_SIZE = 100; +// Tokens a text index values lookup reads per key. The dictionary is sorted, +// so a key with more distinct values than this gets the most frequent among +// its alphabetically first ones. Keep it above "Load more"'s 10,000 values. +const TEXT_INDEX_TOKENS_PER_KEY = 100_000; + /** * The index read could not be scoped to the requested date range, so it was * refused in favour of a path that can. Not a failure — an expected property @@ -1714,19 +1719,6 @@ export class Metadata { try { const sqlBranches: Array = []; for (const [columnName, info] of queryOptions.entries()) { - const orChain = concatChSql( - ' OR ', - // Inline keys as SQL-escaped literals, not bind params: ~100 - // per-key params exceed the web client's URL param budget and - // silently switch the request to a multipart body that proxies - // may reject. Keys are ingest-controlled, hence SqlString.escape. - info.keys.map( - k => - chSql`startsWith(token, ${{ - UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`), - }})`, - ), - ); const partsFilter = await this.partsOverlapFilter({ databaseName, tableName, @@ -1734,20 +1726,29 @@ export class Metadata { timestampValueExpression, }); const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; - // `cardinality` is the token's row count in the part, so this keeps - // each key's most frequent values rather than the first ones read. - const sql = chSql` - SELECT * FROM ( - SELECT ${{ String: columnName }} as column, - substring(token, 1, position(token, ${{ String: info.separator }}) - 1) AS key, - topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(${valueSql}, cardinality) AS value + // One branch per key, each with its own LIMIT: the read for a key + // stops after TEXT_INDEX_TOKENS_PER_KEY tokens, so a key with a + // value per row can't slow down or starve the others. `cardinality` + // is the token's row count in the part, so topKWeighted keeps the + // most frequent values among the tokens read. + for (const k of info.keys) { + // Inline keys as SQL-escaped literals, not bind params: ~100 + // per-key params exceed the web client's URL param budget and + // silently switch the request to a multipart body that proxies + // may reject. Keys are ingest-controlled, hence SqlString.escape. + sqlBranches.push(chSql` + SELECT ${{ String: columnName }} AS column, + ${{ UNSAFE_RAW_SQL: SqlString.escape(k) }} AS key, + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(v, cardinality) AS value + FROM ( + SELECT ${valueSql} AS v, cardinality FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} - AND (${orChain}) - AND ${valueSql} != '' - GROUP BY column, key - )`; - sqlBranches.push(sql); + AND startsWith(token, ${{ UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`) }}) + AND v != '' + LIMIT ${{ UNSAFE_RAW_SQL: String(TEXT_INDEX_TOKENS_PER_KEY) }} + )`); + } } const sql = concatChSql(' UNION ALL ', sqlBranches); @@ -1757,12 +1758,10 @@ export class Metadata { query_params: sql.params, connectionId, clickhouse_settings: { - max_rows_to_read: String( - this.getClickHouseSettings().max_rows_to_read ?? - DEFAULT_METADATA_MAX_ROWS_TO_READ, - ), - read_overflow_mode: 'break', ...this.getClickHouseSettings(), + // The per-key LIMIT bounds the read. A row cap would stop the + // sorted dictionary read partway and leave later keys empty. + max_rows_to_read: '0', }, abort_signal: signal, }) @@ -1814,14 +1813,16 @@ export class Metadata { dateRange, timestampValueExpression, }); + // See getMapTextIndexKeyValues for the per-column LIMIT. const sql = chSql` - SELECT * FROM ( - SELECT ${{ String: columnName }} AS key, - topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(token, cardinality) AS value + SELECT ${{ String: columnName }} AS key, + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(token, cardinality) AS value + FROM ( + SELECT token, cardinality FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} AND token != '' - GROUP BY key + LIMIT ${{ UNSAFE_RAW_SQL: String(TEXT_INDEX_TOKENS_PER_KEY) }} )`; sqlBranches.push(sql); } @@ -1833,12 +1834,9 @@ export class Metadata { query_params: sql.params, connectionId, clickhouse_settings: { - max_rows_to_read: String( - this.getClickHouseSettings().max_rows_to_read ?? - DEFAULT_METADATA_MAX_ROWS_TO_READ, - ), - read_overflow_mode: 'break', ...this.getClickHouseSettings(), + // See getMapTextIndexKeyValues. + max_rows_to_read: '0', }, abort_signal: signal, }) From 06a7b170fc0d6ab78998516072648d9040d8c9e5 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:40:01 -0500 Subject: [PATCH 3/6] perf: run text index values in batches of 10 keys --- .../src/__tests__/metadata.test.ts | 18 +++++++++ packages/common-utils/src/core/metadata.ts | 37 ++++++++++++------- 2 files changed, 41 insertions(+), 14 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 0114d18e04..cb6d17dd98 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1424,6 +1424,24 @@ describe('Metadata', () => { expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined(); }); + it('splits Map text index values into queries of at most 10 keys', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: Array.from( + { length: 25 }, + (_, i) => `LogAttributes['key${i}']`, + ), + }); + + const branchCounts = (mockClickhouseClient.query as jest.Mock).mock.calls + .map(([opts]) => opts.query as string) + .filter(q => q.includes('startsWith(token,')) + .map(q => q.split('UNION ALL').length); + expect(branchCounts).toEqual([10, 10, 5]); + }); + it('routes ServiceName and SeverityText through getMetadataMVKeyValues (KV rollup MV path)', async () => { setupDefaultLogsSchema(); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 301abf62aa..21c7af1a11 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -73,6 +73,8 @@ export const GET_ALL_KEY_VALUES_CHUNK_SIZE = 100; // so a key with more distinct values than this gets the most frequent among // its alphabetically first ones. Keep it above "Load more"'s 10,000 values. const TEXT_INDEX_TOKENS_PER_KEY = 100_000; +// Keys per text index values query; see dispatchKeyValueQueries. +const TEXT_INDEX_KEYS_PER_QUERY = 10; /** * The index read could not be scoped to the requested date range, so it was @@ -2983,26 +2985,33 @@ export class Metadata { // fire all the kv fetch queries const promises: Array> = []; - if (mapTextIndexQueryOptions.size > 0) { - promises.push( - this.getMapTextIndexKeyValues({ - databaseName, - tableName, - connectionId, - queryOptions: mapTextIndexQueryOptions, - dateRange, - timestampValueExpression, - signal, - }), - ); + // Text index lookups run in small parallel batches, so a slow, + // high-cardinality key holds up only its own batch's values. + for (const [column, options] of mapTextIndexQueryOptions) { + for (const keys of chunk(options.keys, TEXT_INDEX_KEYS_PER_QUERY)) { + promises.push( + this.getMapTextIndexKeyValues({ + databaseName, + tableName, + connectionId, + queryOptions: new Map([[column, { ...options, keys }]]), + dateRange, + timestampValueExpression, + signal, + }), + ); + } } - if (nativeTextIndexQueryOptions.size > 0) { + for (const entries of chunk( + [...nativeTextIndexQueryOptions], + TEXT_INDEX_KEYS_PER_QUERY, + )) { promises.push( this.getTextIndexKeyValues({ databaseName, tableName, connectionId, - queryOptions: nativeTextIndexQueryOptions, + queryOptions: new Map(entries), dateRange, timestampValueExpression, signal, From 969a5e04b69a11f23a9b5d925df2cd032a01201f Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 21:48:14 -0500 Subject: [PATCH 4/6] perf: read text index values in 1024-row blocks so per-key LIMITs stop early --- packages/common-utils/src/__tests__/metadata.test.ts | 5 +++++ packages/common-utils/src/core/metadata.ts | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index cb6d17dd98..9495777a05 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1378,6 +1378,10 @@ describe('Metadata', () => { expect(sql).toContain('topKWeighted('); expect(sql).toContain('LIMIT 100000'); expect(sql).not.toContain('startsWith(token,'); + expect(calls[calls.length - 1][0].clickhouse_settings).toMatchObject({ + max_rows_to_read: '0', + max_block_size: '1024', + }); }); it("routes LogAttributes['requestId'] through getMapTextIndexKeyValues (map KV text index path)", async () => { @@ -1422,6 +1426,7 @@ describe('Metadata', () => { } expect(call.clickhouse_settings.max_rows_to_read).toBe('0'); expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined(); + expect(call.clickhouse_settings.max_block_size).toBe('1024'); }); it('splits Map text index values into queries of at most 10 keys', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 21c7af1a11..fa9ec1be4c 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1764,6 +1764,9 @@ export class Metadata { // The per-key LIMIT bounds the read. A row cap would stop the // sorted dictionary read partway and leave later keys empty. max_rows_to_read: '0', + // LIMIT stops a part's read only after a whole block, so the + // default 65k-row block makes every key read far past its LIMIT. + max_block_size: '1024', }, abort_signal: signal, }) @@ -1839,6 +1842,7 @@ export class Metadata { ...this.getClickHouseSettings(), // See getMapTextIndexKeyValues. max_rows_to_read: '0', + max_block_size: '1024', }, abort_signal: signal, }) From a90f883fadb88358020044a8d6a8dba173eaf540 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 22:13:53 -0500 Subject: [PATCH 5/6] Revert "perf: read text index values in 1024-row blocks so per-key LIMITs stop early" This reverts commit 969a5e04b69a11f23a9b5d925df2cd032a01201f. --- packages/common-utils/src/__tests__/metadata.test.ts | 5 ----- packages/common-utils/src/core/metadata.ts | 4 ---- 2 files changed, 9 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 9495777a05..cb6d17dd98 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1378,10 +1378,6 @@ describe('Metadata', () => { expect(sql).toContain('topKWeighted('); expect(sql).toContain('LIMIT 100000'); expect(sql).not.toContain('startsWith(token,'); - expect(calls[calls.length - 1][0].clickhouse_settings).toMatchObject({ - max_rows_to_read: '0', - max_block_size: '1024', - }); }); it("routes LogAttributes['requestId'] through getMapTextIndexKeyValues (map KV text index path)", async () => { @@ -1426,7 +1422,6 @@ describe('Metadata', () => { } expect(call.clickhouse_settings.max_rows_to_read).toBe('0'); expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined(); - expect(call.clickhouse_settings.max_block_size).toBe('1024'); }); it('splits Map text index values into queries of at most 10 keys', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index fa9ec1be4c..21c7af1a11 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1764,9 +1764,6 @@ export class Metadata { // The per-key LIMIT bounds the read. A row cap would stop the // sorted dictionary read partway and leave later keys empty. max_rows_to_read: '0', - // LIMIT stops a part's read only after a whole block, so the - // default 65k-row block makes every key read far past its LIMIT. - max_block_size: '1024', }, abort_signal: signal, }) @@ -1842,7 +1839,6 @@ export class Metadata { ...this.getClickHouseSettings(), // See getMapTextIndexKeyValues. max_rows_to_read: '0', - max_block_size: '1024', }, abort_signal: signal, }) From 6525149b281abf2773a59667dcc69c153dc3cc00 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 10 Nov 2026 22:15:23 -0500 Subject: [PATCH 6/6] Revert "perf: read text index values per key with a token cap" This reverts commit e66388dafe66fd7008bcd1863f0104fdc9219874. --- .changeset/topk-text-index-values.md | 18 ++--- .../src/__tests__/metadata.int.test.ts | 16 +--- .../src/__tests__/metadata.test.ts | 36 ++------- packages/common-utils/src/core/metadata.ts | 75 ++++++++++--------- 4 files changed, 55 insertions(+), 90 deletions(-) diff --git a/.changeset/topk-text-index-values.md b/.changeset/topk-text-index-values.md index a25ccb0f78..95f8e695d3 100644 --- a/.changeset/topk-text-index-values.md +++ b/.changeset/topk-text-index-values.md @@ -2,16 +2,12 @@ '@hyperdx/common-utils': patch --- -perf: read each key's text index values separately, and show the most -frequent ones +perf: show the most frequent text index values, in batches of 10 keys "All values" lookups that read a text index (Map key=value indexes and text -indexes on plain columns) read every token of every requested key, capped at -`max_rows_to_read` for the whole query. One key with a value per row (request -IDs, UUIDs) slowed the whole query down, and because the index is read in -sorted order, the cap left keys that sort after it with no values. Each key now -has its own query branch that reads at most 100,000 tokens, and there is no -whole-query row cap. Values come from `topKWeighted(limit)` weighted by each -token's row count, so a capped list holds the most common values, most common -first. For keys with more distinct values than the token cap, they are the most -common among the alphabetically first ones. +indexes on plain columns) used `groupUniqArray(limit)`, which keeps the +alphabetically first tokens it reads. They now use `topKWeighted(limit)` +weighted by each token's row count, so a capped list holds the most common +values, most common first. These lookups also run in parallel batches of at +most 10 keys instead of 100, so a key with very many values delays only its own +batch. diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 5b5acf2f9f..10e8340391 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -925,21 +925,13 @@ describe('Metadata Integration Tests', () => { const result = await metadata.getAllKeyValues({ ...commonArgs, - keyExpressions: [ - `ResourceAttributes['${freqKey}']`, - `ResourceAttributes['${mapKey}']`, - ], + keyExpressions: [`ResourceAttributes['${freqKey}']`], maxValuesPerKey: 1, }); - // UNION ALL branches can arrive in any order. - expect(result).toHaveLength(2); - expect(result).toEqual( - expect.arrayContaining([ - { key: `ResourceAttributes['${freqKey}']`, value: ['z-common'] }, - { key: `ResourceAttributes['${mapKey}']`, value: [podNameA] }, - ]), - ); + expect(result).toEqual([ + { key: `ResourceAttributes['${freqKey}']`, value: ['z-common'] }, + ]); }); it('routes TraceId through getTextIndexKeyValues', async () => { diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index cb6d17dd98..746d3e999c 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1376,7 +1376,7 @@ describe('Metadata', () => { expect(sql).toContain('mergeTreeTextIndex('); expect(sql).toContain('topKWeighted('); - expect(sql).toContain('LIMIT 100000'); + expect(sql).toContain('GROUP BY key'); expect(sql).not.toContain('startsWith(token,'); }); @@ -1395,33 +1395,7 @@ describe('Metadata', () => { expect(sql).toContain('mergeTreeTextIndex('); expect(sql).toContain('startsWith(token,'); expect(sql).toContain('substring(token, position(token,'); - expect(sql).toContain("startsWith(token, 'requestId=')"); - }); - - it('reads a capped number of tokens per Map key, without a row cap that could starve later keys', async () => { - setupDefaultLogsSchema(); - - await metadata.getAllKeyValues({ - ...baseArgs, - keyExpressions: [ - "LogAttributes['requestId']", - "LogAttributes['region']", - ], - }); - - const call = (mockClickhouseClient.query as jest.Mock).mock.calls.find( - ([opts]) => (opts.query as string).includes('startsWith(token,'), - )![0]; - const branches = (call.query as string).split('UNION ALL'); - expect(branches).toHaveLength(2); - expect(branches[0]).toContain("startsWith(token, 'requestId=')"); - expect(branches[0]).toContain("'requestId' AS key"); - expect(branches[1]).toContain("startsWith(token, 'region=')"); - for (const branch of branches) { - expect(branch).toContain('LIMIT 100000'); - } - expect(call.clickhouse_settings.max_rows_to_read).toBe('0'); - expect(call.clickhouse_settings.read_overflow_mode).toBeUndefined(); + expect(sql).toContain('GROUP BY column, key'); }); it('splits Map text index values into queries of at most 10 keys', async () => { @@ -1435,11 +1409,11 @@ describe('Metadata', () => { ), }); - const branchCounts = (mockClickhouseClient.query as jest.Mock).mock.calls + const keysPerQuery = (mockClickhouseClient.query as jest.Mock).mock.calls .map(([opts]) => opts.query as string) .filter(q => q.includes('startsWith(token,')) - .map(q => q.split('UNION ALL').length); - expect(branchCounts).toEqual([10, 10, 5]); + .map(q => q.split('startsWith(token,').length - 1); + expect(keysPerQuery).toEqual([10, 10, 5]); }); it('routes ServiceName and SeverityText through getMetadataMVKeyValues (KV rollup MV path)', async () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 21c7af1a11..f07061dff8 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -69,10 +69,6 @@ const DEFAULT_MAX_KEYS = 1000; // Cap keys per dispatched query: each key is another operation for the db to fetch, and simply fetching all keys at once can be too much for the db to handle. export const GET_ALL_KEY_VALUES_CHUNK_SIZE = 100; -// Tokens a text index values lookup reads per key. The dictionary is sorted, -// so a key with more distinct values than this gets the most frequent among -// its alphabetically first ones. Keep it above "Load more"'s 10,000 values. -const TEXT_INDEX_TOKENS_PER_KEY = 100_000; // Keys per text index values query; see dispatchKeyValueQueries. const TEXT_INDEX_KEYS_PER_QUERY = 10; @@ -1721,6 +1717,19 @@ export class Metadata { try { const sqlBranches: Array = []; for (const [columnName, info] of queryOptions.entries()) { + const orChain = concatChSql( + ' OR ', + // Inline keys as SQL-escaped literals, not bind params: ~100 + // per-key params exceed the web client's URL param budget and + // silently switch the request to a multipart body that proxies + // may reject. Keys are ingest-controlled, hence SqlString.escape. + info.keys.map( + k => + chSql`startsWith(token, ${{ + UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`), + }})`, + ), + ); const partsFilter = await this.partsOverlapFilter({ databaseName, tableName, @@ -1728,29 +1737,20 @@ export class Metadata { timestampValueExpression, }); const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; - // One branch per key, each with its own LIMIT: the read for a key - // stops after TEXT_INDEX_TOKENS_PER_KEY tokens, so a key with a - // value per row can't slow down or starve the others. `cardinality` - // is the token's row count in the part, so topKWeighted keeps the - // most frequent values among the tokens read. - for (const k of info.keys) { - // Inline keys as SQL-escaped literals, not bind params: ~100 - // per-key params exceed the web client's URL param budget and - // silently switch the request to a multipart body that proxies - // may reject. Keys are ingest-controlled, hence SqlString.escape. - sqlBranches.push(chSql` - SELECT ${{ String: columnName }} AS column, - ${{ UNSAFE_RAW_SQL: SqlString.escape(k) }} AS key, - topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(v, cardinality) AS value - FROM ( - SELECT ${valueSql} AS v, cardinality + // `cardinality` is the token's row count in the part, so this keeps + // each key's most frequent values rather than the first ones read. + const sql = chSql` + SELECT * FROM ( + SELECT ${{ String: columnName }} as column, + substring(token, 1, position(token, ${{ String: info.separator }}) - 1) AS key, + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(${valueSql}, cardinality) AS value FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} - AND startsWith(token, ${{ UNSAFE_RAW_SQL: SqlString.escape(`${k}${info.separator}`) }}) - AND v != '' - LIMIT ${{ UNSAFE_RAW_SQL: String(TEXT_INDEX_TOKENS_PER_KEY) }} - )`); - } + AND (${orChain}) + AND ${valueSql} != '' + GROUP BY column, key + )`; + sqlBranches.push(sql); } const sql = concatChSql(' UNION ALL ', sqlBranches); @@ -1760,10 +1760,12 @@ export class Metadata { query_params: sql.params, connectionId, clickhouse_settings: { + max_rows_to_read: String( + this.getClickHouseSettings().max_rows_to_read ?? + DEFAULT_METADATA_MAX_ROWS_TO_READ, + ), + read_overflow_mode: 'break', ...this.getClickHouseSettings(), - // The per-key LIMIT bounds the read. A row cap would stop the - // sorted dictionary read partway and leave later keys empty. - max_rows_to_read: '0', }, abort_signal: signal, }) @@ -1815,16 +1817,14 @@ export class Metadata { dateRange, timestampValueExpression, }); - // See getMapTextIndexKeyValues for the per-column LIMIT. const sql = chSql` - SELECT ${{ String: columnName }} AS key, - topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(token, cardinality) AS value - FROM ( - SELECT token, cardinality + SELECT * FROM ( + SELECT ${{ String: columnName }} AS key, + topKWeighted(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(info.limit, 'limit') }})(token, cardinality) AS value FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) WHERE ${partsFilter} AND token != '' - LIMIT ${{ UNSAFE_RAW_SQL: String(TEXT_INDEX_TOKENS_PER_KEY) }} + GROUP BY key )`; sqlBranches.push(sql); } @@ -1836,9 +1836,12 @@ export class Metadata { query_params: sql.params, connectionId, clickhouse_settings: { + max_rows_to_read: String( + this.getClickHouseSettings().max_rows_to_read ?? + DEFAULT_METADATA_MAX_ROWS_TO_READ, + ), + read_overflow_mode: 'break', ...this.getClickHouseSettings(), - // See getMapTextIndexKeyValues. - max_rows_to_read: '0', }, abort_signal: signal, })