diff --git a/.changeset/text-index-key-order.md b/.changeset/text-index-key-order.md new file mode 100644 index 0000000000..b3af9ae7ec --- /dev/null +++ b/.changeset/text-index-key-order.md @@ -0,0 +1,13 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +'@hyperdx/api': patch +--- + +fix: rank Map keys by frequency when a text index serves key discovery + +The two `mergeTreeTextIndex` paths in `getMapKeys` grouped keys and applied +`LIMIT maxKeys` with no `ORDER BY`, so on a column with more keys than the +limit the filter sidebar showed an arbitrary subset. They now order by +`sum(cardinality) DESC, key`, which keeps the most frequent keys, as the key +rollup path already does with `sum(count) DESC`. diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index ef33934124..a780102bd1 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -1411,4 +1411,106 @@ describe('Metadata Integration Tests', () => { expect(keys).toEqual(['legacy.key']); }); }); + + describe.each([ + { + path: 'key', + tableName: 'test_map_keys_key_text_index_order', + indexDefinition: `INDEX idx_log_attr_key mapKeys(LogAttributes) TYPE text(tokenizer = 'array') GRANULARITY 1`, + }, + { + path: 'kv', + tableName: 'test_map_keys_kv_text_index_order', + indexDefinition: `LogAttributeItems Array(String) MATERIALIZED + arrayMap(x -> concat(x.1, '=', x.2), CAST(LogAttributes, 'Array(Tuple(String, String))')), + INDEX idx_log_attr_items LogAttributeItems TYPE text(tokenizer = 'array') GRANULARITY 1`, + }, + ])( + 'getMapKeys - $path text index ordering', + ({ tableName, indexDefinition }) => { + let metadata: Metadata; + let textIndexSupported = false; + + beforeAll(async () => { + const probe = new Metadata(hdxClient, new MetadataCache()); + textIndexSupported = supportsMergeTreeTextIndex( + await probe.getServerVersion({ connectionId: 'test_connection' }), + ); + if (!textIndexSupported) return; + + await client.command({ + query: `CREATE OR REPLACE TABLE default.${tableName} ( + Timestamp DateTime64(9), + LogAttributes Map(LowCardinality(String), String), + ${indexDefinition} + ) + ENGINE = MergeTree() + PARTITION BY toDate(Timestamp) + ORDER BY Timestamp + `, + }); + + // The text index dictionary is sorted, so without an ORDER BY the + // LIMIT keeps the alphabetically first keys. The frequent keys sort + // after the 200 one-off keys, and in reverse order of frequency. + const insert = (attributes: string, rows: number) => + client.command({ + query: `INSERT INTO default.${tableName} (Timestamp, LogAttributes) + SELECT now64(9) - INTERVAL 10 MINUTE, ${attributes} + FROM numbers(${rows})`, + }); + await insert(`map(concat('rare.', toString(number)), 'v')`, 200); + await insert( + `map('user.id', 'x', 'trace.flags', 'x', 'service.name', 'x')`, + 40, + ); + await insert(`map('user.id', 'x', 'trace.flags', 'x')`, 10); + await insert(`map('user.id', 'x')`, 10); + }); + + afterAll(async () => { + await client.command({ + query: `DROP TABLE IF EXISTS default.${tableName}`, + }); + }); + + beforeEach(() => { + metadata = new Metadata(hdxClient, new MetadataCache()); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('keeps the most frequent keys when maxKeys truncates the list', async () => { + if (!textIndexSupported) { + console.warn( + 'Skipping: ClickHouse < 26.3 does not support mergeTreeTextIndex()', + ); + return; + } + const querySpy = jest.spyOn(hdxClient, 'query'); + + const keys = await metadata.getMapKeys({ + databaseName: 'default', + tableName, + column: 'LogAttributes', + connectionId: 'test_connection', + maxKeys: 3, + timestampValueExpression: 'Timestamp', + dateRange: [ + new Date(Date.now() - 60 * 60 * 1000), + new Date(Date.now() + 60 * 1000), + ], + }); + + expect( + querySpy.mock.calls.some(([{ query }]) => + query.includes('mergeTreeTextIndex'), + ), + ).toBe(true); + expect(keys).toEqual(['user.id', 'trace.flags', 'service.name']); + }); + }, + ); }); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index f3050f662e..d00818f0e9 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -2734,6 +2734,84 @@ describe('Metadata', () => { }); }); + describe('getMapKeys (text index paths)', () => { + const textIndexLookups: [string, TextIndexInfoLookup][] = [ + [ + 'key', + new Map([ + [ + 'LogAttributes', + { + key: { + indexName: 'idx_log_attr_keys', + mapColumn: 'LogAttributes', + }, + }, + ], + ]), + ], + [ + 'kv', + new Map([ + [ + 'LogAttributes', + { + kv: { + columnName: 'LogAttributeItems', + indexName: 'idx_log_attr_items', + separator: '=', + useHasAny: true, + mapColumn: 'LogAttributes', + }, + }, + ], + ]), + ], + ]; + + afterEach(() => { + jest.restoreAllMocks(); + (mockClickhouseClient.query as jest.Mock).mockReset(); + }); + + it.each(textIndexLookups)( + 'ranks keys by row count before the LIMIT on the %s text index path', + async (_, textIndexLookup) => { + const md = new Metadata(mockClickhouseClient, new MetadataCache()); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(textIndexLookup); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => + Promise.resolve({ data: [{ key: 'http.method' }, { key: 'k8s' }] }), + }); + + const keys = await md.getMapKeys({ + databaseName: 'default', + tableName: 'otel_logs', + column: 'LogAttributes', + connectionId: 'test_connection', + maxKeys: 2, + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-01T01:00:00Z'), + ], + timestampValueExpression: 'Timestamp', + }); + + expect(keys).toEqual(['http.method', 'k8s']); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1); + const { query } = (mockClickhouseClient.query as jest.Mock).mock + .calls[0][0]; + expect(query).toContain('FROM mergeTreeTextIndex('); + expect(query).toMatch( + /GROUP BY key HAVING key != ''\s+ORDER BY sum\(cardinality\) DESC, key\s+LIMIT/, + ); + }, + ); + }); + describe('getMapValues', () => { const buildMetadata = () => { const realCache = new ( diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index dcfa524535..662fa5cba7 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1017,11 +1017,13 @@ export class Metadata { timestampValueExpression, }); const index = textIndexInfo.key.indexName; + // `cardinality` is the token's row count in the part, not a distinct count. const sql = chSql` SELECT token AS key FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: index }}) WHERE ${partsFilter} GROUP BY key HAVING key != '' + ORDER BY sum(cardinality) DESC, key LIMIT ${{ Int32: maxKeys }}`; try { const keys = await this.clickhouseClient @@ -1063,11 +1065,13 @@ export class Metadata { }); const index = textIndexInfo.kv.indexName; const separator = textIndexInfo.kv.separator; + // `cardinality` is the token's row count in the part, not a distinct count. const sql = chSql` SELECT splitByString(${{ String: separator }}, token)[1] AS key FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: index }}) WHERE ${partsFilter} GROUP BY key HAVING key != '' + ORDER BY sum(cardinality) DESC, key LIMIT ${{ Int32: maxKeys }}`; try { const keys = await this.clickhouseClient