From 8dc5e0d29a9266291692d39a4c209273379e4661 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:36:44 -0400 Subject: [PATCH 01/24] feat: add intelligent router for getAllKeyValues; use text index to power filters --- .../src/components/DBSearchPageFilters.tsx | 3 +- .../components/DBSearchPageFilters/hooks.ts | 129 +-- .../app/src/hooks/useAutoCompleteOptions.tsx | 18 + packages/app/src/hooks/useMetadata.tsx | 71 +- .../src/__tests__/metadata.int.test.ts | 181 ---- .../src/__tests__/metadata.test.ts | 109 --- .../rewriteSqlFilterWithKvItems.test.ts | 47 +- .../src/core/clickhouseVersion.ts | 17 + packages/common-utils/src/core/metadata.ts | 861 +++++++++++++----- .../src/core/renderChartConfig.ts | 22 +- packages/common-utils/src/core/utils.ts | 27 + packages/common-utils/src/queryParser.ts | 222 +++-- 12 files changed, 936 insertions(+), 771 deletions(-) diff --git a/packages/app/src/components/DBSearchPageFilters.tsx b/packages/app/src/components/DBSearchPageFilters.tsx index 9a7feddcd2..67a154aae7 100644 --- a/packages/app/src/components/DBSearchPageFilters.tsx +++ b/packages/app/src/components/DBSearchPageFilters.tsx @@ -1171,7 +1171,7 @@ const DBSearchPageFiltersComponent = ({ const [showMoreFields, setShowMoreFields] = useState(false); const { - data: facets, + data: fetchFacetsData, isLoading: isFacetsLoading, isFetching: isFacetsFetching, error, @@ -1186,6 +1186,7 @@ const DBSearchPageFiltersComponent = ({ filterState, showMoreFields, }); + const facets = fetchFacetsData.keyValues; useEffect(() => { if (error) { diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index 84fbe8b245..c756edf0fd 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -7,15 +7,9 @@ import { } from '@hyperdx/common-utils/dist/filters'; import { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; -import api from '@/api'; -import { - DEFAULT_FILTER_KEYS_FETCH_LIMIT, - DEFAULT_FILTER_KEYS_FETCH_LIMIT_WITH_MVS, -} from '@/defaults'; import { Facet, useAllFields, - useAllFieldsAndValues, useColumns, useDateTimeColumns, useGetKeyValues, @@ -34,7 +28,7 @@ const INITIAL_LOAD_LIMIT = 20; /* The maximum number of values per filter to load when "Load More" is clicked */ const LOAD_MORE_LOAD_LIMIT = 10000; -function useFacetsFromRawTables({ +function useFacets({ chartConfig, sourceId, mode, @@ -151,12 +145,12 @@ function useFacetsFromRawTables({ [chartConfig, dateRange, mode], ); - // Exact pipeline step 2: fetch values for discovered keys const { data: rawExactFacets, ...rest } = useGetKeyValues( { chartConfig: facetsChartConfig, limit: INITIAL_LOAD_LIMIT, keys: escapedKeysToFetch, + mode, }, { enabled }, ); @@ -216,72 +210,12 @@ function useFacetsFromRawTables({ return { ...rest, error: allFieldsError ?? rest.error, - data: exactFacets, + data: { keys: allFields, keyValues: exactFacets }, isLoading: isAllFieldsLoading || rest.isLoading, loadMoreFacetsForKey, }; } -function useAllFacetsFromMVs({ - sourceId, - dateRange, - enabled, -}: { - sourceId: string | null; - dateRange: [Date, Date]; - enabled?: boolean; -}) { - const { data: source } = useSource({ - id: sourceId, - }); - const tableConnection = tcFromSource(source); - const hasMVs = tableConnection && !!tableConnection.metadataMVs; - const { data: me } = api.useMe(); - const defaultLimit = hasMVs - ? DEFAULT_FILTER_KEYS_FETCH_LIMIT_WITH_MVS - : DEFAULT_FILTER_KEYS_FETCH_LIMIT; - const maxKeys = me?.team?.filterKeysFetchLimit ?? defaultLimit; - const { data: columns } = useColumns(tableConnection); - const knownColumns = useMemo( - () => (columns ? new Set(columns.map(c => c.name)) : new Set()), - [columns], - ); - - const metadata = useMetadataWithSettings(); - const loadMoreFacetsForKey = useCallback( - async (key: string) => { - try { - const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns); - const results = await metadata.getAllKeyValues({ - databaseName: tableConnection.databaseName, - tableName: tableConnection.tableName, - connectionId: tableConnection.connectionId, - keyExpressions: [sqlKey], - maxValuesPerKey: LOAD_MORE_LOAD_LIMIT, - metadataMVs: tableConnection.metadataMVs, - dateRange, - timestampValueExpression: source?.timestampValueExpression, - }); - const newValues = results[0] ? results[0].value : []; - return { key, value: newValues }; - } catch (error) { - console.error('failed to fetch more keys via MV facets', error); - } - }, - [knownColumns, dateRange, metadata, tableConnection, source], - ); - - const queryRes = useAllFieldsAndValues( - { - ...tableConnection, - dateRange, - maxKeys, - }, - { enabled }, - ); - return { ...queryRes, loadMoreFacetsForKey }; -} - export function useFetchFacets({ chartConfig, sourceId, @@ -297,33 +231,20 @@ export function useFetchFacets({ filterState?: FilterState; showMoreFields?: boolean; }) { - const { data: source } = useSource({ - id: sourceId, - }); - const tableConnection = tcFromSource(source); - const hasMVs = !!tableConnection.metadataMVs; - const useRawTablePipeline = !hasMVs || mode === 'exact'; - - const fromMVs = useAllFacetsFromMVs({ - sourceId, - dateRange, - enabled: !useRawTablePipeline, - }); - // Exact pipeline: fetch values for discovered keys - const fromRawTables = useFacetsFromRawTables({ + const facetsQuery = useFacets({ chartConfig, sourceId, mode, dateRange, filterState, showMoreFields, - enabled: useRawTablePipeline, + enabled: true, }); const [extraFacets, setExtraFacets] = useState(null); const facets = useMemo(() => { - const facets = useRawTablePipeline ? fromRawTables.data : fromMVs.data; + const facets = facetsQuery.data.keyValues; if (!facets) return undefined; if (!extraFacets || extraFacets.length === 0) return facets; const seenFacets = new Set(); @@ -353,18 +274,16 @@ export function useFetchFacets({ } } return output; - }, [fromMVs.data, fromRawTables.data, useRawTablePipeline, extraFacets]); + }, [facetsQuery.data, extraFacets]); const [extraFacetKeys, setExtraFacetKeys] = useState>(new Set()); const [loadMoreLoadingKeys, setLoadMoreLoadingKeys] = useState>( new Set(), ); - const extraFacetsLoading = loadMoreLoadingKeys.size > 0; + const areExtraFacetsLoading = loadMoreLoadingKeys.size > 0; const loadMoreFacetsForKey = useCallback( async (key: string) => { - const strategy = useRawTablePipeline - ? fromRawTables.loadMoreFacetsForKey - : fromMVs.loadMoreFacetsForKey; + const strategy = facetsQuery.loadMoreFacetsForKey; setLoadMoreLoadingKeys(prev => produce(prev, draft => { draft.add(key); @@ -385,11 +304,7 @@ export function useFetchFacets({ }), ); }, - [ - fromRawTables.loadMoreFacetsForKey, - fromMVs.loadMoreFacetsForKey, - useRawTablePipeline, - ], + [facetsQuery.loadMoreFacetsForKey], ); // Clear extras when the query scope that produced them changes; otherwise @@ -397,7 +312,6 @@ export function useFetchFacets({ useEffect(() => { // eslint-disable-next-line react-hooks/set-state-in-effect setExtraFacets(null); - setExtraFacetKeys(new Set()); }, [ sourceId, @@ -408,25 +322,12 @@ export function useFetchFacets({ chartConfig.whereLanguage, ]); - const output = useMemo(() => { - return { - ...(useRawTablePipeline ? fromRawTables : fromMVs), - data: facets, - loadMoreFacetsForKey: loadMoreFacetsForKey, - areExtraFacetsLoading: extraFacetsLoading, - loadMoreLoadingKeys, - extraFacetKeys, - }; - }, [ - useRawTablePipeline, - fromMVs, - fromRawTables, - facets, + return { + ...facetsQuery, + data: { keys: facetsQuery.data?.keys, keyValues: facets }, loadMoreFacetsForKey, - extraFacetsLoading, + areExtraFacetsLoading, loadMoreLoadingKeys, extraFacetKeys, - ]); - - return output; + }; } diff --git a/packages/app/src/hooks/useAutoCompleteOptions.tsx b/packages/app/src/hooks/useAutoCompleteOptions.tsx index 0dbde75e0e..05c7eccf40 100644 --- a/packages/app/src/hooks/useAutoCompleteOptions.tsx +++ b/packages/app/src/hooks/useAutoCompleteOptions.tsx @@ -185,6 +185,24 @@ export function useAutoCompleteOptions( ); // Fetch fields, using rollup for map key discovery when available + + // AVK: TODO use useFetchFacets here instead of useMultipleAllFields and useGetKeyValues + // const { + // data: fetchFacetsData, + // isLoading: isFacetsLoading, + // isFetching: isFacetsFetching, + // error, + // loadMoreFacetsForKey, + // loadMoreLoadingKeys, + // extraFacetKeys, + // } = useFetchFacets({ + // chartConfig, + // sourceId: sourceId ?? null, + // dateRange, + // mode: showAllValues ? 'all' : 'exact', + // filterState, + // showMoreFields, + // }); const { data: fields } = useMultipleAllFields(tcs, { dateRange: effectiveDateRange, timestampValueExpression: source?.timestampValueExpression, diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index 46198a4921..7a87f2ab29 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -383,7 +383,7 @@ export function useMultipleGetKeyValues( return metadata.getAllKeyValues({ databaseName, tableName, - keyExpressions: keys.slice(0, maxKeys), + keyExpressions: keys, connectionId, metadataMVs, dateRange, @@ -489,75 +489,6 @@ export function useGetKeyValues( ); } -/** - * Combined key + value discovery in a single rollup query. - * Returns all fields and their top N values without needing a separate - * useAllFields + useGetKeyValues chain. - */ -export function useAllFieldsAndValues( - { - databaseName, - tableName, - connectionId, - metadataMVs, - dateRange, - maxValuesPerKey, - maxKeys, - }: { - databaseName: string; - tableName: string; - connectionId: string; - metadataMVs?: MetadataMaterializedViews; - dateRange?: [Date, Date]; - maxValuesPerKey?: number; - maxKeys?: number; - }, - options?: Omit, 'queryKey'>, -) { - const metadata = useMetadataWithSettings(); - const { data: me } = api.useMe(); - const { enabled = true } = options || {}; - const fieldMetadataDisabled = !!me?.team?.fieldMetadataDisabled; - - return useQuery({ - queryKey: [ - 'useMetadata.useAllFieldsAndValues', - databaseName, - tableName, - connectionId, - metadataMVs, - dateRange?.[0]?.getTime(), - dateRange?.[1]?.getTime(), - maxValuesPerKey, - maxKeys, - ], - queryFn: async ({ signal }) => { - if (fieldMetadataDisabled) { - return []; - } - return metadata.getAllFieldsAndValues({ - databaseName, - tableName, - connectionId, - metadataMVs, - dateRange, - maxValuesPerKey, - maxKeys, - signal, - }); - }, - staleTime: 1000 * 60 * 5, - placeholderData: keepPreviousData, - ...options, - enabled: - !!enabled && - !fieldMetadataDisabled && - !!databaseName && - !!tableName && - !!connectionId, - }); -} - export function deduplicate2dArray(array2d: T[][]): T[] { // deduplicate common fields const array: T[] = []; diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 3676064b2d..668de7f950 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -776,185 +776,4 @@ describe('Metadata Integration Tests', () => { expect(parsed!.separator).toBe('='); }); }); - - describe('getAllFieldsAndValues', () => { - let metadata: Metadata; - const kvRollupTableName = 'test_kv_rollup'; - - const metadataMVs = { - keyRollupTable: 'test_key_rollup', // not used by this method - kvRollupTable: kvRollupTableName, - granularity: '1 minute', - }; - - beforeAll(async () => { - // Create KV rollup table matching the schema getAllFieldsAndValues expects - await client.command({ - query: `CREATE OR REPLACE TABLE default.${kvRollupTableName} ( - Timestamp DateTime CODEC(Delta, ZSTD(1)), - ColumnIdentifier String CODEC(ZSTD(1)), - Key String CODEC(ZSTD(1)), - Value String CODEC(ZSTD(1)), - count UInt64 - ) - ENGINE = SummingMergeTree() - ORDER BY (Timestamp, ColumnIdentifier, Key, Value) - `, - }); - - // Insert sample data: native columns + map sub-fields - await client.command({ - query: `INSERT INTO default.${kvRollupTableName} - (Timestamp, ColumnIdentifier, Key, Value, count) VALUES - ('2024-01-10 12:00:00', 'NativeColumn', 'SeverityText', 'info', 10), - ('2024-01-10 12:00:00', 'NativeColumn', 'SeverityText', 'error', 5), - ('2024-01-10 12:00:00', 'NativeColumn', 'SeverityText', 'warning', 2), - ('2024-01-10 12:00:00', 'NativeColumn', 'SeverityText', '', 1), - ('2024-01-10 12:00:00', 'NativeColumn', 'ServiceName', 'api', 8), - ('2024-01-10 12:00:00', 'NativeColumn', 'ServiceName', 'web', 6), - ('2024-01-10 12:00:00', 'ResourceAttributes', 'env', 'prod', 12), - ('2024-01-10 12:00:00', 'ResourceAttributes', 'env', 'staging', 3), - ('2024-01-10 12:00:00', 'ResourceAttributes', 'region', 'us-east', 7), - ('2024-01-10 12:00:00', 'ResourceAttributes', 'region', 'eu-west', 4) - `, - }); - }); - - beforeEach(() => { - metadata = new Metadata(hdxClient, new MetadataCache()); - }); - - afterAll(async () => { - await client.command({ - query: `DROP TABLE IF EXISTS default.${kvRollupTableName}`, - }); - }); - - it('should return all keys and values from the KV rollup table', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: [new Date('2024-01-01'), new Date('2024-01-31')], - }); - - // Should have 4 keys: SeverityText, ServiceName, ResourceAttributes['env'], ResourceAttributes['region'] - expect(result).toHaveLength(4); - - // Native columns use bare key names - const severity = result.find(r => r.key === 'SeverityText'); - expect(severity).toBeDefined(); - expect(severity!.value).toEqual( - expect.arrayContaining(['info', 'error', 'warning']), - ); - // Empty values should be excluded - expect(severity!.value).not.toContain(''); - - const service = result.find(r => r.key === 'ServiceName'); - expect(service).toBeDefined(); - expect(service!.value).toEqual(expect.arrayContaining(['api', 'web'])); - - // Map sub-fields use bracket notation - const env = result.find(r => r.key === "ResourceAttributes['env']"); - expect(env).toBeDefined(); - expect(env!.value).toEqual(expect.arrayContaining(['prod', 'staging'])); - - const region = result.find(r => r.key === "ResourceAttributes['region']"); - expect(region).toBeDefined(); - expect(region!.value).toEqual( - expect.arrayContaining(['us-east', 'eu-west']), - ); - }); - - it('should return native columns before map sub-fields', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: [new Date('2024-01-01'), new Date('2024-01-31')], - }); - - // NativeColumn rows are ordered first by the ORDER BY clause - const nativeKeys = result - .filter(r => !r.key.includes('[')) - .map(r => r.key); - const mapKeys = result.filter(r => r.key.includes('[')).map(r => r.key); - - // All native keys should come before map keys - const lastNativeIdx = Math.max( - ...nativeKeys.map(k => result.findIndex(r => r.key === k)), - ); - const firstMapIdx = Math.min( - ...mapKeys.map(k => result.findIndex(r => r.key === k)), - ); - expect(lastNativeIdx).toBeLessThan(firstMapIdx); - }); - - it('should respect maxKeys limit', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: [new Date('2024-01-01'), new Date('2024-01-31')], - maxKeys: 2, - }); - - expect(result).toHaveLength(2); - }); - - it('should respect maxValuesPerKey limit', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: [new Date('2024-01-01'), new Date('2024-01-31')], - maxValuesPerKey: 1, - }); - - // Each key should have at most 1 value - for (const entry of result) { - expect(entry.value.length).toBeLessThanOrEqual(1); - } - }); - - it('should return empty array when metadataMVs is undefined', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs: undefined, - dateRange: [new Date('2024-01-01'), new Date('2024-01-31')], - }); - - expect(result).toEqual([]); - }); - - it('should return empty array when dateRange is undefined', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: undefined, - }); - - expect(result).toEqual([]); - }); - - it('should return empty array when date range has no matching data', async () => { - const result = await metadata.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'unused_base_table', - connectionId: 'test_connection', - metadataMVs, - dateRange: [new Date('2025-06-01'), new Date('2025-06-30')], - }); - - expect(result).toEqual([]); - }); - }); }); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 166d66ab1f..93db570a75 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1815,37 +1815,6 @@ describe('parametric aggregate arguments are inlined as literals', () => { (mockClickhouseClient.query as jest.Mock).mockReset(); }); - it('emits groupUniqArray(N)(Value) with a literal N — not a CAST-wrapped query parameter', async () => { - const md = buildMetadata(); - - (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ - json: () => Promise.resolve({ data: [] }), - }); - - await md.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'otel_logs', - connectionId: 'conn-1', - metadataMVs: { - keyRollupTable: 'otel_logs_key_rollup_15m', - kvRollupTable: 'otel_logs_kv_rollup_15m', - granularity: '15 minute', - }, - dateRange: [ - new Date('2026-05-11T16:00:00Z'), - new Date('2026-05-11T17:00:00Z'), - ], - maxValuesPerKey: 20, - }); - - const call = (mockClickhouseClient.query as jest.Mock).mock.calls[0][0]; - expect(call.query).toContain('groupUniqArray(20)(Value)'); - expect(call.query).not.toMatch( - /groupUniqArray\(\{[^}]+:Int32\}\)\(Value\)/, - ); - expect(Object.values(call.query_params)).not.toContain(20); - }); - it('emits groupUniqArrayArray(N)(keys) with a literal N in the sampledKeys query', async () => { const md = buildMetadata(); @@ -1898,33 +1867,6 @@ describe('parametric aggregate arguments are inlined as literals', () => { ['Infinity', Number.POSITIVE_INFINITY], ]; - it.each(badValues)( - 'getAllFieldsAndValues throws when maxValuesPerKey is %s and never queries ClickHouse', - async (_label, badValue) => { - const md = buildMetadata(); - - await expect( - md.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'otel_logs', - connectionId: 'conn-1', - metadataMVs: { - keyRollupTable: 'otel_logs_key_rollup_15m', - kvRollupTable: 'otel_logs_kv_rollup_15m', - granularity: '15 minute', - }, - dateRange: [ - new Date('2026-05-11T16:00:00Z'), - new Date('2026-05-11T17:00:00Z'), - ], - maxValuesPerKey: badValue as number, - }), - ).rejects.toThrow(/maxValuesPerKey must be a non-negative integer/); - - expect(mockClickhouseClient.query).not.toHaveBeenCalled(); - }, - ); - it.each(badValues)( 'getMapKeys throws when maxKeys is %s and never runs the sampledKeys query', async (_label, badValue) => { @@ -1963,56 +1905,5 @@ describe('parametric aggregate arguments are inlined as literals', () => { expect(onlyCall.query).not.toContain('groupUniqArrayArray'); }, ); - - it('error message includes the offending value for easy debugging', async () => { - const md = buildMetadata(); - - await expect( - md.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'otel_logs', - connectionId: 'conn-1', - metadataMVs: { - keyRollupTable: 'otel_logs_key_rollup_15m', - kvRollupTable: 'otel_logs_kv_rollup_15m', - granularity: '15 minute', - }, - dateRange: [ - new Date('2026-05-11T16:00:00Z'), - new Date('2026-05-11T17:00:00Z'), - ], - maxValuesPerKey: 'oops' as unknown as number, - }), - ).rejects.toThrow('got: oops'); - }); - - it('accepts the valid boundary case maxValuesPerKey=0 (degenerate but well-formed)', async () => { - const md = buildMetadata(); - - (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ - json: () => Promise.resolve({ data: [] }), - }); - - await expect( - md.getAllFieldsAndValues({ - databaseName: 'default', - tableName: 'otel_logs', - connectionId: 'conn-1', - metadataMVs: { - keyRollupTable: 'otel_logs_key_rollup_15m', - kvRollupTable: 'otel_logs_kv_rollup_15m', - granularity: '15 minute', - }, - dateRange: [ - new Date('2026-05-11T16:00:00Z'), - new Date('2026-05-11T17:00:00Z'), - ], - maxValuesPerKey: 0, - }), - ).resolves.toEqual([]); - - const call = (mockClickhouseClient.query as jest.Mock).mock.calls[0][0]; - expect(call.query).toContain('groupUniqArray(0)(Value)'); - }); }); }); diff --git a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts index 6f63366396..fb81d16a91 100644 --- a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts +++ b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts @@ -1,26 +1,38 @@ import { rewriteSqlFilterWithKvItems } from '@/core/renderChartConfig'; -import { KvItemsLookup } from '@/queryParser'; +import { TextIndexInfoLookup } from '@/queryParser'; type PartialKvItemsInfo = { - kvItemsColumn: string; + columnName: string; separator: string; useHasAny?: boolean; }; const makeLookup = ( entries: Array<[string, PartialKvItemsInfo]>, -): KvItemsLookup => - new Map(entries.map(([k, v]) => [k, { useHasAny: true, ...v }])); - -const defaultLookup: KvItemsLookup = makeLookup([ - ['LogAttributes', { kvItemsColumn: 'LogAttributeItems', separator: '=' }], +): TextIndexInfoLookup => + new Map( + entries.map(([mapColumn, v]) => [ + mapColumn, + { + kv: { + useHasAny: true, + indexName: `${v.columnName}_idx`, + mapColumn, + ...v, + }, + }, + ]), + ); + +const defaultLookup: TextIndexInfoLookup = makeLookup([ + ['LogAttributes', { columnName: 'LogAttributeItems', separator: '=' }], ]); -const legacyLookup: KvItemsLookup = makeLookup([ +const legacyLookup: TextIndexInfoLookup = makeLookup([ [ 'LogAttributes', { - kvItemsColumn: 'LogAttributeItems', + columnName: 'LogAttributeItems', separator: '=', useHasAny: false, }, @@ -250,10 +262,7 @@ describe('rewriteSqlFilterWithKvItems', () => { describe('lookup configuration', () => { it('uses the configured separator in the rewritten concat', () => { const colonLookup = makeLookup([ - [ - 'LogAttributes', - { kvItemsColumn: 'LogAttributeItems', separator: ':' }, - ], + ['LogAttributes', { columnName: 'LogAttributeItems', separator: ':' }], ]); const result = rewriteSqlFilterWithKvItems( "LogAttributes['k'] = 'v'", @@ -264,10 +273,7 @@ describe('rewriteSqlFilterWithKvItems', () => { it('uses the configured kv items column name (backtick-quoted)', () => { const lookup = makeLookup([ - [ - 'LogAttributes', - { kvItemsColumn: 'CustomItemsColumn', separator: '=' }, - ], + ['LogAttributes', { columnName: 'CustomItemsColumn', separator: '=' }], ]); const result = rewriteSqlFilterWithKvItems( "LogAttributes['k'] = 'v'", @@ -278,13 +284,10 @@ describe('rewriteSqlFilterWithKvItems', () => { it('applies independent lookup entries to each map column', () => { const lookup = makeLookup([ - [ - 'LogAttributes', - { kvItemsColumn: 'LogAttributeItems', separator: '=' }, - ], + ['LogAttributes', { columnName: 'LogAttributeItems', separator: '=' }], [ 'ResourceAttributes', - { kvItemsColumn: 'ResourceAttributeItems', separator: ':' }, + { columnName: 'ResourceAttributeItems', separator: ':' }, ], ]); const result = rewriteSqlFilterWithKvItems( diff --git a/packages/common-utils/src/core/clickhouseVersion.ts b/packages/common-utils/src/core/clickhouseVersion.ts index 7fe2fbb428..05226ec2c7 100644 --- a/packages/common-utils/src/core/clickhouseVersion.ts +++ b/packages/common-utils/src/core/clickhouseVersion.ts @@ -139,3 +139,20 @@ export function supportsDirectReadMap( if (!branchMin) return false; return compareClickHouseVersion(version, branchMin) >= 0; } + +/** + * First release that shipped the `mergeTreeTextIndex(database, table, index)` + * table function used to introspect text skip indices. + */ +const MERGE_TREE_TEXT_INDEX_MIN: ClickHouseVersion = [26, 3, 0, 0]; + +/** + * Returns true when the connected ClickHouse server supports the + * `mergeTreeTextIndex` table function (>= 26.3). Returns false when the + * version is undefined or older. + */ +export function supportsMergeTreeTextIndex( + version: ClickHouseVersion | undefined, +): boolean { + return isClickHouseVersionAtLeast(version, MERGE_TREE_TEXT_INDEX_MIN); +} diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 454c38ce52..13cd65d93a 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -13,6 +13,13 @@ import { tableExpr, } from '@/clickhouse'; import { renderChartConfig, timeFilterExpr } from '@/core/renderChartConfig'; +import { + buildTextIndexInfoLookup, + KvIndexInfo, + skipIndexMatches, + TextIndexInfo, + TextIndexInfoLookup, +} from '@/queryParser'; import type { BuilderChartConfig, BuilderChartConfigWithDateRange, @@ -29,7 +36,10 @@ import { import { getAlignedDateRange, getDistributedTableArgs, + MetadataMVQueryOptions, objectHash, + TextIndexColumnQueryOptions, + TextIndexMapColumnQueryOptions, } from './utils'; // If filters initially are taking too long to load, decrease this number. @@ -37,6 +47,25 @@ import { export const DEFAULT_METADATA_MAX_ROWS_TO_READ = 3e6; const DEFAULT_MAX_KEYS = 1000; +type KeyFetchingStrategies = { + mapTextIndexLookup: TextIndexInfo[]; + nativeTextIndexLookup: SkipIndexMetadata[]; + metadataMVs: { columnName: string; mvName: string }[]; + rawTable: string[]; +}; + +export type KeyValues = { + key: string; + value: string[] | number[]; +}; + +function addMapTextIndexEntry( + kvIndexInfo: KvIndexInfo, + column: string, + key: string, + map: TextIndexMapColumnQueryOptions, +) {} + // See https://github.com/hyperdxio/hyperdx/issues/2163. Inlining a validated // integer literal avoids the `_CAST` wrapper entirely. const inlineNonNegativeInt = (value: number, label: string): string => { @@ -139,6 +168,11 @@ export class MetadataCache { // TODO: Shard cache by time } +export type MapColumnTextIndexes = { + keysIndex?: { indexName: string }; + itemsIndex?: { indexName: string; separator: string }; +}; + export type TableMetadata = { database: string; name: string; @@ -466,6 +500,79 @@ export class Metadata { })[0]; } + async getMapColumnTextIndexes({ + databaseName, + tableName, + connectionId, + }: TableConnection) { + return this.cache.getOrFetch( + `${connectionId}.${databaseName}.${tableName}.mapColumnTextIndexes`, + async () => { + return buildTextIndexInfoLookup({ + metadata: this, + databaseName: databaseName, + tableName: tableName, + connectionId: connectionId, + }); + }, + ); + } + + async getNativeArrayColumnTextIndexes({ + databaseName, + tableName, + connectionId, + }: TableConnection): Promise> { + return this.cache.getOrFetch( + `${connectionId}.${databaseName}.${tableName}.nativeColumnTextIndexes`, + async () => { + const [columns, skipIndices] = await Promise.all([ + this.getColumns({ databaseName, tableName, connectionId }), + this.getSkipIndices({ + databaseName, + tableName, + connectionId, + }).catch(() => [] as SkipIndexMetadata[]), + ]); + + /** Map from map column name to its text index info */ + const indices: Map = new Map(); + for (const idx of skipIndices) { + if ( + skipIndexMatches(idx, 'text', { tokenizer: 'array' }) && + columns.some(col => col.name === idx.expression) + ) { + indices.set(idx.expression, idx); + } + } + return indices; + }, + ); + } + + private async partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }: { + databaseName: string; + tableName: string; + dateRange?: [Date, Date]; + timestampValueExpression?: string; + }): Promise { + if (!dateRange || !timestampValueExpression) return chSql`1`; + const startTime = chSql`fromUnixTimestamp64Milli(${{ Int64: dateRange[0].getTime() }})`; + const endTime = chSql`fromUnixTimestamp64Milli(${{ Int64: dateRange[1].getTime() }})`; + return chSql`part_name IN ( + SELECT name + FROM system.parts + WHERE database = ${{ String: databaseName }} AND table = ${{ String: tableName }} + AND active=1 + AND ((min_time >= ${startTime} AND min_time <= ${endTime}) OR (max_time <= ${endTime} AND max_time >= ${startTime}) OR (min_time <= ${startTime} AND max_time >= ${endTime})) + )`; + } + async getMapKeys({ databaseName, tableName, @@ -510,6 +617,70 @@ export class Metadata { return cachedKeys; } + const textIndexInfoLookup = await this.getMapColumnTextIndexes({ + databaseName, + tableName, + connectionId, + }); + + // Text Index path: query the key rollup index + const textIndexInfo = textIndexInfoLookup.get(column); + if (textIndexInfo?.key?.indexName) { + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const index = textIndexInfo.key.indexName; + const sql = chSql` + SELECT token AS key + FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: index }}) + WHERE ${partsFilter} + GROUP BY key HAVING key != '' + LIMIT ${{ Int32: maxKeys }}`; + const keys = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(r => r.json<{ key: string }>()) + .then(d => d.data.map(r => r.key).filter(Boolean)); + if (keys.length > 0) { + this.cache.set(cacheKey, keys); + return keys; + } + } else if (textIndexInfo?.kv?.indexName) { + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const index = textIndexInfo.kv.indexName; + const sql = chSql` + SELECT splitByString(${{ String: index }}, token)[1] AS key + FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: index }}) + WHERE ${partsFilter} + GROUP BY key HAVING key != '' + LIMIT ${{ Int32: maxKeys }}`; + const keys = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(r => r.json<{ key: string }>()) + .then(d => d.data.map(r => r.key).filter(Boolean)); + if (keys.length > 0) { + this.cache.set(cacheKey, keys); + return keys; + } + } + // Rollup path: query the key rollup table filtered by ColumnIdentifier and date range if (metadataMVs && alignedDateRange) { const rollupKeys = await this.cache.getOrFetch( @@ -885,6 +1056,239 @@ export class Metadata { }); } + private async getMapTextIndexValues({ + databaseName, + tableName, + connectionId, + queryOptions, + dateRange, + timestampValueExpression, + signal, + }: TableConnection & { + queryOptions: TextIndexMapColumnQueryOptions; + dateRange: [Date, Date]; + timestampValueExpression: string; + signal?: AbortSignal; + }): Promise { + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getMapTextIndexValues`; + return this.cache.getOrFetch(cacheKey, async () => { + const sqlBranches: Array = []; + for (const [columnName, info] of queryOptions.entries()) { + const orChain = concatChSql( + ' OR ', + info.keys.map( + k => + chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`, + ), + ); + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; + 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 + FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) + WHERE ${partsFilter} + AND (${orChain}) + AND ${valueSql} != '' + GROUP BY column, key + )`; + sqlBranches.push(sql); + } + const sql = concatChSql(' UNION ALL ', sqlBranches); + + return await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + 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(), + }, + abort_signal: signal, + }) + .then(res => + res.json<{ column: string; key: string; value: string[] }>(), + ) + .then(d => + d.data.map(row => ({ + key: `${row.column}['${row.key}']`, + value: row.value, + })), + ); + }); + } + + private async getTextIndexValues({ + databaseName, + tableName, + connectionId, + queryOptions, + dateRange, + timestampValueExpression, + signal, + }: TableConnection & { + queryOptions: TextIndexColumnQueryOptions; + dateRange: [Date, Date]; + timestampValueExpression: string; + signal?: AbortSignal; + }): Promise { + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getTextIndexValues`; + return this.cache.getOrFetch(cacheKey, async () => { + const sqlBranches: Array = []; + for (const [columnName, info] of queryOptions.entries()) { + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const sql = chSql` + SELECT * FROM ( + SELECT ${{ String: columnName }} AS key, + groupUniqArray(${{ Int32: info.limit }})(token) AS value + FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: info.indexName }}) + WHERE ${partsFilter} + AND token != '' + GROUP BY key + )`; + sqlBranches.push(sql); + } + const sql = concatChSql(' UNION ALL ', sqlBranches); + + const values = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + 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(), + }, + abort_signal: signal, + }) + .then(res => res.json()) + .then(d => d.data); + return values; + }); + } + + private async getMetadataMVKeyValues({ + databaseName, + connectionId, + dateRange, + metadataMVs, + queryOptions, + maxValuesPerKey, + signal, + }: TableConnection & { + queryOptions: MetadataMVQueryOptions; + dateRange: [Date, Date]; + maxValuesPerKey: number; + signal?: AbortSignal; + }): Promise { + const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(Array.from(queryOptions.entries()))}.getMetadataMVKeyValues`; + return this.cache.getOrFetch(cacheKey, async () => { + if (!metadataMVs) { + console.warn('getMetadataMVKeyValues: metadataMVs is undefined'); + return undefined; + } + + const alignedDateRange = getAlignedDateRange( + dateRange, + metadataMVs.granularity, + ); + const startExpr = renderStartOfBucketExpr( + metadataMVs.granularity, + chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[0].getTime() }})`, + ); + const endExpr = renderStartOfBucketExpr( + metadataMVs.granularity, + chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[1].getTime() }})`, + ); + const timeFilter = chSql`Timestamp >= ${startExpr} AND Timestamp <= ${endExpr}`; + + const sqlBranches: ChSql[] = []; + for (const [mvName, entry] of queryOptions.entries()) { + // this should only be one mv... but we have a for loop in case + const branch: ChSql[] = []; + for (const [columnName, keys] of entry) { + const sql = chSql`(ColumnIdentifier = ${{ String: columnName }} AND Key IN (${concatChSql( + ',', + keys.map(key => chSql`${{ String: key }}`), + )}))`; + branch.push(sql); + } + const sql = chSql` + SELECT * FROM ( + SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values, sum(count) as total_count + FROM ${tableExpr({ database: databaseName, table: mvName })} + WHERE ${concatChSql(' OR ', branch)} + AND ${timeFilter} + AND Value != '' + GROUP BY ColumnIdentifier, Key + ORDER BY ColumnIdentifier, Key, total_count DESC + LIMIT ${{ Int32: maxValuesPerKey }} BY ColumnIdentifier, Key + )`; + sqlBranches.push(sql); + } + const sql = concatChSql(' UNION ALL ', sqlBranches); + + type BatchRow = { + ColumnIdentifier: string; + Key: string; + Values: string[]; + total_count: number; + }; + + try { + return await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: { + ...this.getClickHouseSettings(), + timeout_overflow_mode: 'break', + max_execution_time: 15, + max_rows_to_read: '0', + }, + abort_signal: signal, + }) + .then(res => res.json()) + .then(d => + d.data.map(row => { + if (row.ColumnIdentifier === 'NativeColumn') { + return { key: row.Key, value: row.Values }; + } + return { + key: `${row.ColumnIdentifier}['${row.Key}']`, + value: row.Values, + }; + }), + ); + } catch (e) { + console.warn('Batched rollup query failed, falling back to per-key', e); + } + return undefined; + }); + } + async getAllFields({ databaseName, tableName, @@ -1051,6 +1455,28 @@ export class Metadata { return tableMetadata; } + async getAllTableMetadata({ + databaseName, + connectionId, + }: { + databaseName: string; + connectionId: string; + }) { + const cacheKey = `${connectionId}.${databaseName}.tableMetadata`; + return this.cache.getOrFetch(cacheKey, async () => { + const sql = chSql`SELECT * FROM system.tables WHERE database = ${{ String: databaseName }}`; + const json = await this.clickhouseClient + .query<'JSON'>({ + connectionId, + query: sql.sql, + query_params: sql.params, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(res => res.json()); + return json.data; + }); + } + /** Reads the value of the setting with the given name from system.settings. */ async getSetting({ settingName, @@ -1531,8 +1957,107 @@ export class Metadata { ); } + private async doMetadataMVsAggregateColumn( + { databaseName, tableName, connectionId }: TableConnection, + columnName: string, + ): Promise { + const allTableMetadata = await this.getAllTableMetadata({ + databaseName, + connectionId, + }); + for (const table of allTableMetadata) { + if ( + table.engine !== 'MaterializedView' || + !table.create_table_query.startsWith( + `CREATE MATERIALIZED VIEW ${databaseName}.${table.name} TO ${databaseName}.${tableName}`, + ) + ) { + continue; + } + return table.as_select.includes(columnName); + } + return false; + } + + private async determineKeyValueFetchingStrategy({ + databaseName, + tableName, + connectionId, + metadataMVs, + }: TableConnection): Promise { + return this.cache.getOrFetch( + `${connectionId}.${databaseName}.${tableName}.${metadataMVs}.determineKeyValueFetchingStrategy`, + async () => { + const columnMetadata = await this.getColumns({ + databaseName, + tableName, + connectionId, + }); + const mapTextIndexInfoLookup = await this.getMapColumnTextIndexes({ + databaseName, + tableName, + connectionId, + }); + const nativeTextIndexInfoLookup = + await this.getNativeArrayColumnTextIndexes({ + databaseName, + tableName, + connectionId, + }); + + const strategies: KeyFetchingStrategies = { + mapTextIndexLookup: [], + nativeTextIndexLookup: [], + metadataMVs: [], + rawTable: [], + }; + + for (const col of columnMetadata) { + if (col.name === 'Timestamp') continue; // ignore the timestamp column + // first check if this column is a map with a kv index + if (mapTextIndexInfoLookup.get(col.name)?.kv) { + strategies.mapTextIndexLookup.push( + mapTextIndexInfoLookup.get(col.name)!, + ); + continue; + } + // second: check if this column is a native column with a kv index + if (nativeTextIndexInfoLookup.has(col.name)) { + strategies.nativeTextIndexLookup.push( + nativeTextIndexInfoLookup.get(col.name)!, + ); + continue; + } + // third: check if there are metadataMVs that contain a SELECT to aggregate this field + if ( + metadataMVs && + metadataMVs.kvRollupTable && + (await this.doMetadataMVsAggregateColumn( + { + databaseName, + tableName: metadataMVs.kvRollupTable, + connectionId, + }, + col.name, + )) + ) { + strategies.metadataMVs.push({ + columnName: col.name, + mvName: metadataMVs.kvRollupTable, + }); + continue; + } + // fallback: normal table scan + strategies.rawTable.push(col.name); + } + + return strategies; + }, + ); + } + /** - * Fetches top values for one or more keys from the KV rollup table in a + * Fetches top values for one or more keys from the text index, metadataMV, or the raw table in a * single batched query. Falls back to getMapValues when no rollup is available. */ async getAllKeyValues({ @@ -1552,10 +2077,10 @@ export class Metadata { maxValuesPerKey?: number; connectionId: string; metadataMVs?: MetadataMaterializedViews; - dateRange?: [Date, Date]; - timestampValueExpression?: string; + dateRange: [Date, Date]; + timestampValueExpression: string; signal?: AbortSignal; - }): Promise<{ key: string; value: string[] }[]> { + }): Promise { if (keyExpressions.length === 0) return []; // Parse all keys into (rollupColumn, rollupKey) pairs @@ -1571,221 +2096,151 @@ export class Metadata { }; }); - // Try rollup table first when available - if (metadataMVs && dateRange) { - const alignedDateRange = getAlignedDateRange( - dateRange, - metadataMVs.granularity, - ); - - const startExpr = renderStartOfBucketExpr( - metadataMVs.granularity, - chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[0].getTime() }})`, - ); - const endExpr = renderStartOfBucketExpr( - metadataMVs.granularity, - chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[1].getTime() }})`, - ); - const timeFilter = chSql`AND Timestamp >= ${startExpr} AND Timestamp <= ${endExpr}`; - - const sortedKeyIds = parsed - .map(p => `${p.rollupColumn}:${p.rollupKey}`) - .sort() - .join(','); - const cacheKey = `${connectionId}.${databaseName}.${tableName}.${sortedKeyIds}.${alignedDateRange[0].getTime()}.${alignedDateRange[1].getTime()}.allKeyValues.${maxValuesPerKey}`; - - const tupleParams = concatChSql( - ',', - parsed.map( - p => - chSql`(${{ String: p.rollupColumn }}, ${{ String: p.rollupKey }})`, - ), - ); - - type BatchRow = { - ColumnIdentifier: string; - Key: string; - Value: string; - total_count: number; - }; - - let batchResults: BatchRow[] = []; - try { - batchResults = await this.cache.getOrFetch(cacheKey, async () => { - const sql = chSql` - SELECT ColumnIdentifier, Key, Value, sum(count) as total_count - FROM ${tableExpr({ database: databaseName, table: metadataMVs.kvRollupTable })} - WHERE (ColumnIdentifier, Key) IN (${tupleParams}) - AND Value != '' - ${timeFilter} - GROUP BY ColumnIdentifier, Key, Value - ORDER BY ColumnIdentifier, Key, total_count DESC - LIMIT ${{ Int32: maxValuesPerKey }} BY ColumnIdentifier, Key - `; + // Strategy: + // JSON -> disabled + // Maps -> kv text index, then try to rollup (if in the MV statement), fallback to raw table scan + // Columns -> text index, then try the rollup (if in the MV statement), fallback to raw table scan + const keyValueFetchingStrategies = + await this.determineKeyValueFetchingStrategy({ + databaseName, + tableName, + connectionId, + metadataMVs, + }); - return await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - query_params: sql.params, - connectionId, - clickhouse_settings: { - ...this.getClickHouseSettings(), - timeout_overflow_mode: 'break', - max_execution_time: 15, - max_rows_to_read: '0', - }, - abort_signal: signal, - }) - .then(res => res.json()) - .then(d => d.data); - }); - } catch (e) { - console.warn('Batched rollup query failed, falling back to per-key', e); + // build expressions for each query type + const mapTextIndexQueryOptions: TextIndexMapColumnQueryOptions = new Map(); + const nativeTextIndexQueryOptions: TextIndexColumnQueryOptions = new Map(); + const metadataMVQueryOptions: MetadataMVQueryOptions = new Map(); + const rawQueryOptions: string[] = []; + for (const key of parsed) { + // first check text index + if (key.mapKey) { + const mapTextIndex = keyValueFetchingStrategies.mapTextIndexLookup.find( + idx => idx.kv && idx.kv.mapColumn === key.column, + ); + if (mapTextIndex?.kv) { + let entry = mapTextIndexQueryOptions.get(key.column); + if (!entry) { + entry = { + indexName: mapTextIndex.kv.indexName, + limit: 20, + separator: mapTextIndex.kv.separator, + keys: [], + }; + mapTextIndexQueryOptions.set(key.column, entry); + } + entry.keys.push(key.mapKey); + continue; + } + } else { + const nativeTextIndex = + keyValueFetchingStrategies.nativeTextIndexLookup.find( + idx => idx.expression === key.column, + ); + if (nativeTextIndex) { + nativeTextIndexQueryOptions.set(key.column, { + indexName: nativeTextIndex.name, + limit: 20, + }); + continue; + } } - // Group results by (ColumnIdentifier, Key) and apply per-key limit - const resultMap = new Map(); - for (const row of batchResults) { - const mapKey = `${row.ColumnIdentifier}:${row.Key}`; - let arr = resultMap.get(mapKey); - if (!arr) { - arr = []; - resultMap.set(mapKey, arr); + // then check metadataMVs + const metadataMVEntry = keyValueFetchingStrategies.metadataMVs.find( + v => v.columnName === key.column.replaceAll('`', ''), + ); + if (metadataMVEntry) { + let tableEntry = metadataMVQueryOptions.get(metadataMVEntry.mvName); + if (!tableEntry) { + tableEntry = new Map(); + metadataMVQueryOptions.set(metadataMVEntry.mvName, tableEntry); } - if (arr.length < maxValuesPerKey) { - arr.push(row.Value); + let columnEntry = tableEntry.get(key.rollupColumn); + if (!columnEntry) { + columnEntry = []; + tableEntry.set(key.rollupColumn, columnEntry); } + columnEntry.push(key.rollupKey); + continue; } - // Build results, falling back to getMapValues for keys with no rollup data - return Promise.all( - parsed.map(async p => { - const mapKey = `${p.rollupColumn}:${p.rollupKey}`; - const values = resultMap.get(mapKey); - if (values && values.length > 0) { - return { key: p.keyExpression, value: values }; - } - const fallback = await this.getMapValues({ - databaseName, - tableName, - column: p.column, - key: p.mapKey, - maxValues: maxValuesPerKey, - connectionId, - dateRange, - timestampValueExpression, - signal, - }); - return { key: p.keyExpression, value: fallback }; + // fallback to raw table scan + if (keyValueFetchingStrategies.rawTable.includes(key.column)) { + if (key.mapKey) { + rawQueryOptions.push(`${key.column}['${key.mapKey}']`); + } else { + rawQueryOptions.push(`${key.column}`); + } + } + } + + // fire all the kv fetch queries + const promises: Array> = []; + if (mapTextIndexQueryOptions.size > 0) { + promises.push( + this.getMapTextIndexValues({ + databaseName, + tableName, + connectionId, + queryOptions: mapTextIndexQueryOptions, + dateRange, + timestampValueExpression, + signal, }), ); } - - // No rollup available — fall back to main table scan for all keys - return Promise.all( - parsed.map(async p => { - const value = await this.getMapValues({ + if (nativeTextIndexQueryOptions.size > 0) { + promises.push( + this.getTextIndexValues({ databaseName, tableName, - column: p.column, - key: p.mapKey, - maxValues: maxValuesPerKey, connectionId, + queryOptions: nativeTextIndexQueryOptions, dateRange, timestampValueExpression, signal, - }); - return { key: p.keyExpression, value }; - }), - ); - } - - /** - * Single-query discovery: returns all (ColumnIdentifier, Key) pairs from the - * KV rollup table. Falls back to column metadata + getMapValues when no rollup - * is available. - */ - async getAllFieldsAndValues({ - databaseName, - tableName, - connectionId, - metadataMVs, - dateRange, - maxValuesPerKey = 20, - maxKeys, - signal, - }: { - databaseName: string; - tableName: string; - connectionId: string; - metadataMVs?: MetadataMaterializedViews; - dateRange?: [Date, Date]; - maxValuesPerKey?: number; - maxKeys?: number; - signal?: AbortSignal; - }): Promise<{ key: string; value: string[] }[]> { - if (!metadataMVs || !dateRange) return []; - - const alignedDateRange = getAlignedDateRange( - dateRange, - metadataMVs.granularity, - ); - const startExpr = renderStartOfBucketExpr( - metadataMVs.granularity, - chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[0].getTime() }})`, - ); - const endExpr = renderStartOfBucketExpr( - metadataMVs.granularity, - chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[1].getTime() }})`, - ); - const timeFilter = chSql`Timestamp >= ${startExpr} AND Timestamp <= ${endExpr}`; - - const cacheKey = `${connectionId}.${databaseName}.${tableName}.${alignedDateRange[0].getTime()}.${alignedDateRange[1].getTime()}.fieldsAndValues.${maxValuesPerKey}.${maxKeys ?? 'all'}`; - - type RollupRow = { - ColumnIdentifier: string; - Key: string; - Values: string[]; - }; - - const rows = await this.cache.getOrFetch(cacheKey, async () => { - const limitClause = maxKeys - ? chSql`LIMIT ${{ Int32: maxKeys }}` - : chSql``; - const sql = chSql` - SELECT ColumnIdentifier, Key, groupUniqArray(${{ UNSAFE_RAW_SQL: inlineNonNegativeInt(maxValuesPerKey, 'maxValuesPerKey') }})(Value) AS Values - FROM ${tableExpr({ database: databaseName, table: metadataMVs.kvRollupTable })} - WHERE Value != '' - AND ${timeFilter} - GROUP BY ColumnIdentifier, Key - ORDER BY ColumnIdentifier = 'NativeColumn' DESC, ColumnIdentifier = 'ResourceAttributes' DESC, ColumnIdentifier, Key - ${limitClause} - `; - - return await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - query_params: sql.params, + }), + ); + } + if (metadataMVQueryOptions.size > 0) { + promises.push( + this.getMetadataMVKeyValues({ + databaseName, + tableName, connectionId, - clickhouse_settings: { - ...this.getClickHouseSettings(), - timeout_overflow_mode: 'break', - max_execution_time: 30, - max_rows_to_read: '0', + queryOptions: metadataMVQueryOptions, + maxValuesPerKey, + dateRange, + metadataMVs, + signal, + }), + ); + } + if (rawQueryOptions.length > 0) { + promises.push( + this.getKeyValues({ + chartConfig: { + from: { + databaseName, + tableName, + }, + connection: connectionId, + dateRange, + timestampValueExpression, + select: '', + where: '', }, - abort_signal: signal, - }) - .then(res => res.json()) - .then(d => d.data); - }); - - return rows.map(row => { - const keyExpr = - row.ColumnIdentifier === 'NativeColumn' - ? row.Key - : `${row.ColumnIdentifier}['${row.Key}']`; - return { key: keyExpr, value: row.Values }; - }); + keys: rawQueryOptions, + source: undefined, + signal, + }), + ); + } + return (await Promise.all(promises)) + .filter(v => v !== undefined) + .flatMap(v => v); } async getKeyValues({ @@ -1804,7 +2259,7 @@ export class Metadata { source: | Omit /* for overlap with ISource type */ | undefined; - }): Promise<{ key: string; value: string[] | number[] }[]> { + }): Promise { const cacheKeyConfig = { ...pick(chartConfig, [ 'connection', @@ -1933,7 +2388,7 @@ export class Metadata { limit?: number; disableRowLimit?: boolean; signal?: AbortSignal; - }): Promise<{ key: string; value: string[] | number[] }[]> { + }): Promise { const cacheKeyConfig = { ...pick(chartConfig, [ 'connection', diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index f8aa4c64a2..4fb0bbbe05 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -23,10 +23,10 @@ import { } from '@/guards'; import { replaceMacros } from '@/macros'; import { - buildKvItemsLookup, + buildTextIndexInfoLookup, CustomSchemaSQLSerializerV2, - KvItemsLookup, SearchQueryBuilder, + TextIndexInfoLookup, } from '@/queryParser'; import { QUERY_PARAMS_BY_DISPLAY_TYPE } from '@/rawSqlParams'; import { @@ -359,9 +359,9 @@ function generateHasSqlForKvItemsColumn( export const rewriteSqlFilterWithKvItems = ( condition: string, - kvItemsLookup: KvItemsLookup, + textIndexInfoLookup: TextIndexInfoLookup, ): string => { - if (kvItemsLookup.size === 0) return condition; + if (textIndexInfoLookup.size === 0) return condition; try { const parser = new SQLParser.Parser(); const prefix = 'SELECT 1 FROM `t` WHERE '; @@ -400,7 +400,7 @@ export const rewriteSqlFilterWithKvItems = ( return; } const mapKey: string = idxNode.value; - const info = kvItemsLookup.get(mapColumn); + const info = textIndexInfoLookup.get(mapColumn)?.kv; if (!info) return; let values: string[]; @@ -436,7 +436,7 @@ export const rewriteSqlFilterWithKvItems = ( let replacement: string; if (values.length === 1) { replacement = generateHasSqlForKvItemsColumn( - info.kvItemsColumn, + info.columnName, mapKey, info.separator, values[0], @@ -445,7 +445,7 @@ export const rewriteSqlFilterWithKvItems = ( // ClickHouse >= 26.5 supports `hasAny` over the direct_read map items // column in a single call. replacement = `hasAny(${SqlString.format('??', [ - info.kvItemsColumn, + info.columnName, ])}, array(${values .map(v => SqlString.format('concat(?, ?, ?)', [mapKey, info.separator, v]), @@ -457,7 +457,7 @@ export const rewriteSqlFilterWithKvItems = ( replacement = `(${values .map(v => generateHasSqlForKvItemsColumn( - info.kvItemsColumn, + info.columnName, mapKey, info.separator, v, @@ -1136,12 +1136,12 @@ async function renderWhere( const hasSqlFilter = chartConfig.filters?.some(f => f.type === 'sql') ?? false; - const kvItemsLookup: KvItemsLookup = + const textIndexInfoLookup: TextIndexInfoLookup = hasSqlFilter && chartConfig.from.databaseName && chartConfig.from.tableName && !hasSubqueryCte(chartConfig.with) - ? await buildKvItemsLookup({ + ? await buildTextIndexInfoLookup({ metadata, databaseName: chartConfig.from.databaseName, tableName: chartConfig.from.tableName, @@ -1160,7 +1160,7 @@ async function renderWhere( } else if (filter.type === 'lucene' || filter.type === 'sql') { const condition = filter.type === 'sql' - ? rewriteSqlFilterWithKvItems(filter.condition, kvItemsLookup) + ? rewriteSqlFilterWithKvItems(filter.condition, textIndexInfoLookup) : filter.condition; return wrapChSqlIfNotEmpty( await renderWhereExpression({ diff --git a/packages/common-utils/src/core/utils.ts b/packages/common-utils/src/core/utils.ts index 7316c5fe9a..258b335af5 100644 --- a/packages/common-utils/src/core/utils.ts +++ b/packages/common-utils/src/core/utils.ts @@ -1352,3 +1352,30 @@ export const isTimeSeriesDisplayType = ( displayType === DisplayType.Line || displayType === DisplayType.StackedBar ); }; + +// This type serves as options to fetch values from normal text indices. +// This is a record of Column Name to required query parameters. +export type TextIndexColumnQueryOptions = Map< + string, + { + indexName: string; + limit: number; + } +>; + +// This type serves as options to fetch values from map text indices. +// This is a record of Map Column Name to required query parameters. +export type TextIndexMapColumnQueryOptions = Map< + string, + { + indexName: string; + limit: number; + separator: string; + keys: string[]; + } +>; + +export type MetadataMVQueryOptions = Map< + string, + Map<'NativeColumn' | string, string[]> // map from column name to keys +>; diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index 7436a291b4..e3c711ec8c 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -10,6 +10,7 @@ import { JSDataType, } from '@/clickhouse'; import { + ClickHouseVersion, isClickHouseVersionAtLeast, supportsDirectReadMap, } from '@/core/clickhouseVersion'; @@ -411,7 +412,7 @@ export abstract class SQLSerializer implements Serializer { found: boolean; mapKeyIndexExpression?: string; arrayMapKeyExpression?: string; - kvItemsExpression?: KvItemsInfo & { mapColumn: string; mapKey: string }; + kvItemsExpression?: KvIndexInfo & { mapKey: string }; }>; operator(op: lucene.Operator) { @@ -473,7 +474,7 @@ export abstract class SQLSerializer implements Serializer { // so we emit: has(arr, 'key') OR NOT mapContains(Map, 'key') if (kvItemsExpression && propertyType === JSDataType.String) { const hasExpr = SqlString.format(`has(??, concat(?, ?, ?))`, [ - kvItemsExpression.kvItemsColumn, + kvItemsExpression.columnName, kvItemsExpression.mapKey, kvItemsExpression.separator, term, @@ -743,8 +744,9 @@ type CustomSchemaSQLColumnExpression = { }; mapKeyIndexExpression?: string; arrayMapKeyExpression?: string; + mapKey?: string; /** When a KV items index exists for a Map column, carries the info needed for the has() optimization */ - kvItemsExpression?: KvItemsInfo & { mapColumn: string; mapKey: string }; + textIndexInfo?: TextIndexInfo; }; export type CustomSchemaConfig = { @@ -857,8 +859,9 @@ function renderArrayFieldExpression({ } /** Describes a KV items column and its concat separator */ -export type KvItemsInfo = { - kvItemsColumn: string; +export type KvIndexInfo = { + columnName: string; + indexName: string; separator: string; /** * Whether the connected ClickHouse server supports `hasAny(items, array(...))` @@ -868,10 +871,16 @@ export type KvItemsInfo = { * calls combined with `OR`. */ useHasAny: boolean; + mapColumn: string; +}; +export type KeyIndexInfo = { + indexName: string; + mapColumn: string; }; +export type TextIndexInfo = { kv?: KvIndexInfo; key?: KeyIndexInfo }; -/** Map from map column name to its KV items info */ -export type KvItemsLookup = Map; +/** Map from map column name to its text index info */ +export type TextIndexInfoLookup = Map; /** * Tokenizes a ClickHouse expression into meaningful tokens (identifiers, parens, @@ -1086,6 +1095,120 @@ const KV_ITEMS_STRATEGIES = [ parseKvItemsCastExpression, ] as const; +export function skipIndexMatches( + idx: SkipIndexMetadata, + expectedType: + | 'text' + | 'bloom_filter' + | 'minmax' + | 'tokenbf_v1' + | 'set' + | 'ngrambf_v1', + options?: { + tokenizer?: string; + }, +): boolean { + if (idx.type !== expectedType) return false; + if ( + expectedType === 'text' && + options?.tokenizer && + idx.typeFull.replaceAll(' ', '') !== + `text(tokenizer='${options.tokenizer}')` + ) + return false; + return true; +} + +function populateValidKvTextIndices( + serverVersion: ClickHouseVersion | undefined, + columns: ColumnMeta[], + skipIndices: SkipIndexMetadata[], + isCloud: boolean, + lookup: TextIndexInfoLookup, +) { + const isDirectReadSupported = supportsDirectReadMap(serverVersion, isCloud); + const useHasAny = isClickHouseVersionAtLeast(serverVersion, [26, 5, 0, 0]); + const candidates = columns.filter( + c => + ((isDirectReadSupported && c.default_type === 'ALIAS') || + c.default_type === 'MATERIALIZED') && + c.default_expression, + ); + for (const candidate of candidates) { + let parsed: { mapColumn: string; separator: string } | undefined; + for (const strategy of KV_ITEMS_STRATEGIES) { + parsed = strategy(candidate.default_expression); + if (parsed) break; + } + if (!parsed) continue; + + const candidateName = normalizeChExpression(candidate.name); + const candidateExpr = normalizeChExpression(candidate.default_expression); + + const validIndex = skipIndices.find(idx => { + if (!skipIndexMatches(idx, 'text', { tokenizer: 'array' })) return false; + const tokenizer = parseTokenizerFromTextIndex(idx); + if (tokenizer?.type !== 'array') return false; + const idxExpr = normalizeChExpression(idx.expression); + return idxExpr === candidateName || idxExpr === candidateExpr; + }); + + if (validIndex) { + let entry = lookup.get(parsed.mapColumn); + if (!entry) { + entry = {}; + lookup.set(parsed.mapColumn, entry); + } + entry.kv = { + columnName: candidate.name, + indexName: validIndex.name, + separator: parsed.separator, + useHasAny, + mapColumn: parsed.mapColumn, + }; + } + } +} + +function populateValidKeyTextIndices( + _serverVersion: ClickHouseVersion | undefined, + columns: ColumnMeta[], + skipIndices: SkipIndexMetadata[], + _isCloud: boolean, + lookup: TextIndexInfoLookup, +) { + const mapKeyIndexStartString = 'mapKeys('; + const candidates = skipIndices.filter( + idx => + idx.expression.startsWith(mapKeyIndexStartString) && + skipIndexMatches(idx, 'text', { tokenizer: 'array' }), + ); + for (const candidate of candidates) { + const parsedMapColumn = candidate.expression.slice( + mapKeyIndexStartString.length, + -1, + ); + + const validColumn = columns.find( + col => + col.name === parsedMapColumn && + col.type === 'Map(LowCardinality(String), String)', + ); + + if (validColumn) { + let entry = lookup.get(parsedMapColumn); + if (!entry) { + entry = {}; + lookup.set(parsedMapColumn, entry); + } + entry.key = { + indexName: candidate.name, + mapColumn: validColumn.name, + }; + } + } +} + /** * Builds a lookup from map column name to KV items column name. * A KV items column is an ALIAS/MATERIALIZED column whose expression is @@ -1101,7 +1224,7 @@ const KV_ITEMS_STRATEGIES = [ * * Returns an empty Map on any failure; never throws. */ -export async function buildKvItemsLookup({ +export async function buildTextIndexInfoLookup({ metadata, databaseName, tableName, @@ -1111,8 +1234,8 @@ export async function buildKvItemsLookup({ databaseName: string; tableName: string; connectionId: string; -}): Promise { - const lookup: KvItemsLookup = new Map(); +}): Promise { + const lookup: TextIndexInfoLookup = new Map(); try { const [serverVersion, columns, skipIndices, isCloud] = await Promise.all([ metadata.getServerVersion({ connectionId }), @@ -1123,45 +1246,21 @@ export async function buildKvItemsLookup({ metadata.isClickHouseCloud({ connectionId }).catch(() => false), ]); - const directReadSupported = supportsDirectReadMap(serverVersion, isCloud); - const useHasAny = isClickHouseVersionAtLeast(serverVersion, [26, 5, 0, 0]); - - const kvItemsCandidates = columns.filter( - c => - (c.default_type === 'ALIAS' || c.default_type === 'MATERIALIZED') && - c.default_expression, + populateValidKvTextIndices( + serverVersion, + columns, + skipIndices, + isCloud, + lookup, ); - - for (const candidate of kvItemsCandidates) { - if (candidate.default_type === 'ALIAS' && !directReadSupported) { - continue; - } - - let parsed: { mapColumn: string; separator: string } | undefined; - for (const strategy of KV_ITEMS_STRATEGIES) { - parsed = strategy(candidate.default_expression); - if (parsed) break; - } - if (!parsed) continue; - - const candidateName = normalizeChExpression(candidate.name); - const candidateExpr = normalizeChExpression(candidate.default_expression); - const hasArrayTextIndex = skipIndices.some(idx => { - if (idx.type !== 'text') return false; - const tokenizer = parseTokenizerFromTextIndex(idx); - if (tokenizer?.type !== 'array') return false; - const idxExpr = normalizeChExpression(idx.expression); - return idxExpr === candidateName || idxExpr === candidateExpr; - }); - - if (hasArrayTextIndex) { - lookup.set(parsed.mapColumn, { - kvItemsColumn: candidate.name, - separator: parsed.separator, - useHasAny, - }); - } - } + populateValidKeyTextIndices( + serverVersion, + columns, + skipIndices, + isCloud, + lookup, + ); + return lookup; } catch (error) { console.warn('Error building KV items lookup:', error); } @@ -1178,7 +1277,7 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { private useTextIndexForImplicitColumn: UseTextIndex; private skipIndicesPromise?: Promise; private enableTextIndexPromise?: Promise; - private kvItemsLookupPromise?: Promise; + private textIndexInfoLookupPromise?: Promise; constructor({ metadata, @@ -1223,11 +1322,11 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { return false; }); - this.kvItemsLookupPromise = this.buildKvItemsLookup(); + this.textIndexInfoLookupPromise = this.buildTextIndexInfoLookup(); } - private buildKvItemsLookup(): Promise { - return buildKvItemsLookup({ + private buildTextIndexInfoLookup(): Promise { + return buildTextIndexInfoLookup({ metadata: this.metadata, databaseName: this.databaseName, tableName: this.tableName, @@ -1535,8 +1634,8 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { const valueType = prefixMatch.type.match(/,\s+(\w+)\)$/)?.[1]; // Check if a KV items index exists for this map column - const kvItemsLookup = await this.kvItemsLookupPromise; - const kvItemsInfo = kvItemsLookup?.get(prefixMatch.name); + const textIndexInfoLookup = await this.textIndexInfoLookupPromise; + const textIndexInfo = textIndexInfoLookup?.get(prefixMatch.name); return { found: true, @@ -1546,12 +1645,12 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { ]), mapKeyIndexExpression: `indexHint(${buildMapContains(`${prefixMatch.name}['${fieldPostfix}']`)})`, columnType: valueType ?? 'Unknown', - ...(kvItemsInfo + mapKey: fieldPostfix, + ...(textIndexInfo ? { - kvItemsExpression: { - ...kvItemsInfo, - mapColumn: prefixMatch.name, - mapKey: fieldPostfix, + textIndexInfo: { + kv: textIndexInfo.kv, + key: textIndexInfo.key, }, } : {}), @@ -1775,7 +1874,10 @@ export class CustomSchemaSQLSerializerV2 extends SQLSerializer { arrayMapKeyExpression: isArray ? expression.arrayMapKeyExpression : undefined, - kvItemsExpression: expression.kvItemsExpression, + kvItemsExpression: + expression.textIndexInfo?.kv && expression.mapKey + ? { ...expression.textIndexInfo.kv, mapKey: expression.mapKey } + : undefined, }; } } From 612bb2f9a9c8124c6781f36cf834b62605d9331c Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:39:32 -0400 Subject: [PATCH 02/24] fix: remove key mv and map branches from recommended log and trace rollup schemas --- .../schema/seed/00006_otel_logs_rollups.sql | 53 +------------------ .../schema/seed/00007_otel_traces_rollups.sql | 44 +-------------- 2 files changed, 3 insertions(+), 94 deletions(-) diff --git a/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql b/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql index dfb448ce22..d5fc66cbc7 100644 --- a/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql +++ b/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql @@ -1,6 +1,6 @@ -- +goose Up --- Map Attributes Key Value Rollup (all map columns in one table) +-- Key Value Rollup (many columns in one table) CREATE TABLE IF NOT EXISTS ${DATABASE}.otel_logs_kv_rollup_15m ( `Timestamp` DateTime, @@ -17,58 +17,9 @@ ORDER BY (ColumnIdentifier, Key, Timestamp, Value) TTL Timestamp + ${TABLES_TTL} SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; --- Key Only Rollup (derived from KV rollup) -CREATE TABLE IF NOT EXISTS ${DATABASE}.otel_logs_key_rollup_15m -( - `Timestamp` DateTime, - `ColumnIdentifier` LowCardinality(String), - `Key` LowCardinality(String), - `count` UInt64, - INDEX idx_count_minmax count TYPE minmax GRANULARITY 1, - INDEX idx_timestamp_minmax Timestamp TYPE minmax GRANULARITY 1 -) -ENGINE = SummingMergeTree -PARTITION BY toDate(Timestamp) -ORDER BY (ColumnIdentifier, Key, Timestamp) -TTL Timestamp + ${TABLES_TTL} -SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; - -CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_logs_key_rollup_15m_mv TO ${DATABASE}.otel_logs_key_rollup_15m -AS SELECT - Timestamp, - ColumnIdentifier, - Key, - sum(count) as count -FROM ${DATABASE}.otel_logs_kv_rollup_15m -GROUP BY ColumnIdentifier, Key, Timestamp; - -- Single MV: CTE with UNION ALL across all columns, then aggregate -CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_logs_attr_kv_rollup_15m_mv TO ${DATABASE}.otel_logs_kv_rollup_15m +CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_logs_kv_rollup_15m_mv TO ${DATABASE}.otel_logs_kv_rollup_15m AS WITH elements AS ( - SELECT - 'ResourceAttributes' AS ColumnIdentifier, - toStartOfFifteenMinutes(Timestamp) AS Timestamp, - replaceRegexpAll(entry.1, '\\[\\d+\\]', '[*]') AS Key, - CAST(entry.2 AS String) AS Value - FROM ${DATABASE}.otel_logs - ARRAY JOIN ResourceAttributes AS entry - UNION ALL - SELECT - 'LogAttributes' AS ColumnIdentifier, - toStartOfFifteenMinutes(Timestamp) AS Timestamp, - replaceRegexpAll(entry.1, '\\[\\d+\\]', '[*]') AS Key, - CAST(entry.2 AS String) AS Value - FROM ${DATABASE}.otel_logs - ARRAY JOIN LogAttributes AS entry - UNION ALL - SELECT - 'ScopeAttributes' AS ColumnIdentifier, - toStartOfFifteenMinutes(Timestamp) AS Timestamp, - replaceRegexpAll(entry.1, '\\[\\d+\\]', '[*]') AS Key, - CAST(entry.2 AS String) AS Value - FROM ${DATABASE}.otel_logs - ARRAY JOIN ScopeAttributes AS entry - UNION ALL SELECT 'NativeColumn' AS ColumnIdentifier, toStartOfFifteenMinutes(Timestamp) AS Timestamp, diff --git a/docker/otel-collector/schema/seed/00007_otel_traces_rollups.sql b/docker/otel-collector/schema/seed/00007_otel_traces_rollups.sql index 2ca574dbb2..107af9956f 100644 --- a/docker/otel-collector/schema/seed/00007_otel_traces_rollups.sql +++ b/docker/otel-collector/schema/seed/00007_otel_traces_rollups.sql @@ -1,6 +1,6 @@ -- +goose Up --- Key Value Rollup (all columns in one table) +-- Key Value Rollup (many columns in one table) CREATE TABLE IF NOT EXISTS ${DATABASE}.otel_traces_kv_rollup_15m ( `Timestamp` DateTime, @@ -17,51 +17,9 @@ ORDER BY (ColumnIdentifier, Key, Timestamp, Value) TTL Timestamp + ${TABLES_TTL} SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; --- Key Only Rollup (derived from KV rollup) -CREATE TABLE IF NOT EXISTS ${DATABASE}.otel_traces_key_rollup_15m -( - `Timestamp` DateTime, - `ColumnIdentifier` LowCardinality(String), - `Key` LowCardinality(String), - `count` UInt64, - INDEX idx_count_minmax count TYPE minmax GRANULARITY 1, - INDEX idx_timestamp_minmax Timestamp TYPE minmax GRANULARITY 1 -) -ENGINE = SummingMergeTree -PARTITION BY toDate(Timestamp) -ORDER BY (ColumnIdentifier, Key, Timestamp) -TTL Timestamp + ${TABLES_TTL} -SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; - --- Key only rollup MV -CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_traces_key_rollup_15m_mv TO ${DATABASE}.otel_traces_key_rollup_15m -AS SELECT - Timestamp, - ColumnIdentifier, - Key, - sum(count) AS count -FROM ${DATABASE}.otel_traces_kv_rollup_15m -GROUP BY ColumnIdentifier, Key, Timestamp; - -- Single MV: CTE with UNION ALL across all columns, then aggregate CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_traces_kv_rollup_15m_mv TO ${DATABASE}.otel_traces_kv_rollup_15m AS WITH elements AS ( - SELECT - 'ResourceAttributes' AS ColumnIdentifier, - toStartOfFifteenMinutes(Timestamp) AS Timestamp, - replaceRegexpAll(entry.1, '\\[\\d+\\]', '[*]') AS Key, - CAST(entry.2 AS String) AS Value - FROM ${DATABASE}.otel_traces - ARRAY JOIN ResourceAttributes AS entry - UNION ALL - SELECT - 'SpanAttributes' AS ColumnIdentifier, - toStartOfFifteenMinutes(Timestamp) AS Timestamp, - replaceRegexpAll(entry.1, '\\[\\d+\\]', '[*]') AS Key, - CAST(entry.2 AS String) AS Value - FROM ${DATABASE}.otel_traces - ARRAY JOIN SpanAttributes AS entry - UNION ALL SELECT 'NativeColumn' AS ColumnIdentifier, toStartOfFifteenMinutes(Timestamp) AS Timestamp, From cfeda952a4db3ad9f7c93ba9ce2533e24971ee5b Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:50:55 -0400 Subject: [PATCH 03/24] fix: rename functions for consistency --- packages/common-utils/src/core/metadata.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 13cd65d93a..db30fc5ae9 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1056,7 +1056,7 @@ export class Metadata { }); } - private async getMapTextIndexValues({ + private async getMapTextIndexKeyValues({ databaseName, tableName, connectionId, @@ -1070,7 +1070,7 @@ export class Metadata { timestampValueExpression: string; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getMapTextIndexValues`; + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getMapTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { const sqlBranches: Array = []; for (const [columnName, info] of queryOptions.entries()) { @@ -1130,7 +1130,7 @@ export class Metadata { }); } - private async getTextIndexValues({ + private async getTextIndexKeyValues({ databaseName, tableName, connectionId, @@ -1144,7 +1144,7 @@ export class Metadata { timestampValueExpression: string; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getTextIndexValues`; + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { const sqlBranches: Array = []; for (const [columnName, info] of queryOptions.entries()) { @@ -2180,7 +2180,7 @@ export class Metadata { const promises: Array> = []; if (mapTextIndexQueryOptions.size > 0) { promises.push( - this.getMapTextIndexValues({ + this.getMapTextIndexKeyValues({ databaseName, tableName, connectionId, @@ -2193,7 +2193,7 @@ export class Metadata { } if (nativeTextIndexQueryOptions.size > 0) { promises.push( - this.getTextIndexValues({ + this.getTextIndexKeyValues({ databaseName, tableName, connectionId, From 1c7dbf86d8f34702f2dba3dc32f49c068a3b5d9b Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:21:37 -0400 Subject: [PATCH 04/24] add tests --- .../filter-modes-and-more-filters.spec.ts | 102 ++++ .../src/__tests__/metadata.test.ts | 449 ++++++++++++++++++ 2 files changed, 551 insertions(+) create mode 100644 packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts diff --git a/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts b/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts new file mode 100644 index 0000000000..5cc016b852 --- /dev/null +++ b/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts @@ -0,0 +1,102 @@ +import { SearchPage } from '../../page-objects/SearchPage'; +import { expect, test } from '../../utils/base-test'; +import { DEFAULT_LOGS_SOURCE_NAME } from '../../utils/constants'; + +const FILTER_QUERY_TIMEOUT = 20_000; + +const LOW_CARDINALITY_FILTER = 'SeverityText'; +const NON_LOW_CARDINALITY_FILTER = 'TraceId'; + +async function openFilterSettings(searchPage: SearchPage) { + await searchPage.page + .getByRole('button', { name: 'Filter settings' }) + .click(); +} + +async function setShowAllValues(searchPage: SearchPage, checked: boolean) { + await openFilterSettings(searchPage); + const showAllCheckbox = searchPage.page.getByRole('checkbox', { + name: 'Show All Values', + }); + const currentlyChecked = await showAllCheckbox.isChecked(); + if (currentlyChecked !== checked) { + await showAllCheckbox.click(); + } + await searchPage.page.keyboard.press('Escape'); +} + +test.describe( + 'Filter modes (all vs exact) and More filters', + { tag: ['@search'] }, + () => { + let searchPage: SearchPage; + + test.beforeEach(async ({ page }) => { + searchPage = new SearchPage(page); + await searchPage.goto(); + await searchPage.selectSource(DEFAULT_LOGS_SOURCE_NAME); + await searchPage.timePicker.selectRelativeTime('Last 1 hour'); + await searchPage.table.waitForRowsToPopulate(); + }); + + test('all mode loads low-cardinality filters by default', async () => { + await setShowAllValues(searchPage, true); + + await expect( + searchPage.filters.getFilterGroup(LOW_CARDINALITY_FILTER), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + + await searchPage.filters.openFilterGroup(LOW_CARDINALITY_FILTER); + await expect( + searchPage.filters.getFilterCheckboxInput( + LOW_CARDINALITY_FILTER, + 'info', + ), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + }); + + test('exact mode loads low-cardinality filters when Show All Values is off', async () => { + await setShowAllValues(searchPage, false); + + await expect( + searchPage.filters.getFilterGroup(LOW_CARDINALITY_FILTER), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + + await searchPage.filters.openFilterGroup(LOW_CARDINALITY_FILTER); + await expect( + searchPage.filters.getFilterCheckboxInput( + LOW_CARDINALITY_FILTER, + 'info', + ), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + }); + + test('More filters reveals non-low-cardinality columns in all mode', async () => { + await setShowAllValues(searchPage, true); + + await expect( + searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), + ).toHaveCount(0); + + await searchPage.filters.showMoreFilters(); + + await expect( + searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + }); + + test('More filters reveals non-low-cardinality columns in exact mode', async () => { + await setShowAllValues(searchPage, false); + + await expect( + searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), + ).toHaveCount(0); + + await searchPage.filters.showMoreFilters(); + + await expect( + searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), + ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); + }); + }, +); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 93db570a75..23b02ef6ae 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -790,6 +790,290 @@ describe('Metadata', () => { }); }); + // Each of the four fetch strategies emits a distinct SQL shape (map-text- + // index, native-text-index, metadata-MV, raw-table). We assert against + // those shapes rather than exposing the private methods. + describe('getAllKeyValues (router)', () => { + const dateRange: [Date, Date] = [ + new Date('2024-01-01'), + new Date('2024-01-02'), + ]; + const baseArgs = { + databaseName: 'default', + tableName: 'otel_logs', + connectionId: 'test_connection', + dateRange, + timestampValueExpression: 'Timestamp', + metadataMVs: { + keyRollupTable: 'otel_logs_key_rollup_15m', + kvRollupTable: 'otel_logs_kv_rollup_15m', + granularity: '15 minute' as const, + }, + }; + + const setupDefaultLogsSchema = () => { + mockCache.getOrFetch.mockImplementation((_key: string, fn: () => any) => + fn(), + ); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => Promise.resolve({ data: [{}] }), + }); + + jest.spyOn(metadata, 'getColumns').mockResolvedValue([ + { name: 'Timestamp', type: 'DateTime64(9)' }, + { name: 'TraceId', type: 'String' }, + { name: 'SpanId', type: 'String' }, + { name: 'ServiceName', type: 'LowCardinality(String)' }, + { name: 'SeverityText', type: 'LowCardinality(String)' }, + { name: 'Body', type: 'String' }, + { name: 'LogAttributes', type: 'Map(LowCardinality(String), String)' }, + { + name: 'ResourceAttributes', + type: 'Map(LowCardinality(String), String)', + }, + ] as any); + + jest.spyOn(metadata, 'getMapColumnTextIndexes').mockResolvedValue( + new Map([ + [ + 'LogAttributes', + { + kv: { + columnName: 'LogAttributes', + mapColumn: 'LogAttributes', + indexName: 'idx_log_attr_items', + separator: '=', + useHasAny: false, + }, + }, + ], + ]) as any, + ); + + jest.spyOn(metadata, 'getNativeArrayColumnTextIndexes').mockResolvedValue( + new Map([ + [ + 'TraceId', + { + name: 'idx_trace_id', + type: 'text', + typeFull: "text(tokenizer = 'array')", + expression: 'TraceId', + granularity: 1, + }, + ], + ]), + ); + + jest + .spyOn(metadata as any, 'doMetadataMVsAggregateColumn') + .mockImplementation((...args: any[]) => { + const columnName = args[1] as string; + return Promise.resolve( + columnName === 'ServiceName' || columnName === 'SeverityText', + ); + }); + }; + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('routes TraceId through getTextIndexKeyValues (native text index path)', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['TraceId'], + }); + + const calls = (mockClickhouseClient.query as jest.Mock).mock.calls; + expect(calls.length).toBeGreaterThan(0); + const sql = calls[calls.length - 1][0].query as string; + + expect(sql).toContain('mergeTreeTextIndex('); + expect(sql).toContain('groupUniqArray('); + expect(sql).toContain('GROUP BY key'); + expect(sql).not.toContain('startsWith(token,'); + }); + + it("routes LogAttributes['requestId'] through getMapTextIndexKeyValues (map KV text index path)", async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["LogAttributes['requestId']"], + }); + + const calls = (mockClickhouseClient.query as jest.Mock).mock.calls; + expect(calls.length).toBeGreaterThan(0); + const sql = calls[calls.length - 1][0].query as string; + + expect(sql).toContain('mergeTreeTextIndex('); + expect(sql).toContain('startsWith(token,'); + expect(sql).toContain('substring(token, position(token,'); + expect(sql).toContain('GROUP BY column, key'); + }); + + it('routes ServiceName and SeverityText through getMetadataMVKeyValues (KV rollup MV path)', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['ServiceName', 'SeverityText'], + }); + + const calls = (mockClickhouseClient.query as jest.Mock).mock.calls; + expect(calls.length).toBeGreaterThan(0); + const lastCall = calls[calls.length - 1][0]; + const sql = lastCall.query as string; + const params = lastCall.query_params as Record; + + expect(sql).toContain('ColumnIdentifier = '); + expect(sql).toContain('Key IN ('); + expect(sql).toContain('BY ColumnIdentifier, Key'); + expect(sql).not.toContain('mergeTreeTextIndex('); + expect(Object.values(params)).toContain('otel_logs_kv_rollup_15m'); + expect(Object.values(params)).toContain('ServiceName'); + expect(Object.values(params)).toContain('SeverityText'); + }); + + it('routes columns without any index or MV entry through the getKeyValues fallback (raw table scan)', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['Body'], + }); + + expect(renderChartConfigSpy).toHaveBeenCalled(); + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + expect(configArg.select).toContain('groupUniqArray('); + expect(configArg.select).toContain('param0'); + }); + + it('fans out across all four fetch paths in a single mixed call', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: [ + 'TraceId', + 'ServiceName', + "LogAttributes['requestId']", + 'Body', + ], + }); + + const queries = (mockClickhouseClient.query as jest.Mock).mock.calls.map( + (c: any[]) => c[0].query as string, + ); + + expect( + queries.some( + (s: string) => + s.includes('mergeTreeTextIndex(') && + s.includes('startsWith(token,'), + ), + ).toBe(true); + expect( + queries.some( + (s: string) => + s.includes('mergeTreeTextIndex(') && + !s.includes('startsWith(token,'), + ), + ).toBe(true); + const paramValues = ( + mockClickhouseClient.query as jest.Mock + ).mock.calls.flatMap((c: any[]) => + Object.values(c[0].query_params ?? {}), + ); + expect(paramValues).toContain('otel_logs_kv_rollup_15m'); + expect(renderChartConfigSpy).toHaveBeenCalled(); + }); + + it('returns [] immediately when keyExpressions is empty', async () => { + setupDefaultLogsSchema(); + + const result = await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: [], + }); + + expect(result).toEqual([]); + expect(mockClickhouseClient.query).not.toHaveBeenCalled(); + }); + + it('skips the Timestamp column when discovering strategies', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['Timestamp'], + }); + + expect(mockClickhouseClient.query).not.toHaveBeenCalled(); + expect(renderChartConfigSpy).not.toHaveBeenCalled(); + }); + + // Without metadataMVs, ServiceName must remain servable via the raw + // table scan. Regression guard for commit 612bb2f9a which removed the + // recommended key/map MV branches from default log/trace rollup schemas. + it('falls back to raw table scan when metadataMVs is undefined', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + metadataMVs: undefined, + keyExpressions: ['ServiceName'], + }); + + expect(renderChartConfigSpy).toHaveBeenCalled(); + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + expect(configArg.select).toContain('groupUniqArray('); + }); + + it('routes ResourceAttributes["k8s.pod.name"] to the raw table when its map has no KV text index', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["ResourceAttributes['k8s.pod.name']"], + }); + + expect(renderChartConfigSpy).toHaveBeenCalled(); + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + expect(configArg.select).toContain('groupUniqArray('); + expect(configArg.select).toContain('param0'); + }); + }); + describe('getValuesDistribution', () => { const mockChartConfig: BuilderChartConfigWithDateRange = { from: { @@ -1138,6 +1422,171 @@ describe('Metadata', () => { }); }); + // Regression guard for commit 612bb2f9a: the `keyRollupTable` MV is no + // longer in the recommended log/trace schemas, but users who still have + // the MV configured must be able to query it. + describe('getMapKeys (key rollup table path)', () => { + const buildMetadata = () => { + const realCache = new ( + jest.requireActual('../core/metadata') as any + ).MetadataCache(); + return new Metadata(mockClickhouseClient, realCache); + }; + + const dateRange: [Date, Date] = [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-01T01:00:00Z'), + ]; + const baseArgs = { + databaseName: 'default', + tableName: 'otel_logs', + column: 'LogAttributes', + connectionId: 'test_connection', + dateRange, + timestampValueExpression: 'Timestamp', + metadataMVs: { + keyRollupTable: 'otel_logs_key_rollup_15m', + kvRollupTable: 'otel_logs_kv_rollup_15m', + granularity: '15 minute' as const, + }, + }; + + afterEach(() => { + jest.restoreAllMocks(); + (mockClickhouseClient.query as jest.Mock).mockReset(); + }); + + it('queries the key rollup table when metadataMVs is configured and no text index exists', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => + Promise.resolve({ + data: [{ Key: 'user.id' }, { Key: 'request.path' }], + }), + }); + + const keys = await md.getMapKeys({ ...baseArgs }); + + expect(keys).toEqual(['user.id', 'request.path']); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1); + const call = (mockClickhouseClient.query as jest.Mock).mock.calls[0][0]; + expect(call.query).toContain('ColumnIdentifier = '); + expect(call.query).toContain('GROUP BY Key'); + expect(call.query).toContain('ORDER BY sum(count) DESC'); + expect(Object.values(call.query_params)).toContain( + 'otel_logs_key_rollup_15m', + ); + expect(Object.values(call.query_params)).toContain('LogAttributes'); + }); + + it('buckets the time filter to the configured MV granularity', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => Promise.resolve({ data: [{ Key: 'user.id' }] }), + }); + + await md.getMapKeys({ ...baseArgs }); + + const call = (mockClickhouseClient.query as jest.Mock).mock.calls[0][0]; + expect(call.query).toContain('toStartOfFifteenMinutes('); + expect(call.query).toContain('Timestamp >='); + expect(call.query).toContain('Timestamp <='); + }); + + it('filters empty keys from the rollup response', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => + Promise.resolve({ + data: [{ Key: 'user.id' }, { Key: '' }, { Key: 'request.path' }], + }), + }); + + const keys = await md.getMapKeys({ ...baseArgs }); + + expect(keys).toEqual(['user.id', 'request.path']); + }); + + it('respects the maxKeys limit as a query LIMIT parameter', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => Promise.resolve({ data: [{ Key: 'k' }] }), + }); + + await md.getMapKeys({ ...baseArgs, maxKeys: 42 }); + + const call = (mockClickhouseClient.query as jest.Mock).mock.calls[0][0]; + expect(Object.values(call.query_params)).toContain(42); + }); + + it('skips the rollup query entirely when metadataMVs is not provided', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + jest.spyOn(md, 'getColumn').mockResolvedValue({ + name: 'LogAttributes', + type: 'Map(LowCardinality(String), String)', + } as any); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => Promise.resolve({ data: [{ keysArr: ['k'] }] }), + }); + + await md.getMapKeys({ + ...baseArgs, + metadataMVs: undefined, + }); + + const queries = (mockClickhouseClient.query as jest.Mock).mock.calls.map( + (c: any[]) => c[0].query as string, + ); + expect( + queries.some((s: string) => s.includes('otel_logs_key_rollup_15m')), + ).toBe(false); + }); + + it('caches rollup keys by aligned date range', async () => { + const md = buildMetadata(); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + (mockClickhouseClient.query as jest.Mock) + .mockResolvedValueOnce({ + json: () => Promise.resolve({ data: [{ Key: 'first' }] }), + }) + .mockResolvedValueOnce({ + json: () => Promise.resolve({ data: [{ Key: 'second' }] }), + }); + + const keysA = await md.getMapKeys({ ...baseArgs }); + const keysARepeat = await md.getMapKeys({ ...baseArgs }); + const keysB = await md.getMapKeys({ + ...baseArgs, + dateRange: [ + new Date('2024-01-02T00:00:00Z'), + new Date('2024-01-02T01:00:00Z'), + ], + }); + + expect(keysA).toEqual(['first']); + expect(keysARepeat).toEqual(['first']); + expect(keysB).toEqual(['second']); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(2); + }); + }); + describe('getMapValues', () => { const buildMetadata = () => { const realCache = new ( From d91e060a92558703d41a052bfa1699e78219f90b Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:22:05 -0400 Subject: [PATCH 05/24] feat: use useFetchFacets in autocomplete --- .../components/DBSearchPageFilters/hooks.ts | 67 ++++++--- .../app/src/hooks/useAutoCompleteOptions.tsx | 128 +++++++++--------- 2 files changed, 107 insertions(+), 88 deletions(-) diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index c756edf0fd..d34f209630 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -36,6 +36,7 @@ function useFacets({ filterState, showMoreFields, enabled, + deferLoadingKeyValues, }: { chartConfig: BuilderChartConfigWithDateRange; sourceId: string | null; @@ -44,11 +45,12 @@ function useFacets({ filterState?: FilterState; showMoreFields?: boolean; enabled?: boolean; + deferLoadingKeyValues?: boolean; }) { const { data: source } = useSource({ id: sourceId, }); - const tableConnection = tcFromSource(source); + const tableConnection = useMemo(() => tcFromSource(source), [source]); const { data: columns, isLoading: isColumnsLoading } = useColumns(tableConnection); const dateTimeColumns = useDateTimeColumns(columns); @@ -152,7 +154,7 @@ function useFacets({ keys: escapedKeysToFetch, mode, }, - { enabled }, + { enabled: enabled && !deferLoadingKeyValues }, ); // Map the (escaped) result keys back to the original UI keys. @@ -173,30 +175,51 @@ function useFacets({ const strippedFilterState: FilterState = { ...filterState }; delete strippedFilterState[key]; if (sqlKey !== key) delete strippedFilterState[sqlKey]; - const newKeyVals = await metadata.getKeyValuesWithMVs({ - chartConfig: { - ...chartConfig, + if (mode === 'exact') { + const newKeyVals = await metadata.getKeyValuesWithMVs({ + chartConfig: { + ...chartConfig, + dateRange, + filters: filtersToQuery( + escapeFilterStateKeys(strippedFilterState, knownColumns), + { dateTimeColumns }, + ), + }, + keys: [sqlKey], + limit: LOAD_MORE_LOAD_LIMIT, + disableRowLimit: true, + source, + }); + return { + key, + value: newKeyVals[0].value?.map(val => val.toString()) ?? [], + }; + } else { + if (!source) + throw new Error('loadMoreFacetsForKey: source must be defined'); + const newKeyVals = await metadata.getAllKeyValues({ + databaseName: source.from.databaseName, + tableName: source.from.tableName, + connectionId: source.connection, + metadataMVs: tableConnection.metadataMVs, + keyExpressions: [sqlKey], + maxValuesPerKey: LOAD_MORE_LOAD_LIMIT, dateRange, - filters: filtersToQuery( - escapeFilterStateKeys(strippedFilterState, knownColumns), - { dateTimeColumns }, - ), - }, - keys: [sqlKey], - limit: LOAD_MORE_LOAD_LIMIT, - disableRowLimit: true, - source, - }); - return { - key, - value: newKeyVals[0].value?.map(val => val.toString()) ?? [], - }; + timestampValueExpression: source?.timestampValueExpression, + }); + return { + key, + value: newKeyVals[0].value?.map(val => val.toString()) ?? [], + }; + } } catch (error) { console.error('failed to fetch more keys', error); } return undefined; }, [ + tableConnection, + mode, metadata, chartConfig, dateRange, @@ -223,6 +246,7 @@ export function useFetchFacets({ mode, filterState, showMoreFields, + deferLoadingKeyValues, }: { chartConfig: BuilderChartConfigWithDateRange; sourceId: string | null; @@ -230,6 +254,7 @@ export function useFetchFacets({ mode: 'all' | 'exact'; filterState?: FilterState; showMoreFields?: boolean; + deferLoadingKeyValues?: boolean; }) { // Exact pipeline: fetch values for discovered keys const facetsQuery = useFacets({ @@ -240,12 +265,12 @@ export function useFetchFacets({ filterState, showMoreFields, enabled: true, + deferLoadingKeyValues, }); const [extraFacets, setExtraFacets] = useState(null); const facets = useMemo(() => { - const facets = facetsQuery.data.keyValues; - if (!facets) return undefined; + const facets = facetsQuery.data.keyValues ?? []; if (!extraFacets || extraFacets.length === 0) return facets; const seenFacets = new Set(); const output: Facet[] = []; diff --git a/packages/app/src/hooks/useAutoCompleteOptions.tsx b/packages/app/src/hooks/useAutoCompleteOptions.tsx index 05c7eccf40..f2412ed736 100644 --- a/packages/app/src/hooks/useAutoCompleteOptions.tsx +++ b/packages/app/src/hooks/useAutoCompleteOptions.tsx @@ -1,20 +1,37 @@ -import { useMemo } from 'react'; +import { useEffect, useMemo } from 'react'; import { JSDataType } from '@hyperdx/common-utils/dist/clickhouse'; import { Field, parseKeyPath, TableConnection, + tcFromSource, } from '@hyperdx/common-utils/dist/core/metadata'; import { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { useFetchFacets } from '@/components/DBSearchPageFilters/hooks'; import { NOW } from '@/config'; -import { - deduplicate2dArray, - useGetKeyValues, - useMultipleAllFields, -} from '@/hooks/useMetadata'; +import { deduplicate2dArray } from '@/hooks/useMetadata'; import { useSource } from '@/source'; -import { mergePath, toArray, useDebounce } from '@/utils'; +import { mergePath, useDebounce } from '@/utils'; + +function chartConfigFromTableConnection( + tc: TableConnection, + timestampValueExpression: string, + dateRange: [Date, Date], +): BuilderChartConfigWithDateRange { + return { + from: { + tableName: tc.tableName, + databaseName: tc.databaseName, + }, + connection: tc.connectionId, + source: undefined, + select: '', + where: '', + timestampValueExpression, + dateRange, + }; +} // Derive top-level Map column names from a fields list. Matches on the // canonical `JSDataType.Map` rather than the raw ClickHouse type string so @@ -162,7 +179,7 @@ export function useAutoCompleteOptions( formatter: ILanguageFormatter, _value: string, { - tableConnection, + tableConnection: _tableConnection, additionalSuggestions, sourceId, dateRange, @@ -177,36 +194,40 @@ export function useAutoCompleteOptions( ) { const { data: source } = useSource({ id: sourceId }); const value = useDebounce(_value, 300); - const tcs = useMemo(() => toArray(tableConnection), [tableConnection]); const effectiveDateRange: [Date, Date] = useMemo( () => dateRange ?? [new Date(NOW - 24 * 60 * 60 * 1000), new Date(NOW)], [dateRange], ); + const tableConnection = _tableConnection + ? Array.isArray(_tableConnection) + ? _tableConnection[0] + : _tableConnection + : undefined; - // Fetch fields, using rollup for map key discovery when available - - // AVK: TODO use useFetchFacets here instead of useMultipleAllFields and useGetKeyValues - // const { - // data: fetchFacetsData, - // isLoading: isFacetsLoading, - // isFetching: isFacetsFetching, - // error, - // loadMoreFacetsForKey, - // loadMoreLoadingKeys, - // extraFacetKeys, - // } = useFetchFacets({ - // chartConfig, - // sourceId: sourceId ?? null, - // dateRange, - // mode: showAllValues ? 'all' : 'exact', - // filterState, - // showMoreFields, - // }); - const { data: fields } = useMultipleAllFields(tcs, { + // Build chart config and keys for fetching values from rollup tables + const chartConfig = useMemo( + () => + chartConfigFromTableConnection( + tableConnection ? tableConnection : tcFromSource(source), + source?.timestampValueExpression ?? '', + effectiveDateRange, + ), + [effectiveDateRange, source, tableConnection], + ); + + const { + data: fetchFacetsData, + isLoading: isFacetsLoading, + loadMoreFacetsForKey, + } = useFetchFacets({ + chartConfig, + sourceId: sourceId ?? null, dateRange: effectiveDateRange, - timestampValueExpression: source?.timestampValueExpression, + mode: 'all', + deferLoadingKeyValues: true, }); + const fields = fetchFacetsData.keys; const { fieldCompleteOptions, fieldCompleteMap } = useMemo(() => { const _columns = (fields ?? []).filter(c => c.jsType !== null); @@ -252,25 +273,6 @@ export function useAutoCompleteOptions( [fieldCompleteMap, fieldNameAtCursor], ); - // Build chart config and keys for fetching values from rollup tables - const chartConfig = useMemo( - () => - tcs.length > 0 - ? { - connection: tcs[0].connectionId, - from: { - databaseName: tcs[0].databaseName, - tableName: tcs[0].tableName, - }, - timestampValueExpression: source?.timestampValueExpression ?? '', - select: '', - where: '', - dateRange: effectiveDateRange, - } - : undefined, - [tcs, effectiveDateRange, source?.timestampValueExpression], - ); - // Map columns from the field list, so a path like `['LogAttributes', '1']` // on a Map(String, ...) renders as `LogAttributes['1']` instead of the // illegal array `LogAttributes[2]`. HDX-4369. @@ -279,28 +281,20 @@ export function useAutoCompleteOptions( [fields], ); - const searchKeys = useMemo( - () => (searchField ? [mergePath(searchField.path, [], mapColumns)] : []), - [searchField, mapColumns], - ); - - const metadataMVs = tcs[0]?.metadataMVs; - const mode = metadataMVs ? 'all' : 'exact'; - - const { data: keyValues, isFetching: isLoadingValues } = useGetKeyValues({ - mode, - chartConfig, - keys: searchKeys, - metadataMVs, - }); + useEffect(() => { + if (searchField && !searchField.type.startsWith('Map')) { + loadMoreFacetsForKey(mergePath(searchField.path, [], mapColumns)); + } + }, [searchField, loadMoreFacetsForKey, mapColumns]); // Build key-value pair suggestions const keyValCompleteOptions = useMemo< { value: string; label: string }[] >(() => { - if (!keyValues || keyValues.length === 0) return []; + if (!fetchFacetsData.keyValues || fetchFacetsData.keyValues.length === 0) + return []; - return keyValues.flatMap(kv => { + return fetchFacetsData.keyValues.flatMap(kv => { const fieldName = parseKeyPath(kv.key).join('.'); return kv.value.flatMap((v: string | Record) => { if (typeof v === 'object' && v !== null) { @@ -317,12 +311,12 @@ export function useAutoCompleteOptions( return { value: formatted, label: formatted }; }); }); - }, [keyValues, formatter]); + }, [fetchFacetsData.keyValues, formatter]); // Combine all autocomplete options const options = useMemo(() => { return deduplicate2dArray([fieldCompleteOptions, keyValCompleteOptions]); }, [fieldCompleteOptions, keyValCompleteOptions]); - return { options, isLoadingValues, tokenInfo }; + return { options, isLoadingValues: isFacetsLoading, tokenInfo }; } From 6edaf6d1d78a82490e04dd9e694f626e8f97cc4b Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 20:57:45 -0400 Subject: [PATCH 06/24] fix lint and docker builds --- .../src/mcp/tools/sources/describeSource.ts | 6 ++-- .../DBSearchPageFilters/hooks.test.tsx | 35 ------------------- 2 files changed, 3 insertions(+), 38 deletions(-) diff --git a/packages/api/src/mcp/tools/sources/describeSource.ts b/packages/api/src/mcp/tools/sources/describeSource.ts index 453d2b9efd..bd2befde8e 100644 --- a/packages/api/src/mcp/tools/sources/describeSource.ts +++ b/packages/api/src/mcp/tools/sources/describeSource.ts @@ -114,7 +114,7 @@ async function sampleMetricNamesForKind({ timestampValueExpression, signal, }); - const names = nameResults[0]?.value ?? []; + const names = nameResults[0]?.value.map(v => v.toString()) ?? []; if (names.length === 0) return []; // Best-effort enrichment with unit + description. One small query @@ -451,7 +451,7 @@ async function describeSourceSchema( }); for (const { key, value } of results) { if (value.length > 0) { - lowCardinalityValues[key] = value; + lowCardinalityValues[key] = value.map(v => v.toString()); } } } catch { @@ -491,7 +491,7 @@ async function describeSourceSchema( }); for (const { key, value } of results) { if (value.length > 0) { - mapAttributeValues[key] = value; + mapAttributeValues[key] = value.map(v => v.toString()); } } } catch { diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx index dd44722c2d..fb8ae437a8 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx +++ b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx @@ -56,7 +56,6 @@ jest.mock('@/hooks/useMetadata', () => ({ useJsonColumns: jest.fn(), useMapColumns: jest.fn(), useAllFields: jest.fn(), - useAllFieldsAndValues: jest.fn(), useGetKeyValues: jest.fn(), })); @@ -71,9 +70,6 @@ const useDateTimeColumns = jest.mocked(useMetadataModule.useDateTimeColumns); const useJsonColumns = jest.mocked(useMetadataModule.useJsonColumns); const useMapColumns = jest.mocked(useMetadataModule.useMapColumns); const useAllFields = jest.mocked(useMetadataModule.useAllFields); -const useAllFieldsAndValues = jest.mocked( - useMetadataModule.useAllFieldsAndValues, -); const useGetKeyValues = jest.mocked(useMetadataModule.useGetKeyValues); const CHART_CONFIG: BuilderChartConfigWithDateRange = { @@ -173,13 +169,6 @@ function setupDefaultMocks({ withMVs }: { withMVs: boolean }) { isFetching: false, error: null, } as any); - - useAllFieldsAndValues.mockReturnValue({ - data: undefined, - isLoading: false, - isFetching: false, - error: null, - } as any); } describe('useFetchFacets', () => { @@ -204,9 +193,7 @@ describe('useFetchFacets', () => { ); const rawCall = useGetKeyValues.mock.calls.at(-1); - const mvCall = useAllFieldsAndValues.mock.calls.at(-1); expect(rawCall?.[1]?.enabled).toBe(true); - expect(mvCall?.[1]?.enabled).toBe(false); }); it('uses the raw-tables pipeline when mode is exact, even if MVs are available', () => { @@ -225,9 +212,7 @@ describe('useFetchFacets', () => { ); const rawCall = useGetKeyValues.mock.calls.at(-1); - const mvCall = useAllFieldsAndValues.mock.calls.at(-1); expect(rawCall?.[1]?.enabled).toBe(true); - expect(mvCall?.[1]?.enabled).toBe(false); }); it('uses the MV pipeline when mode is all and MVs are available', () => { @@ -246,8 +231,6 @@ describe('useFetchFacets', () => { ); const rawCall = useGetKeyValues.mock.calls.at(-1); - const mvCall = useAllFieldsAndValues.mock.calls.at(-1); - expect(mvCall?.[1]?.enabled).toBe(true); expect(rawCall?.[1]?.enabled).toBe(false); }); }); @@ -261,12 +244,6 @@ describe('useFetchFacets', () => { isFetching: false, error: null, } as any); - useAllFieldsAndValues.mockReturnValue({ - data: [{ key: 'ShouldNotBeUsed', value: ['x'] }], - isLoading: false, - isFetching: false, - error: null, - } as any); const { wrapper } = makeWrapper(); @@ -288,12 +265,6 @@ describe('useFetchFacets', () => { it('returns data from the MV pipeline when that pipeline is active', () => { setupDefaultMocks({ withMVs: true }); - useAllFieldsAndValues.mockReturnValue({ - data: [{ key: 'ServiceName', value: ['api', 'web'] }], - isLoading: false, - isFetching: false, - error: null, - } as any); useGetKeyValues.mockReturnValue({ data: [{ key: 'ShouldNotBeUsed', value: ['x'] }], isLoading: false, @@ -550,12 +521,6 @@ describe('useFetchFacets', () => { describe('loadMoreFacetsForKey (MV pipeline)', () => { it('delegates to getAllKeyValues and merges the result', async () => { setupDefaultMocks({ withMVs: true }); - useAllFieldsAndValues.mockReturnValue({ - data: [{ key: 'ServiceName', value: ['api'] }], - isLoading: false, - isFetching: false, - error: null, - } as any); const getAllKeyValues = jest .fn() .mockResolvedValue([{ key: 'ServiceName', value: ['api', 'web'] }]); From bcfb574271bf397df60bd67f00a43c3aced3790f Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 21:05:23 -0400 Subject: [PATCH 07/24] pr feedback --- .../src/components/DBSearchPageFilters/hooks.ts | 5 ++++- packages/common-utils/src/core/metadata.ts | 14 ++++---------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index d34f209630..f713518d0d 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -209,7 +209,10 @@ function useFacets({ }); return { key, - value: newKeyVals[0].value?.map(val => val.toString()) ?? [], + value: + newKeyVals.length > 0 + ? (newKeyVals[0].value?.map(val => val.toString()) ?? []) + : [], }; } } catch (error) { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 2e559e1657..db50fb86b2 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -59,13 +59,6 @@ export type KeyValues = { value: string[] | number[]; }; -function addMapTextIndexEntry( - kvIndexInfo: KvIndexInfo, - column: string, - key: string, - map: TextIndexMapColumnQueryOptions, -) {} - // See https://github.com/hyperdxio/hyperdx/issues/2163. Inlining a validated // integer literal avoids the `_CAST` wrapper entirely. const inlineNonNegativeInt = (value: number, label: string): string => { @@ -660,8 +653,9 @@ export class Metadata { timestampValueExpression, }); const index = textIndexInfo.kv.indexName; + const separator = textIndexInfo.kv.separator; const sql = chSql` - SELECT splitByString(${{ String: index }}, token)[1] AS key + SELECT splitByString(${{ String: separator }}, token)[1] AS key FROM mergeTreeTextIndex(${{ String: databaseName }}, ${{ String: tableName }}, ${{ String: index }}) WHERE ${partsFilter} GROUP BY key HAVING key != '' @@ -1208,7 +1202,7 @@ export class Metadata { maxValuesPerKey: number; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(Array.from(queryOptions.entries()))}.getMetadataMVKeyValues`; + const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(metadataMVs)}.${JSON.stringify(Array.from(queryOptions.entries()))}.getMetadataMVKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { if (!metadataMVs) { console.warn('getMetadataMVKeyValues: metadataMVs is undefined'); @@ -1992,7 +1986,7 @@ export class Metadata { metadataMVs, }: TableConnection): Promise { return this.cache.getOrFetch( - `${connectionId}.${databaseName}.${tableName}.${metadataMVs}.determineKeyValueFetchingStrategy`, + `${connectionId}.${databaseName}.${tableName}.${JSON.stringify(metadataMVs)}.determineKeyValueFetchingStrategy`, async () => { const columnMetadata = await this.getColumns({ databaseName, From 4f8d9a567ae6ecc96e80a7baff7bd80d36a4f08c Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 21:45:28 -0400 Subject: [PATCH 08/24] fix: http 431 errors from too many query params solved by chunking requests --- .../src/__tests__/metadata.test.ts | 34 +++++++++++++++++++ packages/common-utils/src/core/metadata.ts | 27 ++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 7359939d53..019a5c92b8 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1072,6 +1072,40 @@ describe('Metadata', () => { expect(configArg.select).toContain('groupUniqArray('); expect(configArg.select).toContain('param0'); }); + + // Guards against HTTP 431 from too many URL-encoded query_params. If the + // internal chunk size ever grows past 25, or someone removes the chunking, + // this test flags it before the ClickHouse HTTP request goes over the wire. + it('splits keyExpressions into multiple ClickHouse queries when count exceeds the internal chunk size', async () => { + setupDefaultLogsSchema(); + + const keyExpressions = Array.from( + { length: 30 }, + (_, i) => `LogAttributes['k${i}']`, + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions, + }); + + const mapTextIndexCalls = ( + mockClickhouseClient.query as jest.Mock + ).mock.calls.filter( + (c: any[]) => + typeof c[0].query === 'string' && + c[0].query.includes('startsWith(token,'), + ); + + expect(mapTextIndexCalls).toHaveLength(2); + + const allParamValues = mapTextIndexCalls.flatMap((c: any[]) => + Object.values(c[0].query_params ?? {}), + ); + for (let i = 0; i < 30; i++) { + expect(allParamValues).toContain(`k${i}=`); + } + }); }); describe('getValuesDistribution', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index db50fb86b2..d517e2d03d 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1,5 +1,5 @@ import type { ClickHouseSettings } from '@clickhouse/client-common'; -import { omit, pick } from 'lodash'; +import { chunk, omit, pick } from 'lodash'; import { BaseClickhouseClient, @@ -47,6 +47,12 @@ import { export const DEFAULT_METADATA_MAX_ROWS_TO_READ = 3e6; const DEFAULT_MAX_KEYS = 1000; +// Cap keys per dispatched query: each key becomes one or more URL-encoded +// query_params on the ClickHouse HTTP call, and a few dozen is enough to +// exceed proxy header limits (HTTP 431). Recursive chunks reuse the cached +// strategy lookup, so the only cost is extra parallel HTTP calls. +const GET_ALL_KEY_VALUES_CHUNK_SIZE = 40; + type KeyFetchingStrategies = { mapTextIndexLookup: TextIndexInfo[]; nativeTextIndexLookup: SkipIndexMetadata[]; @@ -2083,6 +2089,25 @@ export class Metadata { }): Promise { if (keyExpressions.length === 0) return []; + if (keyExpressions.length > GET_ALL_KEY_VALUES_CHUNK_SIZE) { + const batched = await Promise.all( + chunk(keyExpressions, GET_ALL_KEY_VALUES_CHUNK_SIZE).map(batch => + this.getAllKeyValues({ + databaseName, + tableName, + keyExpressions: batch, + maxValuesPerKey, + connectionId, + metadataMVs, + dateRange, + timestampValueExpression, + signal, + }), + ), + ); + return batched.flat(); + } + // Parse all keys into (rollupColumn, rollupKey) pairs const parsed = keyExpressions.map(keyExpr => { const path = parseKeyPath(keyExpr); From 6453af2674ce395bfc0434b010e3147236d4bb83 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Tue, 14 Jul 2026 22:12:48 -0400 Subject: [PATCH 09/24] fix tests --- .../src/__tests__/metadata.test.ts | 66 ++++++++++--------- packages/common-utils/src/core/metadata.ts | 2 + packages/common-utils/src/queryParser.ts | 15 +++-- 3 files changed, 45 insertions(+), 38 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 019a5c92b8..198d6f96ef 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1073,14 +1073,16 @@ describe('Metadata', () => { expect(configArg.select).toContain('param0'); }); - // Guards against HTTP 431 from too many URL-encoded query_params. If the - // internal chunk size ever grows past 25, or someone removes the chunking, - // this test flags it before the ClickHouse HTTP request goes over the wire. + // Guards against HTTP 431 from too many URL-encoded query_params. Passes + // more keys than the private GET_ALL_KEY_VALUES_CHUNK_SIZE (currently 40) + // so the recursion has to fire at least two chunks; if someone removes + // the chunking this test flags it before the ClickHouse HTTP request + // goes over the wire. Bump the key count if that constant ever exceeds 49. it('splits keyExpressions into multiple ClickHouse queries when count exceeds the internal chunk size', async () => { setupDefaultLogsSchema(); const keyExpressions = Array.from( - { length: 30 }, + { length: 50 }, (_, i) => `LogAttributes['k${i}']`, ); @@ -1097,12 +1099,12 @@ describe('Metadata', () => { c[0].query.includes('startsWith(token,'), ); - expect(mapTextIndexCalls).toHaveLength(2); + expect(mapTextIndexCalls.length).toBeGreaterThanOrEqual(2); const allParamValues = mapTextIndexCalls.flatMap((c: any[]) => Object.values(c[0].query_params ?? {}), ); - for (let i = 0; i < 30; i++) { + for (let i = 0; i < 50; i++) { expect(allParamValues).toContain(`k${i}=`); } }); @@ -1312,12 +1314,21 @@ describe('Metadata', () => { }); describe('getMapKeys', () => { - // Fresh real cache so cache-key assertions are meaningful per test + // Fresh real cache so cache-key assertions are meaningful per test. + // Also stub out getMapColumnTextIndexes — these tests exercise the + // raw-table sampledKeys path and don't set up a text index, so we don't + // want its underlying (getServerVersion / getColumns / getSkipIndices / + // isClickHouseCloud) queries consuming slots in the mockResolvedValueOnce + // chain each test carefully composes. const buildMetadata = () => { const realCache = new ( jest.requireActual('../core/metadata') as any ).MetadataCache(); - return new Metadata(mockClickhouseClient, realCache); + const md = new Metadata(mockClickhouseClient, realCache); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + return md; }; const lowCardinalityMapColumn = { @@ -1910,6 +1921,9 @@ describe('Metadata', () => { jest.requireActual('../core/metadata') as any ).MetadataCache(); const md = new Metadata(mockClickhouseClient, realCache); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); // Mock getColumns → returns one Map column (mockClickhouseClient.query as jest.Mock) @@ -1974,6 +1988,9 @@ describe('Metadata', () => { jest.requireActual('../core/metadata') as any ).MetadataCache(); const md = new Metadata(mockClickhouseClient, realCache); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); (mockClickhouseClient.query as jest.Mock) .mockResolvedValueOnce({ @@ -2364,7 +2381,11 @@ describe('parametric aggregate arguments are inlined as literals', () => { const realCache = new ( jest.requireActual('../core/metadata') as any ).MetadataCache(); - return new Metadata(mockClickhouseClient, realCache); + const md = new Metadata(mockClickhouseClient, realCache); + jest + .spyOn(md, 'getMapColumnTextIndexes') + .mockResolvedValue(new Map() as any); + return md; }; beforeEach(() => { @@ -2424,27 +2445,10 @@ describe('parametric aggregate arguments are inlined as literals', () => { ]; it.each(badValues)( - 'getMapKeys throws when maxKeys is %s and never runs the sampledKeys query', + 'getMapKeys throws when maxKeys is %s and never runs any ClickHouse query', async (_label, badValue) => { const md = buildMetadata(); - (mockClickhouseClient.query as jest.Mock).mockResolvedValueOnce({ - json: () => - Promise.resolve({ - data: [ - { - name: 'LogAttributes', - type: 'Map(String, String)', - default_type: '', - default_expression: '', - comment: '', - codec_expression: '', - ttl_expression: '', - }, - ], - }), - }); - await expect( md.getMapKeys({ databaseName: 'otel', @@ -2455,10 +2459,10 @@ describe('parametric aggregate arguments are inlined as literals', () => { }), ).rejects.toThrow(/maxKeys must be a non-negative integer/); - expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1); - const onlyCall = (mockClickhouseClient.query as jest.Mock).mock - .calls[0][0]; - expect(onlyCall.query).not.toContain('groupUniqArrayArray'); + // Validation happens synchronously at the top of getMapKeys before + // any of the text-index / rollup / raw-scan paths fire, so no + // ClickHouse round-trip is wasted on a value we already know is bad. + expect(mockClickhouseClient.query).not.toHaveBeenCalled(); }, ); }); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index d517e2d03d..cc23997259 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -595,6 +595,8 @@ export class Metadata { timestampValueExpression?: string; signal?: AbortSignal; }) { + inlineNonNegativeInt(maxKeys, 'maxKeys'); + // Align date range to rollup granularity for consistent cache keys const alignedDateRange = metadataMVs && dateRange diff --git a/packages/common-utils/src/queryParser.ts b/packages/common-utils/src/queryParser.ts index e3c711ec8c..0c5a2621d8 100644 --- a/packages/common-utils/src/queryParser.ts +++ b/packages/common-utils/src/queryParser.ts @@ -1109,13 +1109,14 @@ export function skipIndexMatches( }, ): boolean { if (idx.type !== expectedType) return false; - if ( - expectedType === 'text' && - options?.tokenizer && - idx.typeFull.replaceAll(' ', '') !== - `text(tokenizer='${options.tokenizer}')` - ) - return false; + if (expectedType === 'text' && options?.tokenizer) { + // ClickHouse's system.data_skipping_indices.type_full can render the + // tokenizer as either `tokenizer=array` or `tokenizer='array'` depending on + // server version. Delegate to the shared parser, which strips quotes and + // handles whitespace, instead of matching a single literal shape. + const parsed = parseTokenizerFromTextIndex(idx); + if (parsed?.type !== options.tokenizer) return false; + } return true; } From 957a47babd1982a529dd4b683749b59087ce3453 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 10:46:41 -0400 Subject: [PATCH 10/24] fix behavior and tests --- .../DBSearchPageFilters/hooks.test.tsx | 229 ++++++++++++++---- .../components/DBSearchPageFilters/hooks.ts | 85 ++++--- .../__tests__/useAutoCompleteOptions.test.tsx | 123 +++++++--- 3 files changed, 318 insertions(+), 119 deletions(-) diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx index fb8ae437a8..04b7e01746 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx +++ b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx @@ -16,18 +16,26 @@ import { useFetchFacets } from './hooks'; enableMapSet(); /** - * These tests focus on the two code paths inside `useFetchFacets`: + * These tests focus on the two code paths inside `useFacets`: * - * 1. Raw-tables pipeline (`useFacetsFromRawTables`): triggered when the - * source has no metadata materialized views, OR when `mode === 'exact'`. - * 2. Materialized-view pipeline (`useAllFacetsFromMVs`): triggered only when - * the source has metadata materialized views AND `mode === 'all'`. + * 1. Raw-tables pipeline: active when `mode === 'exact'`. Calls + * `useGetKeyValues({ mode: 'exact' })` and scopes "Load more" through + * `metadata.getKeyValuesWithMVs`. + * 2. "All" pipeline: active when `mode === 'all'`. Calls + * `useGetKeyValues({ mode: 'all' })` — whose intelligent router picks + * MV/text-index/raw internally — and delegates "Load more" to + * `metadata.getAllKeyValues`. * - * Plus the shared state layer that merges "load more" results into whichever - * pipeline is active (union — primary values are preserved and never - * overridden by extras) and resets that state whenever the query scope that - * produced the extras changes (source, date range, mode, filter state, or - * the where clause). + * Both paths share a single `useGetKeyValues` call whose behavior is driven + * entirely by the `mode` argument. Selection is mode-only; the presence or + * absence of metadata materialized views on the source does not affect which + * path runs. + * + * Plus the shared state layer that merges "load more" results into the + * active path (union — primary values are preserved and never overridden by + * extras) and resets that state whenever the query scope that produced the + * extras changes (source, date range, mode, filter state, or the where + * clause). */ jest.mock('@/api', () => ({ @@ -177,7 +185,27 @@ describe('useFetchFacets', () => { }); describe('pipeline selection', () => { - it('uses the raw-tables pipeline when the source has no metadata materialized views', () => { + it('routes useGetKeyValues with mode="exact" when mode is exact', () => { + setupDefaultMocks({ withMVs: false }); + const { wrapper } = makeWrapper(); + + renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'exact', + }), + { wrapper }, + ); + + const call = useGetKeyValues.mock.calls.at(-1); + expect(call?.[0]?.mode).toBe('exact'); + expect(call?.[1]?.enabled).toBe(true); + }); + + it('routes useGetKeyValues with mode="all" when mode is all', () => { setupDefaultMocks({ withMVs: false }); const { wrapper } = makeWrapper(); @@ -192,11 +220,12 @@ describe('useFetchFacets', () => { { wrapper }, ); - const rawCall = useGetKeyValues.mock.calls.at(-1); - expect(rawCall?.[1]?.enabled).toBe(true); + const call = useGetKeyValues.mock.calls.at(-1); + expect(call?.[0]?.mode).toBe('all'); + expect(call?.[1]?.enabled).toBe(true); }); - it('uses the raw-tables pipeline when mode is exact, even if MVs are available', () => { + it('selection is mode-only: MV presence does not change which mode is passed', () => { setupDefaultMocks({ withMVs: true }); const { wrapper } = makeWrapper(); @@ -211,12 +240,38 @@ describe('useFetchFacets', () => { { wrapper }, ); - const rawCall = useGetKeyValues.mock.calls.at(-1); - expect(rawCall?.[1]?.enabled).toBe(true); + const call = useGetKeyValues.mock.calls.at(-1); + expect(call?.[0]?.mode).toBe('exact'); }); + }); - it('uses the MV pipeline when mode is all and MVs are available', () => { - setupDefaultMocks({ withMVs: true }); + // Autocomplete opts into `deferLoadingKeyValues: true` so it can render + // field-name suggestions from `data.keys` without triggering the values + // query — only firing that query once the user is actively searching on + // a fully-formed key. Guard against a regression that couples the two. + describe('deferLoadingKeyValues', () => { + it('disables the useGetKeyValues query when deferLoadingKeyValues is true', () => { + setupDefaultMocks({ withMVs: false }); + const { wrapper } = makeWrapper(); + + renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'all', + deferLoadingKeyValues: true, + }), + { wrapper }, + ); + + const call = useGetKeyValues.mock.calls.at(-1); + expect(call?.[1]?.enabled).toBe(false); + }); + + it('enables the useGetKeyValues query when deferLoadingKeyValues is false or omitted', () => { + setupDefaultMocks({ withMVs: false }); const { wrapper } = makeWrapper(); renderHook( @@ -226,24 +281,87 @@ describe('useFetchFacets', () => { sourceId: 'source1', dateRange: DATE_RANGE, mode: 'all', + deferLoadingKeyValues: false, }), { wrapper }, ); - const rawCall = useGetKeyValues.mock.calls.at(-1); - expect(rawCall?.[1]?.enabled).toBe(false); + const call = useGetKeyValues.mock.calls.at(-1); + expect(call?.[1]?.enabled).toBe(true); + }); + + it('does not defer the field metadata query — useAllFields stays enabled', () => { + setupDefaultMocks({ withMVs: false }); + const { wrapper } = makeWrapper(); + + renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'all', + deferLoadingKeyValues: true, + }), + { wrapper }, + ); + + const call = useAllFields.mock.calls.at(-1); + expect(call?.[1]?.enabled).toBe(true); + }); + + it('still surfaces field metadata via data.keys even while deferring values', () => { + setupDefaultMocks({ withMVs: false }); + const { wrapper } = makeWrapper(); + + const { result } = renderHook( + () => + useFetchFacets({ + chartConfig: CHART_CONFIG, + sourceId: 'source1', + dateRange: DATE_RANGE, + mode: 'all', + deferLoadingKeyValues: true, + }), + { wrapper }, + ); + + expect(result.current.data.keys).toEqual([ + { + path: ['ServiceName'], + type: 'LowCardinality(String)', + jsType: 'string', + }, + ]); + expect(result.current.data.keyValues).toBeUndefined(); }); }); describe('data selection', () => { - it('returns data from the raw-tables pipeline when that pipeline is active', () => { + // Route mock responses by the `mode` arg so each pipeline sees a + // distinct fixture — that way an assertion against `data.keyValues` + // actually proves the active pipeline's response is returned. + function mockGetKeyValuesByMode(byMode: { exact: unknown; all: unknown }) { + useGetKeyValues.mockImplementation(((args: { mode?: 'all' | 'exact' }) => + args?.mode === 'all' ? byMode.all : byMode.exact) as any); + } + + it('returns data from the raw-tables pipeline when mode is exact', () => { setupDefaultMocks({ withMVs: false }); - useGetKeyValues.mockReturnValue({ - data: [{ key: 'ServiceName', value: ['api', 'web'] }], - isLoading: false, - isFetching: false, - error: null, - } as any); + mockGetKeyValuesByMode({ + exact: { + data: [{ key: 'ServiceName', value: ['api', 'web'] }], + isLoading: false, + isFetching: false, + error: null, + }, + all: { + data: [{ key: 'ShouldNotBeUsed', value: ['x'] }], + isLoading: false, + isFetching: false, + error: null, + }, + }); const { wrapper } = makeWrapper(); @@ -258,19 +376,27 @@ describe('useFetchFacets', () => { { wrapper }, ); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api', 'web'] }, ]); }); - it('returns data from the MV pipeline when that pipeline is active', () => { + it('returns data from the "all" pipeline when mode is all', () => { setupDefaultMocks({ withMVs: true }); - useGetKeyValues.mockReturnValue({ - data: [{ key: 'ShouldNotBeUsed', value: ['x'] }], - isLoading: false, - isFetching: false, - error: null, - } as any); + mockGetKeyValuesByMode({ + exact: { + data: [{ key: 'ShouldNotBeUsed', value: ['x'] }], + isLoading: false, + isFetching: false, + error: null, + }, + all: { + data: [{ key: 'ServiceName', value: ['api', 'web'] }], + isLoading: false, + isFetching: false, + error: null, + }, + }); const { wrapper } = makeWrapper(); @@ -285,12 +411,12 @@ describe('useFetchFacets', () => { { wrapper }, ); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api', 'web'] }, ]); }); - it('returns undefined when the active pipeline has no data yet', () => { + it('returns undefined keyValues when the active pipeline has no data yet', () => { setupDefaultMocks({ withMVs: false }); const { wrapper } = makeWrapper(); @@ -305,7 +431,10 @@ describe('useFetchFacets', () => { { wrapper }, ); - expect(result.current.data).toBeUndefined(); + // `data.keys` is field metadata (from `useAllFields`) and is + // independent of the values query; it stays defined once metadata + // loads. Only `keyValues` is gated on the active pipeline query. + expect(result.current.data.keyValues).toBeUndefined(); }); }); @@ -431,7 +560,7 @@ describe('useFetchFacets', () => { await result.current.loadMoreFacetsForKey('ServiceName'); }); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api', 'primary-only', 'web', 'db'], @@ -470,7 +599,7 @@ describe('useFetchFacets', () => { await result.current.loadMoreFacetsForKey('NewKey'); }); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, { key: 'NewKey', value: ['n1', 'n2'] }, ]); @@ -508,7 +637,7 @@ describe('useFetchFacets', () => { await result.current.loadMoreFacetsForKey('ServiceName'); }); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, ]); expect(result.current.loadMoreLoadingKeys.has('ServiceName')).toBe(false); @@ -548,7 +677,7 @@ describe('useFetchFacets', () => { expect(getAllKeyValues).toHaveBeenCalledTimes(1); expect(getKeyValuesWithMVs).not.toHaveBeenCalled(); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api', 'web'] }, ]); }); @@ -586,7 +715,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.has('NewKey')).toBe(true); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, { key: 'NewKey', value: ['n1'] }, ]); @@ -594,7 +723,7 @@ describe('useFetchFacets', () => { rerender({ sourceId: 'source2' }); expect(result.current.extraFacetKeys.size).toBe(0); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, ]); }); @@ -629,7 +758,7 @@ describe('useFetchFacets', () => { await result.current.loadMoreFacetsForKey('NewKey'); }); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, { key: 'NewKey', value: ['n1'] }, ]); @@ -639,7 +768,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.size).toBe(0); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, ]); }); @@ -679,7 +808,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.has('NewKey')).toBe(true); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, { key: 'NewKey', value: ['n1'] }, ]); @@ -694,7 +823,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.size).toBe(0); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, ]); }); @@ -730,7 +859,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.has('NewKey')).toBe(true); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, { key: 'NewKey', value: ['n1'] }, ]); @@ -740,7 +869,7 @@ describe('useFetchFacets', () => { }); expect(result.current.extraFacetKeys.size).toBe(0); - expect(result.current.data).toEqual([ + expect(result.current.data.keyValues).toEqual([ { key: 'ServiceName', value: ['api'] }, ]); }); diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index f713518d0d..dfa3cfdd86 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -147,7 +147,7 @@ function useFacets({ [chartConfig, dateRange, mode], ); - const { data: rawExactFacets, ...rest } = useGetKeyValues( + const { data: rawFacets, ...rest } = useGetKeyValues( { chartConfig: facetsChartConfig, limit: INITIAL_LOAD_LIMIT, @@ -158,13 +158,13 @@ function useFacets({ ); // Map the (escaped) result keys back to the original UI keys. - const exactFacets = useMemo( + const facets = useMemo( () => - rawExactFacets?.map(f => ({ + rawFacets?.map(f => ({ ...f, key: sqlKeyToUiKey.get(f.key) ?? f.key, })), - [rawExactFacets, sqlKeyToUiKey], + [rawFacets, sqlKeyToUiKey], ); const metadata = useMetadataWithSettings(); @@ -172,10 +172,10 @@ function useFacets({ async (key: string): Promise => { try { const sqlKey = toQuotedClickHouseKeyExpression(key, knownColumns); - const strippedFilterState: FilterState = { ...filterState }; - delete strippedFilterState[key]; - if (sqlKey !== key) delete strippedFilterState[sqlKey]; if (mode === 'exact') { + const strippedFilterState: FilterState = { ...filterState }; + delete strippedFilterState[key]; + if (sqlKey !== key) delete strippedFilterState[sqlKey]; const newKeyVals = await metadata.getKeyValuesWithMVs({ chartConfig: { ...chartConfig, @@ -194,35 +194,36 @@ function useFacets({ key, value: newKeyVals[0].value?.map(val => val.toString()) ?? [], }; - } else { - if (!source) - throw new Error('loadMoreFacetsForKey: source must be defined'); - const newKeyVals = await metadata.getAllKeyValues({ - databaseName: source.from.databaseName, - tableName: source.from.tableName, - connectionId: source.connection, - metadataMVs: tableConnection.metadataMVs, - keyExpressions: [sqlKey], - maxValuesPerKey: LOAD_MORE_LOAD_LIMIT, - dateRange, - timestampValueExpression: source?.timestampValueExpression, - }); - return { - key, - value: - newKeyVals.length > 0 - ? (newKeyVals[0].value?.map(val => val.toString()) ?? []) - : [], - }; } + + if (!source) { + throw new Error('loadMoreFacetsForKey: source must be defined'); + } + const newKeyVals = await metadata.getAllKeyValues({ + databaseName: source.from.databaseName, + tableName: source.from.tableName, + connectionId: source.connection, + metadataMVs: tableConnection.metadataMVs, + keyExpressions: [sqlKey], + maxValuesPerKey: LOAD_MORE_LOAD_LIMIT, + dateRange, + timestampValueExpression: source.timestampValueExpression, + }); + return { + key, + value: + newKeyVals.length > 0 + ? (newKeyVals[0].value?.map(val => val.toString()) ?? []) + : [], + }; } catch (error) { console.error('failed to fetch more keys', error); } return undefined; }, [ - tableConnection, mode, + tableConnection, metadata, chartConfig, dateRange, @@ -236,7 +237,7 @@ function useFacets({ return { ...rest, error: allFieldsError ?? rest.error, - data: { keys: allFields, keyValues: exactFacets }, + data: { keys: allFields, keyValues: facets }, isLoading: isAllFieldsLoading || rest.isLoading, loadMoreFacetsForKey, }; @@ -259,7 +260,6 @@ export function useFetchFacets({ showMoreFields?: boolean; deferLoadingKeyValues?: boolean; }) { - // Exact pipeline: fetch values for discovered keys const facetsQuery = useFacets({ chartConfig, sourceId, @@ -272,14 +272,19 @@ export function useFetchFacets({ }); const [extraFacets, setExtraFacets] = useState(null); - const facets = useMemo(() => { - const facets = facetsQuery.data.keyValues ?? []; - if (!extraFacets || extraFacets.length === 0) return facets; - const seenFacets = new Set(); + const facets = useMemo(() => { + const base = facetsQuery.data.keyValues; + const hasExtras = !!extraFacets && extraFacets.length > 0; + + if (base === undefined && !hasExtras) return undefined; + if (!hasExtras) return base; + if (base === undefined) return extraFacets ?? undefined; + + const seenFacets = new Set(); const output: Facet[] = []; - for (const facet of facets) { + for (const facet of base) { seenFacets.add(facet.key); - const extraFacet = extraFacets.find(ef => ef.key === facet.key); + const extraFacet = extraFacets!.find(ef => ef.key === facet.key); if (extraFacet) { // Union values: primary is query-scoped and must not be overridden; // extras from "Load More" only append (see PR #2329, commit 8938b05ef). @@ -304,9 +309,11 @@ export function useFetchFacets({ return output; }, [facetsQuery.data, extraFacets]); - const [extraFacetKeys, setExtraFacetKeys] = useState>(new Set()); + const [extraFacetKeys, setExtraFacetKeys] = useState>( + () => new Set(), + ); const [loadMoreLoadingKeys, setLoadMoreLoadingKeys] = useState>( - new Set(), + () => new Set(), ); const areExtraFacetsLoading = loadMoreLoadingKeys.size > 0; const loadMoreFacetsForKey = useCallback( @@ -352,7 +359,7 @@ export function useFetchFacets({ return { ...facetsQuery, - data: { keys: facetsQuery.data?.keys, keyValues: facets }, + data: { keys: facetsQuery.data.keys, keyValues: facets }, loadMoreFacetsForKey, areExtraFacetsLoading, loadMoreLoadingKeys, diff --git a/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx b/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx index 1bd36b2890..dc71cfcabf 100644 --- a/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx +++ b/packages/app/src/hooks/__tests__/useAutoCompleteOptions.test.tsx @@ -1,5 +1,8 @@ +import React from 'react'; +import { enableMapSet } from 'immer'; import { JSDataType } from '@hyperdx/common-utils/dist/clickhouse'; import { Field } from '@hyperdx/common-utils/dist/core/metadata'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { renderHook } from '@testing-library/react'; import { LuceneLanguageFormatter } from '@/components/SearchInput/SearchInputV2'; @@ -8,19 +11,67 @@ import { tokenizeAtCursor, useAutoCompleteOptions, } from '@/hooks/useAutoCompleteOptions'; -import { useGetKeyValues, useMultipleAllFields } from '@/hooks/useMetadata'; +import { useAllFields, useGetKeyValues } from '@/hooks/useMetadata'; + +enableMapSet(); + +// The hook transitively pulls in `useFetchFacets`, which touches almost +// every hook in `useMetadata` plus `usePinnedFilters` and `api.useMe`. +// Mocking each direct dependency is cheaper than wiring up a live +// QueryClient + source config just to render the tree. +jest.mock('@/api', () => ({ + __esModule: true, + default: { + useMe: jest + .fn() + .mockReturnValue({ data: null, isFetched: true, isLoading: false }), + }, +})); -// Mock dependencies -jest.mock('../useMetadata', () => ({ - ...jest.requireActual('../useMetadata.tsx'), - useMultipleAllFields: jest.fn(), - useGetKeyValues: jest.fn(), +jest.mock('@/searchFilters', () => ({ + __esModule: true, + usePinnedFilters: jest.fn().mockReturnValue({ + isFieldPinned: () => false, + isSharedFieldPinned: () => false, + }), + escapeFilterStateKeys: (state: unknown) => state, })); +jest.mock('../useMetadata', () => { + const actual = jest.requireActual('../useMetadata.tsx'); + return { + __esModule: true, + ...actual, + useMetadataWithSettings: jest.fn().mockReturnValue({ + getKeyValuesWithMVs: jest.fn(), + getAllKeyValues: jest.fn(), + }), + useAllFields: jest.fn(), + useMultipleAllFields: jest.fn(), + useGetKeyValues: jest.fn(), + useColumns: jest.fn().mockReturnValue({ data: [], isLoading: false }), + useDateTimeColumns: jest.fn().mockReturnValue(new Map()), + useJsonColumns: jest.fn().mockReturnValue({ data: [] }), + useMapColumns: jest.fn().mockReturnValue({ data: [] }), + }; +}); + jest.mock('../../source', () => ({ + __esModule: true, useSource: jest.fn().mockReturnValue({ data: undefined }), + useSources: jest.fn().mockReturnValue({ data: [], isLoading: false }), })); +function makeWrapper() { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const wrapper: React.FC<{ children: React.ReactNode }> = ({ children }) => ( + {children} + ); + return wrapper; +} + const luceneFormatter = new LuceneLanguageFormatter(); const mockFields: Field[] = [ @@ -48,26 +99,32 @@ const mockTableConnection = { }; describe('useAutoCompleteOptions', () => { + let wrapper: React.FC<{ children: React.ReactNode }>; + beforeEach(() => { - // Reset mocks before each test jest.clearAllMocks(); - // Setup default mock implementations - (useMultipleAllFields as jest.Mock).mockReturnValue({ + (useAllFields as jest.Mock).mockReturnValue({ data: mockFields, + isLoading: false, + error: null, }); (useGetKeyValues as jest.Mock).mockReturnValue({ data: null, isFetching: false, }); + + wrapper = makeWrapper(); }); it('should return field options with correct lucene formatting', () => { - const { result } = renderHook(() => - useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { - tableConnection: mockTableConnection, - }), + const { result } = renderHook( + () => + useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { + tableConnection: mockTableConnection, + }), + { wrapper }, ); expect(result.current.options).toEqual([ @@ -97,14 +154,16 @@ describe('useAutoCompleteOptions', () => { isFetching: false, }); - const { result } = renderHook(() => - useAutoCompleteOptions( - luceneFormatter, - 'ResourceAttributes.service.name', - { - tableConnection: mockTableConnection, - }, - ), + const { result } = renderHook( + () => + useAutoCompleteOptions( + luceneFormatter, + 'ResourceAttributes.service.name', + { + tableConnection: mockTableConnection, + }, + ), + { wrapper }, ); expect(result.current.options).toEqual([ @@ -147,10 +206,12 @@ describe('useAutoCompleteOptions', () => { isFetching: false, }); - const { result } = renderHook(() => - useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { - tableConnection: mockTableConnection, - }), + const { result } = renderHook( + () => + useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { + tableConnection: mockTableConnection, + }), + { wrapper }, ); expect(result.current.options).toEqual([ @@ -178,11 +239,13 @@ describe('useAutoCompleteOptions', () => { }); it('should handle additional suggestions', () => { - const { result } = renderHook(() => - useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { - tableConnection: mockTableConnection, - additionalSuggestions: ['custom.field'], - }), + const { result } = renderHook( + () => + useAutoCompleteOptions(luceneFormatter, 'ResourceAttributes', { + tableConnection: mockTableConnection, + additionalSuggestions: ['custom.field'], + }), + { wrapper }, ); expect(result.current.options).toEqual([ From 24b51b4f85f3f86400bc220b0a6854df1f711d05 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 11:52:22 -0400 Subject: [PATCH 11/24] fix: pass maxValuesPerKey through metadata keyvalue queries --- .../src/__tests__/metadata.test.ts | 204 ++++++++++++++++++ packages/common-utils/src/core/metadata.ts | 175 ++++++++------- 2 files changed, 302 insertions(+), 77 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 198d6f96ef..bf96d2e54d 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1108,6 +1108,210 @@ describe('Metadata', () => { expect(allParamValues).toContain(`k${i}=`); } }); + + // Regression guard: prior to this fix, the text-index and raw-table + // branches hardcoded `limit: 20`, silently capping `loadMoreFacetsForKey` + // (which requests 10000 values) at 20. Only the MV branch honored + // `maxValuesPerKey`, so "Load More" was a no-op for any column resolved + // 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. + const MAX_VALUES = 7777; + + it('threads maxValuesPerKey into the map text-index query', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["LogAttributes['requestId']"], + maxValuesPerKey: MAX_VALUES, + }); + + const mapTextIndexCall = ( + mockClickhouseClient.query as jest.Mock + ).mock.calls.find( + (c: any[]) => + typeof c[0].query === 'string' && + 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); + }); + + it('threads maxValuesPerKey into the native text-index query', async () => { + setupDefaultLogsSchema(); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['TraceId'], + maxValuesPerKey: MAX_VALUES, + }); + + const nativeTextIndexCall = ( + mockClickhouseClient.query as jest.Mock + ).mock.calls.find( + (c: any[]) => + typeof c[0].query === 'string' && + c[0].query.includes('mergeTreeTextIndex(') && + !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); + }); + + it('threads maxValuesPerKey into the raw-table getKeyValues fallback', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['Body'], + maxValuesPerKey: MAX_VALUES, + }); + + expect(renderChartConfigSpy).toHaveBeenCalled(); + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + expect(configArg.select).toContain(`groupUniqArray(${MAX_VALUES})`); + }); + }); + + // Text-index queries can fail transiently (e.g. an index part merged + // between plan and read, or an older server that doesn't support + // `mergeTreeTextIndex`). Prior to this fix, one rejection propagated + // through `Promise.all` and wiped filter values for every column in + // the batch — regardless of which strategy served each column. + describe('text-index failure isolation', () => { + // Match each strategy to the response shape its own parser expects. + // Text-index returns rows like { key, value }; the MV rollup parses + // { ColumnIdentifier, Key, Values } (see `getMetadataMVKeyValues`, + // which maps NativeColumn rows back to { key: Key, value: Values }). + function mockQueryByStrategy(strategies: { + onMapTextIndex?: () => Promise; + onNativeTextIndex?: () => Promise; + onMVRollup?: () => Promise; + }) { + (mockClickhouseClient.query as jest.Mock).mockImplementation( + ({ query }: any) => { + const q = typeof query === 'string' ? query : ''; + if (q.includes('startsWith(token,')) { + return ( + strategies.onMapTextIndex?.() ?? + Promise.resolve({ json: () => Promise.resolve({ data: [] }) }) + ); + } + if (q.includes('mergeTreeTextIndex(')) { + return ( + strategies.onNativeTextIndex?.() ?? + Promise.resolve({ json: () => Promise.resolve({ data: [] }) }) + ); + } + if (q.includes('ColumnIdentifier =')) { + return ( + strategies.onMVRollup?.() ?? + Promise.resolve({ json: () => Promise.resolve({ data: [] }) }) + ); + } + return Promise.resolve({ + json: () => Promise.resolve({ data: [] }), + }); + }, + ); + } + + it('returns results from other strategies when the native text-index query throws', async () => { + setupDefaultLogsSchema(); + const consoleWarnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => undefined); + + mockQueryByStrategy({ + onNativeTextIndex: () => + Promise.reject(new Error('text index unavailable')), + onMVRollup: () => + Promise.resolve({ + json: () => + Promise.resolve({ + data: [ + { + ColumnIdentifier: 'NativeColumn', + Key: 'ServiceName', + Values: ['api', 'web'], + total_count: 42, + }, + ], + }), + }), + }); + + const result = await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['TraceId', 'ServiceName'], + }); + + expect(result).toEqual([{ key: 'ServiceName', value: ['api', 'web'] }]); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining('getTextIndexKeyValues failed'), + expect.any(Error), + ); + + consoleWarnSpy.mockRestore(); + }); + + it('returns results from other strategies when the map text-index query throws', async () => { + setupDefaultLogsSchema(); + const consoleWarnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => undefined); + + mockQueryByStrategy({ + onMapTextIndex: () => + Promise.reject(new Error('map text index unavailable')), + onMVRollup: () => + Promise.resolve({ + json: () => + Promise.resolve({ + data: [ + { + ColumnIdentifier: 'NativeColumn', + Key: 'ServiceName', + Values: ['api'], + total_count: 7, + }, + ], + }), + }), + }); + + const result = await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ["LogAttributes['requestId']", 'ServiceName'], + }); + + expect(result).toEqual([{ key: 'ServiceName', value: ['api'] }]); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining('getMapTextIndexKeyValues failed'), + expect.any(Error), + ); + + consoleWarnSpy.mockRestore(); + }); + }); }); describe('getValuesDistribution', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index cc23997259..87962166cf 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1077,26 +1077,27 @@ export class Metadata { dateRange: [Date, Date]; timestampValueExpression: string; signal?: AbortSignal; - }): Promise { + }): Promise { const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getMapTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { - const sqlBranches: Array = []; - for (const [columnName, info] of queryOptions.entries()) { - const orChain = concatChSql( - ' OR ', - info.keys.map( - k => - chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`, - ), - ); - const partsFilter = await this.partsOverlapFilter({ - databaseName, - tableName, - dateRange, - timestampValueExpression, - }); - const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; - const sql = chSql` + try { + const sqlBranches: Array = []; + for (const [columnName, info] of queryOptions.entries()) { + const orChain = concatChSql( + ' OR ', + info.keys.map( + k => + chSql`startsWith(token, ${{ String: `${k}${info.separator}` }})`, + ), + ); + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const valueSql = chSql`substring(token, position(token, ${{ String: info.separator }}) + ${{ Int32: info.separator.length }})`; + const sql = chSql` SELECT * FROM ( SELECT ${{ String: columnName }} as column, substring(token, 1, position(token, ${{ String: info.separator }}) - 1) AS key, @@ -1107,34 +1108,44 @@ export class Metadata { AND ${valueSql} != '' GROUP BY column, key )`; - sqlBranches.push(sql); - } - const sql = concatChSql(' UNION ALL ', sqlBranches); + sqlBranches.push(sql); + } + const sql = concatChSql(' UNION ALL ', sqlBranches); - return await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - 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(), - }, - abort_signal: signal, - }) - .then(res => - res.json<{ column: string; key: string; value: string[] }>(), - ) - .then(d => - d.data.map(row => ({ - key: `${row.column}['${row.key}']`, - value: row.value, - })), + return await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + 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(), + }, + abort_signal: signal, + }) + .then(res => + res.json<{ column: string; key: string; value: string[] }>(), + ) + .then(d => + d.data.map(row => ({ + key: `${row.column}['${row.key}']`, + value: row.value, + })), + ); + } catch (error) { + // Text-index queries can fail transiently (part merged mid-read, + // unsupported server, etc.). Isolate the failure so sibling + // strategies (native text index, MV, raw table) still return data. + console.warn( + 'getMapTextIndexKeyValues failed; skipping this strategy for the current batch', + error, ); + return undefined; + } }); } @@ -1151,18 +1162,19 @@ export class Metadata { dateRange: [Date, Date]; timestampValueExpression: string; signal?: AbortSignal; - }): Promise { + }): Promise { const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { - const sqlBranches: Array = []; - for (const [columnName, info] of queryOptions.entries()) { - const partsFilter = await this.partsOverlapFilter({ - databaseName, - tableName, - dateRange, - timestampValueExpression, - }); - const sql = chSql` + try { + const sqlBranches: Array = []; + for (const [columnName, info] of queryOptions.entries()) { + const partsFilter = await this.partsOverlapFilter({ + databaseName, + tableName, + dateRange, + timestampValueExpression, + }); + const sql = chSql` SELECT * FROM ( SELECT ${{ String: columnName }} AS key, groupUniqArray(${{ Int32: info.limit }})(token) AS value @@ -1171,28 +1183,36 @@ export class Metadata { AND token != '' GROUP BY key )`; - sqlBranches.push(sql); - } - const sql = concatChSql(' UNION ALL ', sqlBranches); + sqlBranches.push(sql); + } + const sql = concatChSql(' UNION ALL ', sqlBranches); - const values = await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - 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(), - }, - abort_signal: signal, - }) - .then(res => res.json()) - .then(d => d.data); - return values; + const values = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + 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(), + }, + abort_signal: signal, + }) + .then(res => res.json()) + .then(d => d.data); + return values; + } catch (error) { + // See `getMapTextIndexKeyValues` — same isolation rationale. + console.warn( + 'getTextIndexKeyValues failed; skipping this strategy for the current batch', + error, + ); + return undefined; + } }); } @@ -2151,7 +2171,7 @@ export class Metadata { if (!entry) { entry = { indexName: mapTextIndex.kv.indexName, - limit: 20, + limit: maxValuesPerKey, separator: mapTextIndex.kv.separator, keys: [], }; @@ -2168,7 +2188,7 @@ export class Metadata { if (nativeTextIndex) { nativeTextIndexQueryOptions.set(key.column, { indexName: nativeTextIndex.name, - limit: 20, + limit: maxValuesPerKey, }); continue; } @@ -2260,6 +2280,7 @@ export class Metadata { where: '', }, keys: rawQueryOptions, + limit: maxValuesPerKey, source: undefined, signal, }), From 2c609a576ad68ac2af49f8b714e155e98875956c Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:11:57 -0400 Subject: [PATCH 12/24] pr feedback --- packages/app/src/source.ts | 1 - packages/common-utils/src/core/metadata.ts | 39 ++++++++++++++-------- packages/common-utils/src/types.ts | 2 +- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/packages/app/src/source.ts b/packages/app/src/source.ts index e53fd99e59..35f479b9d3 100644 --- a/packages/app/src/source.ts +++ b/packages/app/src/source.ts @@ -395,7 +395,6 @@ export async function inferTableSourceConfig({ const metadataMVsConfig = rollupMeta ? { metadataMaterializedViews: { - keyRollupTable: `${tableName}_key_rollup_15m`, kvRollupTable: `${tableName}_kv_rollup_15m`, // Fall back to '15 minute' to preserve the prior default when the // MV's `as_select` doesn't contain a recognized bucketing function. diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 87962166cf..43a2bda849 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -698,15 +698,28 @@ export class Metadata { chSql`fromUnixTimestamp64Milli(${{ Int64: alignedDateRange[1].getTime() }})`, ); const timeFilter = chSql`AND Timestamp >= ${startExpr} AND Timestamp <= ${endExpr}`; - const sql = chSql` - SELECT Key - FROM ${tableExpr({ database: databaseName, table: metadataMVs.keyRollupTable })} - WHERE ColumnIdentifier = ${{ String: column }} - ${timeFilter} - GROUP BY Key - ORDER BY sum(count) DESC - LIMIT ${{ Int32: maxKeys }} - `; + let sql: ChSql; + if (metadataMVs.keyRollupTable) { + sql = chSql` + SELECT Key + FROM ${tableExpr({ database: databaseName, table: metadataMVs.keyRollupTable })} + WHERE ColumnIdentifier = ${{ String: column }} + ${timeFilter} + GROUP BY Key + ORDER BY sum(count) DESC + LIMIT ${{ Int32: maxKeys }} + `; + } else { + sql = chSql` + SELECT Key + FROM ${tableExpr({ database: databaseName, table: metadataMVs.kvRollupTable })} + WHERE ColumnIdentifier = ${{ String: column }} + ${timeFilter} + GROUP BY Key + ORDER BY sum(count) DESC + LIMIT ${{ Int32: maxKeys }} + `; + } return await this.clickhouseClient .query<'JSON'>({ @@ -2136,9 +2149,9 @@ export class Metadata { const isMapKey = path.length >= 2; return { keyExpression: keyExpr, - rollupColumn: isMapKey ? path[0] : 'NativeColumn', - rollupKey: isMapKey ? path[1] : path[0], - column: path[0], + rollupColumn: isMapKey ? unquoteIdentifier(path[0]) : 'NativeColumn', + rollupKey: isMapKey ? path[1] : unquoteIdentifier(path[0]), + column: unquoteIdentifier(path[0]), mapKey: isMapKey ? path[1] : undefined, }; }); @@ -2196,7 +2209,7 @@ export class Metadata { // then check metadataMVs const metadataMVEntry = keyValueFetchingStrategies.metadataMVs.find( - v => v.columnName === key.column.replaceAll('`', ''), + v => v.columnName === key.column, ); if (metadataMVEntry) { let tableEntry = metadataMVQueryOptions.get(metadataMVEntry.mvName); diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 792692de98..c2d82b7daa 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -1744,7 +1744,7 @@ export type MaterializedViewConfiguration = z.infer< >; export const MetadataMaterializedViewsSchema = z.object({ - keyRollupTable: z.string().min(1, 'Key rollup table name is required'), + keyRollupTable: z.string().nullish(), kvRollupTable: z.string().min(1, 'KV rollup table name is required'), granularity: SQLIntervalSchema, }); From 8623cd3be25b97a3d167007eba6f3a2cb17e14f7 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 14:37:21 -0400 Subject: [PATCH 13/24] fix e2e tests --- packages/app/src/source.ts | 6 +- .../filter-modes-and-more-filters.spec.ts | 102 ------------------ .../src/__tests__/metadata.test.ts | 48 +++++++++ packages/common-utils/src/core/metadata.ts | 76 ++++++++----- 4 files changed, 101 insertions(+), 131 deletions(-) delete mode 100644 packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts diff --git a/packages/app/src/source.ts b/packages/app/src/source.ts index 35f479b9d3..904626d093 100644 --- a/packages/app/src/source.ts +++ b/packages/app/src/source.ts @@ -386,9 +386,7 @@ export async function inferTableSourceConfig({ connectionId, }), ]); - return keyMeta != null && kvMeta != null - ? { keyMeta, kvMeta } - : undefined; + return kvMeta != null ? { keyMeta, kvMeta } : undefined; })() : undefined; @@ -399,7 +397,7 @@ export async function inferTableSourceConfig({ // Fall back to '15 minute' to preserve the prior default when the // MV's `as_select` doesn't contain a recognized bucketing function. granularity: - inferGranularityFromMVSelect(rollupMeta.keyMeta.as_select) ?? + inferGranularityFromMVSelect(rollupMeta.kvMeta.as_select) ?? '15 minute', }, } diff --git a/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts b/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts deleted file mode 100644 index 5cc016b852..0000000000 --- a/packages/app/tests/e2e/features/search/filter-modes-and-more-filters.spec.ts +++ /dev/null @@ -1,102 +0,0 @@ -import { SearchPage } from '../../page-objects/SearchPage'; -import { expect, test } from '../../utils/base-test'; -import { DEFAULT_LOGS_SOURCE_NAME } from '../../utils/constants'; - -const FILTER_QUERY_TIMEOUT = 20_000; - -const LOW_CARDINALITY_FILTER = 'SeverityText'; -const NON_LOW_CARDINALITY_FILTER = 'TraceId'; - -async function openFilterSettings(searchPage: SearchPage) { - await searchPage.page - .getByRole('button', { name: 'Filter settings' }) - .click(); -} - -async function setShowAllValues(searchPage: SearchPage, checked: boolean) { - await openFilterSettings(searchPage); - const showAllCheckbox = searchPage.page.getByRole('checkbox', { - name: 'Show All Values', - }); - const currentlyChecked = await showAllCheckbox.isChecked(); - if (currentlyChecked !== checked) { - await showAllCheckbox.click(); - } - await searchPage.page.keyboard.press('Escape'); -} - -test.describe( - 'Filter modes (all vs exact) and More filters', - { tag: ['@search'] }, - () => { - let searchPage: SearchPage; - - test.beforeEach(async ({ page }) => { - searchPage = new SearchPage(page); - await searchPage.goto(); - await searchPage.selectSource(DEFAULT_LOGS_SOURCE_NAME); - await searchPage.timePicker.selectRelativeTime('Last 1 hour'); - await searchPage.table.waitForRowsToPopulate(); - }); - - test('all mode loads low-cardinality filters by default', async () => { - await setShowAllValues(searchPage, true); - - await expect( - searchPage.filters.getFilterGroup(LOW_CARDINALITY_FILTER), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - - await searchPage.filters.openFilterGroup(LOW_CARDINALITY_FILTER); - await expect( - searchPage.filters.getFilterCheckboxInput( - LOW_CARDINALITY_FILTER, - 'info', - ), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - }); - - test('exact mode loads low-cardinality filters when Show All Values is off', async () => { - await setShowAllValues(searchPage, false); - - await expect( - searchPage.filters.getFilterGroup(LOW_CARDINALITY_FILTER), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - - await searchPage.filters.openFilterGroup(LOW_CARDINALITY_FILTER); - await expect( - searchPage.filters.getFilterCheckboxInput( - LOW_CARDINALITY_FILTER, - 'info', - ), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - }); - - test('More filters reveals non-low-cardinality columns in all mode', async () => { - await setShowAllValues(searchPage, true); - - await expect( - searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), - ).toHaveCount(0); - - await searchPage.filters.showMoreFilters(); - - await expect( - searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - }); - - test('More filters reveals non-low-cardinality columns in exact mode', async () => { - await setShowAllValues(searchPage, false); - - await expect( - searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), - ).toHaveCount(0); - - await searchPage.filters.showMoreFilters(); - - await expect( - searchPage.filters.getFilterGroup(NON_LOW_CARDINALITY_FILTER), - ).toBeVisible({ timeout: FILTER_QUERY_TIMEOUT }); - }); - }, -); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index bf96d2e54d..b3e9e81d2f 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1201,10 +1201,13 @@ describe('Metadata', () => { // Text-index returns rows like { key, value }; the MV rollup parses // { ColumnIdentifier, Key, Values } (see `getMetadataMVKeyValues`, // which maps NativeColumn rows back to { key: Key, value: Values }). + // The raw-table fallback aliases columns as paramN and returns a single + // row like { param0: [...], param1: [...] } (see `getKeyValues`). function mockQueryByStrategy(strategies: { onMapTextIndex?: () => Promise; onNativeTextIndex?: () => Promise; onMVRollup?: () => Promise; + onRawTable?: () => Promise; }) { (mockClickhouseClient.query as jest.Mock).mockImplementation( ({ query }: any) => { @@ -1227,6 +1230,12 @@ describe('Metadata', () => { Promise.resolve({ json: () => Promise.resolve({ data: [] }) }) ); } + if (q.includes('AS param')) { + return ( + strategies.onRawTable?.() ?? + Promise.resolve({ json: () => Promise.resolve({ data: [] }) }) + ); + } return Promise.resolve({ json: () => Promise.resolve({ data: [] }), }); @@ -1311,6 +1320,45 @@ describe('Metadata', () => { consoleWarnSpy.mockRestore(); }); + + it('returns results from other strategies when the raw-table getKeyValues fallback throws', async () => { + setupDefaultLogsSchema(); + const consoleWarnSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => undefined); + + mockQueryByStrategy({ + onRawTable: () => + Promise.reject(new Error('raw table query timed out')), + onMVRollup: () => + Promise.resolve({ + json: () => + Promise.resolve({ + data: [ + { + ColumnIdentifier: 'NativeColumn', + Key: 'ServiceName', + Values: ['api', 'web'], + total_count: 42, + }, + ], + }), + }), + }); + + const result = await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: ['Body', 'ServiceName'], + }); + + expect(result).toEqual([{ key: 'ServiceName', value: ['api', 'web'] }]); + expect(consoleWarnSpy).toHaveBeenCalledWith( + expect.stringContaining('getKeyValues (raw table) failed'), + expect.any(Error), + ); + + consoleWarnSpy.mockRestore(); + }); }); }); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 43a2bda849..1d697989d1 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -640,18 +640,26 @@ export class Metadata { WHERE ${partsFilter} GROUP BY key HAVING key != '' LIMIT ${{ Int32: maxKeys }}`; - const keys = await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - query_params: sql.params, - connectionId, - clickhouse_settings: this.getClickHouseSettings(), - }) - .then(r => r.json<{ key: string }>()) - .then(d => d.data.map(r => r.key).filter(Boolean)); - if (keys.length > 0) { - this.cache.set(cacheKey, keys); - return keys; + try { + const keys = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(r => r.json<{ key: string }>()) + .then(d => d.data.map(r => r.key).filter(Boolean)); + if (keys.length > 0) { + this.cache.set(cacheKey, keys); + return keys; + } + } catch (e) { + console.warn( + 'getMapKeys rollup query failed for key text index query', + e, + ); + return []; } } else if (textIndexInfo?.kv?.indexName) { const partsFilter = await this.partsOverlapFilter({ @@ -668,18 +676,26 @@ export class Metadata { WHERE ${partsFilter} GROUP BY key HAVING key != '' LIMIT ${{ Int32: maxKeys }}`; - const keys = await this.clickhouseClient - .query<'JSON'>({ - query: sql.sql, - query_params: sql.params, - connectionId, - clickhouse_settings: this.getClickHouseSettings(), - }) - .then(r => r.json<{ key: string }>()) - .then(d => d.data.map(r => r.key).filter(Boolean)); - if (keys.length > 0) { - this.cache.set(cacheKey, keys); - return keys; + try { + const keys = await this.clickhouseClient + .query<'JSON'>({ + query: sql.sql, + query_params: sql.params, + connectionId, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(r => r.json<{ key: string }>()) + .then(d => d.data.map(r => r.key).filter(Boolean)); + if (keys.length > 0) { + this.cache.set(cacheKey, keys); + return keys; + } + } catch (e) { + console.warn( + 'getMapKeys rollup query failed for kv text index query', + e, + ); + return []; } } @@ -1243,7 +1259,7 @@ export class Metadata { maxValuesPerKey: number; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(metadataMVs)}.${JSON.stringify(Array.from(queryOptions.entries()))}.getMetadataMVKeyValues`; + const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(metadataMVs)}.${JSON.stringify(Array.from(queryOptions.entries()).map(o => [o[0], Array.from(o[1])]))}.getMetadataMVKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { if (!metadataMVs) { console.warn('getMetadataMVKeyValues: metadataMVs is undefined'); @@ -2280,6 +2296,10 @@ export class Metadata { } if (rawQueryOptions.length > 0) { promises.push( + // Isolate raw-table failures (timeout, abort, network) the same way + // the three sibling strategies do internally, so a single rejection + // here doesn't discard already-successful text-index / MV results + // when they're aggregated through `Promise.all` below. this.getKeyValues({ chartConfig: { from: { @@ -2296,6 +2316,12 @@ export class Metadata { limit: maxValuesPerKey, source: undefined, signal, + }).catch(error => { + console.warn( + 'getKeyValues (raw table) failed; skipping this strategy for the current batch', + error, + ); + return undefined; }), ); } From e740aeaa75fea9d5c949a149eaaad9a3cf07f2ea Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 15:40:38 -0400 Subject: [PATCH 14/24] enable limits --- .../TeamSettings/TeamQueryConfigSection.tsx | 3 +-- packages/app/src/defaults.ts | 3 +-- packages/app/src/hooks/useMetadata.tsx | 3 ++- packages/common-utils/src/core/metadata.ts | 26 +++++++++++++++---- 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx b/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx index de850d2d6f..e685112003 100644 --- a/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx +++ b/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx @@ -21,7 +21,6 @@ import api from '@/api'; import SelectControlled from '@/components/SelectControlled'; import { DEFAULT_FILTER_KEYS_FETCH_LIMIT, - DEFAULT_FILTER_KEYS_FETCH_LIMIT_WITH_MVS, DEFAULT_QUERY_TIMEOUT, DEFAULT_SEARCH_ROW_LIMIT, } from '@/defaults'; @@ -327,7 +326,7 @@ export default function TeamQueryConfigSection() { min={1} max={1000} displayValue={displayValueWithUnit('keys')} - description={`Default is ${DEFAULT_FILTER_KEYS_FETCH_LIMIT} keys, or ${DEFAULT_FILTER_KEYS_FETCH_LIMIT_WITH_MVS} when metadata materialized views are enabled.`} + description={`Default is ${DEFAULT_FILTER_KEYS_FETCH_LIMIT}`} /> { + const clickhouseVersion = await this.getServerVersion({ connectionId }); + const canQueryMergeTreeTextIndex = + supportsMergeTreeTextIndex(clickhouseVersion); const columnMetadata = await this.getColumns({ databaseName, tableName, @@ -2072,14 +2082,20 @@ export class Metadata { for (const col of columnMetadata) { if (col.name === 'Timestamp') continue; // ignore the timestamp column // first check if this column is a map with a kv index - if (mapTextIndexInfoLookup.get(col.name)?.kv) { + if ( + canQueryMergeTreeTextIndex && + mapTextIndexInfoLookup.get(col.name)?.kv + ) { strategies.mapTextIndexLookup.push( mapTextIndexInfoLookup.get(col.name)!, ); continue; } // second: check if this column is a native column with a kv index - if (nativeTextIndexInfoLookup.has(col.name)) { + if ( + canQueryMergeTreeTextIndex && + nativeTextIndexInfoLookup.has(col.name) + ) { strategies.nativeTextIndexLookup.push( nativeTextIndexInfoLookup.get(col.name)!, ); From a62bda2b79e0e263dff35b20d72f1df848a1ceb9 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:54:14 -0400 Subject: [PATCH 15/24] fix --- packages/common-utils/src/clickhouse/index.ts | 4 +++- packages/common-utils/src/core/metadata.ts | 19 +++++++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index 3123baec9a..22195b4505 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -828,7 +828,9 @@ export function parameterizedQueryToSql({ params: Record; }) { return Object.entries(params).reduce((acc, [key, value]) => { - return acc.replace(new RegExp(`{${key}:\\w+}`, 'g'), value); + return acc.replace(new RegExp(`{${key}:(\\w+)}`, 'g'), (_, type) => { + return type === 'String' ? `'${value}'` : String(value); + }); }, sql); } diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 0f25e57f3e..63bb65c0d4 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1300,14 +1300,13 @@ export class Metadata { } const sql = chSql` SELECT * FROM ( - SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values, sum(count) as total_count + SELECT ColumnIdentifier, Key, groupUniqArray(${{ Int32: maxValuesPerKey }})(Value) as Values FROM ${tableExpr({ database: databaseName, table: mvName })} - WHERE ${concatChSql(' OR ', branch)} + WHERE ${concatChSql(' OR ', branch)} AND ${timeFilter} - AND Value != '' + AND notEmpty(Value) GROUP BY ColumnIdentifier, Key - ORDER BY ColumnIdentifier, Key, total_count DESC - LIMIT ${{ Int32: maxValuesPerKey }} BY ColumnIdentifier, Key + ORDER BY ColumnIdentifier, Key )`; sqlBranches.push(sql); } @@ -2258,12 +2257,16 @@ export class Metadata { continue; } - // fallback to raw table scan + // Re-quote `key.column` for the raw-table SQL: it was stripped of + // backticks by `unquoteIdentifier` above, and column names with hyphens + // or dots (e.g. `Map-Attributes`, `service-name`) need them back or the + // whole batch fails, dropping facets for every key on this path. if (keyValueFetchingStrategies.rawTable.includes(key.column)) { + const quotedColumn = quoteIdentifierIfNeeded(key.column); if (key.mapKey) { - rawQueryOptions.push(`${key.column}['${key.mapKey}']`); + rawQueryOptions.push(`${quotedColumn}['${key.mapKey}']`); } else { - rawQueryOptions.push(`${key.column}`); + rawQueryOptions.push(quotedColumn); } } } From 4183842000ce314b34699d23bae660e00e933d14 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 20:40:19 -0400 Subject: [PATCH 16/24] fix tests --- packages/common-utils/src/__tests__/metadata.test.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index b3e9e81d2f..d40b006d3d 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -819,6 +819,8 @@ describe('Metadata', () => { json: () => Promise.resolve({ data: [{}] }), }); + jest.spyOn(metadata, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); + jest.spyOn(metadata, 'getColumns').mockResolvedValue([ { name: 'Timestamp', type: 'DateTime64(9)' }, { name: 'TraceId', type: 'String' }, @@ -1580,6 +1582,7 @@ describe('Metadata', () => { jest .spyOn(md, 'getMapColumnTextIndexes') .mockResolvedValue(new Map() as any); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); return md; }; @@ -1800,7 +1803,9 @@ describe('Metadata', () => { const realCache = new ( jest.requireActual('../core/metadata') as any ).MetadataCache(); - return new Metadata(mockClickhouseClient, realCache); + const md = new Metadata(mockClickhouseClient, realCache); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); + return md; }; const dateRange: [Date, Date] = [ @@ -2176,6 +2181,7 @@ describe('Metadata', () => { jest .spyOn(md, 'getMapColumnTextIndexes') .mockResolvedValue(new Map() as any); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); // Mock getColumns → returns one Map column (mockClickhouseClient.query as jest.Mock) @@ -2243,6 +2249,7 @@ describe('Metadata', () => { jest .spyOn(md, 'getMapColumnTextIndexes') .mockResolvedValue(new Map() as any); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); (mockClickhouseClient.query as jest.Mock) .mockResolvedValueOnce({ @@ -2637,6 +2644,7 @@ describe('parametric aggregate arguments are inlined as literals', () => { jest .spyOn(md, 'getMapColumnTextIndexes') .mockResolvedValue(new Map() as any); + jest.spyOn(md, 'getServerVersion').mockResolvedValue([26, 3, 0, 0]); return md; }; From 8bfc0ae8a938683dee43fd1661e28418f5e0e64a Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 15 Jul 2026 21:28:32 -0400 Subject: [PATCH 17/24] fix --- packages/api/package.json | 2 + .../src/mcp/tools/sources/describeSource.ts | 7 +- .../src/__tests__/metadata.test.ts | 139 ++++++++++++++++++ packages/common-utils/src/core/metadata.ts | 22 ++- yarn.lock | 2 + 5 files changed, 169 insertions(+), 3 deletions(-) diff --git a/packages/api/package.json b/packages/api/package.json index 312a94c86e..485701072c 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -50,6 +50,7 @@ "promised-handlebars": "^2.0.1", "protobufjs": "^7.6.3", "serialize-error": "^8.1.0", + "sqlstring": "^2.3.3", "uuid": "^8.3.2", "zod": "3.25", "zod-express-middleware": "^1.4.0" @@ -65,6 +66,7 @@ "@types/minimist": "^1.2.2", "@types/ms": "^0.7.31", "@types/passport-local": "^1.0.34", + "@types/sqlstring": "^2.3.0", "@types/supertest": "^2.0.12", "@types/swagger-jsdoc": "^6", "@types/uuid": "^8.3.4", diff --git a/packages/api/src/mcp/tools/sources/describeSource.ts b/packages/api/src/mcp/tools/sources/describeSource.ts index bd2befde8e..6163c6d067 100644 --- a/packages/api/src/mcp/tools/sources/describeSource.ts +++ b/packages/api/src/mcp/tools/sources/describeSource.ts @@ -9,6 +9,7 @@ import { import { ClickhouseClient } from '@hyperdx/common-utils/dist/clickhouse/node'; import { getMetadata } from '@hyperdx/common-utils/dist/core/metadata'; import { type MetricTable, SourceKind } from '@hyperdx/common-utils/dist/types'; +import SqlString from 'sqlstring'; import { z } from 'zod'; import { getConnectionById } from '@/controllers/connection'; @@ -473,7 +474,11 @@ async function describeSourceSchema( const keyExprs: string[] = []; for (const [colName, keys] of Object.entries(mapKeysResults)) { for (const key of keys.slice(0, MAX_MAP_KEYS_TO_SAMPLE)) { - keyExprs.push(`${colName}['${key}']`); + // Map keys come from ClickHouse data (customer telemetry) so they can + // contain arbitrary characters, including single quotes. Escape as a + // SQL string literal — `SqlString.escape` returns a fully-quoted, + // safely-escaped value — before embedding in the key expression. + keyExprs.push(`${colName}[${SqlString.escape(key)}]`); } } diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index d40b006d3d..9d31318479 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1075,6 +1075,31 @@ describe('Metadata', () => { expect(configArg.select).toContain('param0'); }); + it('SQL-escapes map keys with single quotes on the raw-table fallback path (SQL-injection regression)', async () => { + setupDefaultLogsSchema(); + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', + ); + + const injectionPayload = "foo'); SELECT 1 FROM system.tables --"; + + await metadata.getAllKeyValues({ + ...baseArgs, + keyExpressions: [`ResourceAttributes['${injectionPayload}']`], + }); + + expect(renderChartConfigSpy).toHaveBeenCalled(); + const configArg = renderChartConfigSpy.mock.calls[ + renderChartConfigSpy.mock.calls.length - 1 + ][0] as any; + const innerSelect = configArg.with?.[0]?.chartConfig?.select as string; + expect(innerSelect).toBeDefined(); + + expect(innerSelect).toContain("foo\\'"); + expect(innerSelect).not.toMatch(/'foo'\); SELECT 1 FROM system\.tables/); + }); + // Guards against HTTP 431 from too many URL-encoded query_params. Passes // more keys than the private GET_ALL_KEY_VALUES_CHUNK_SIZE (currently 40) // so the recursion has to fire at least two chunks; if someone removes @@ -2596,6 +2621,120 @@ describe('Metadata', () => { expect(result).toBeNull(); }); }); + + describe('doMetadataMVsAggregateColumn (word-boundary token match)', () => { + const tableConn = { + databaseName: 'default', + tableName: 'otel_traces', + connectionId: 'test_connection', + }; + + const mvWithNamesAndBackticks = { + database: 'default', + name: 'traces_kv_rollup_15m_mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW default.traces_kv_rollup_15m_mv TO default.otel_traces AS SELECT ...', + as_select: `WITH elements AS ( + SELECT 'NativeColumn' AS ColumnIdentifier, 'StatusCode' AS Key, CAST(StatusCode, 'String') AS Value FROM default.otel_traces + UNION ALL + SELECT 'NativeColumn' AS ColumnIdentifier, 'SpanId' AS Key, CAST(SpanId, 'String') AS Value FROM default.otel_traces + UNION ALL + SELECT 'NativeColumn' AS ColumnIdentifier, 'ServiceName' AS Key, CAST(ServiceName, 'String') AS Value FROM default.otel_traces + UNION ALL + SELECT 'ResourceAttributes' AS ColumnIdentifier, entry.1 AS Key, CAST(entry.2, 'String') AS Value FROM default.otel_traces ARRAY JOIN \`Map-Attributes\` AS entry + ) SELECT Timestamp, ColumnIdentifier, Key, Value, count() AS count FROM elements GROUP BY Timestamp, ColumnIdentifier, Key, Value`, + }; + + beforeEach(() => { + jest + .spyOn(metadata, 'getAllTableMetadata') + .mockResolvedValue([mvWithNamesAndBackticks] as any); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('matches columns whose exact name appears as a token', async () => { + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'SpanId', + ), + ).toBe(true); + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'ServiceName', + ), + ).toBe(true); + }); + + it('does NOT false-match a column whose name is a substring of another token', async () => { + // Substring false-positive regression: `Status` inside `StatusCode`, + // `Span` inside `SpanId`, `Service` inside `ServiceName`. Prior naive + // `sql.includes(columnName)` would return true for all three, silently + // routing them to the rollup MV (which has no rows for them) instead of + // the raw-table fallback. + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'Status', + ), + ).toBe(false); + expect( + await (metadata as any).doMetadataMVsAggregateColumn(tableConn, 'Span'), + ).toBe(false); + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'Service', + ), + ).toBe(false); + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'Native', + ), + ).toBe(false); + }); + + it('matches a backtick-quoted identifier with hyphens', async () => { + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'Map-Attributes', + ), + ).toBe(true); + }); + + it('does NOT match a bare identifier that sits inside a backtick-quoted longer name', async () => { + // `Map` and `Attributes` are substrings of `Map-Attributes`; the + // hyphen boundary must not turn them into false positives. + expect( + await (metadata as any).doMetadataMVsAggregateColumn(tableConn, 'Map'), + ).toBe(false); + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'Attributes', + ), + ).toBe(false); + }); + + it('returns false for columns absent from the MV', async () => { + expect( + await (metadata as any).doMetadataMVsAggregateColumn(tableConn, 'Body'), + ).toBe(false); + }); + }); }); describe('parseKeyPath', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 63bb65c0d4..8176dff7c2 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1,5 +1,6 @@ import type { ClickHouseSettings } from '@clickhouse/client-common'; import { chunk, omit, pick } from 'lodash'; +import SqlString from 'sqlstring'; import { BaseClickhouseClient, @@ -102,6 +103,17 @@ const quoteIdentifierIfNeeded = (identifier: string): string => { : quoteJsonPathSegment(unquoted); }; +const columnAppearsInMvSelect = (sql: string, columnName: string): boolean => { + const escaped = columnName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const isBareIdentifier = /^[A-Za-z_][A-Za-z0-9_]*$/.test(columnName); + const pattern = isBareIdentifier + ? new RegExp( + `\`${escaped}\`|(? Date: Thu, 16 Jul 2026 09:11:31 -0400 Subject: [PATCH 18/24] fix: use_multipart_params_auto set for clickhouse-js clients --- .../src/__tests__/metadata.test.ts | 19 ++++++++++++------- .../common-utils/src/clickhouse/browser.ts | 1 + packages/common-utils/src/clickhouse/node.ts | 1 + packages/common-utils/src/core/metadata.ts | 16 ++++++---------- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index 9d31318479..cbc8fa76f5 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -1,5 +1,10 @@ import { ClickhouseClient } from '@/clickhouse/node'; -import { Metadata, MetadataCache, parseKeyPath } from '@/core/metadata'; +import { + GET_ALL_KEY_VALUES_CHUNK_SIZE, + Metadata, + MetadataCache, + parseKeyPath, +} from '@/core/metadata'; import * as renderChartConfigModule from '@/core/renderChartConfig'; import { timeFilterExpr } from '@/core/renderChartConfig'; import { isBuilderChartConfig } from '@/guards'; @@ -1101,15 +1106,15 @@ describe('Metadata', () => { }); // Guards against HTTP 431 from too many URL-encoded query_params. Passes - // more keys than the private GET_ALL_KEY_VALUES_CHUNK_SIZE (currently 40) - // so the recursion has to fire at least two chunks; if someone removes - // the chunking this test flags it before the ClickHouse HTTP request - // goes over the wire. Bump the key count if that constant ever exceeds 49. + // more keys than GET_ALL_KEY_VALUES_CHUNK_SIZE so the recursion has to + // fire at least two chunks; if someone removes the chunking this test + // flags it before the ClickHouse HTTP request goes over the wire. it('splits keyExpressions into multiple ClickHouse queries when count exceeds the internal chunk size', async () => { setupDefaultLogsSchema(); + const keyCount = GET_ALL_KEY_VALUES_CHUNK_SIZE + 10; const keyExpressions = Array.from( - { length: 50 }, + { length: keyCount }, (_, i) => `LogAttributes['k${i}']`, ); @@ -1131,7 +1136,7 @@ describe('Metadata', () => { const allParamValues = mapTextIndexCalls.flatMap((c: any[]) => Object.values(c[0].query_params ?? {}), ); - for (let i = 0; i < 50; i++) { + for (let i = 0; i < keyCount; i++) { expect(allParamValues).toContain(`k${i}=`); } }); diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 7e52cec40c..169b994db6 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -101,6 +101,7 @@ export class ClickhouseClient extends BaseClickhouseClient { clickhouse_settings: clickhouseSettings, username: this.username ?? '', password: this.password ?? '', + use_multipart_params_auto: true, // Disable keep-alive to prevent multiple concurrent dashboard requests from exceeding the 64KB payload size limit. keep_alive: { enabled: false, diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index a43e0911b8..37c510525c 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -25,6 +25,7 @@ export class ClickhouseClient extends BaseClickhouseClient { password: this.password, request_timeout: this.requestTimeout, application: this.application, + use_multipart_params_auto: true, }); } diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 8176dff7c2..7707a7ce8c 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -16,10 +16,8 @@ import { import { renderChartConfig, timeFilterExpr } from '@/core/renderChartConfig'; import { buildTextIndexInfoLookup, - KvIndexInfo, skipIndexMatches, TextIndexInfo, - TextIndexInfoLookup, } from '@/queryParser'; import type { BuilderChartConfig, @@ -52,11 +50,8 @@ import { export const DEFAULT_METADATA_MAX_ROWS_TO_READ = 3e6; const DEFAULT_MAX_KEYS = 1000; -// Cap keys per dispatched query: each key becomes one or more URL-encoded -// query_params on the ClickHouse HTTP call, and a few dozen is enough to -// exceed proxy header limits (HTTP 431). Recursive chunks reuse the cached -// strategy lookup, so the only cost is extra parallel HTTP calls. -const GET_ALL_KEY_VALUES_CHUNK_SIZE = 40; +// 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; type KeyFetchingStrategies = { mapTextIndexLookup: TextIndexInfo[]; @@ -1348,12 +1343,13 @@ export class Metadata { .then(res => res.json()) .then(d => d.data.map(row => { + const value = row.Values.filter(v => v); if (row.ColumnIdentifier === 'NativeColumn') { - return { key: row.Key, value: row.Values }; + return { key: row.Key, value }; } return { key: `${row.ColumnIdentifier}['${row.Key}']`, - value: row.Values, + value, }; }), ); @@ -2148,7 +2144,7 @@ export class Metadata { databaseName, tableName, keyExpressions, - maxValuesPerKey = 1000, + maxValuesPerKey = 20, connectionId, metadataMVs, dateRange, From cd71948d9e8bd475aa18d01a4a70095b749c9bbe Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Thu, 16 Jul 2026 15:54:16 -0400 Subject: [PATCH 19/24] fix: hash large objects to reduce cache key size --- packages/common-utils/src/core/metadata.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 7707a7ce8c..f5ad4ad71e 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -1121,7 +1121,8 @@ export class Metadata { timestampValueExpression: string; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getMapTextIndexKeyValues`; + const queryOptionsHash = objectHash(queryOptions); + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${queryOptionsHash}.${timestampValueExpression}.getMapTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { try { const sqlBranches: Array = []; @@ -1206,7 +1207,8 @@ export class Metadata { timestampValueExpression: string; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${JSON.stringify(Array.from(queryOptions.entries()))}.${timestampValueExpression}.getTextIndexKeyValues`; + const queryOptionsHash = objectHash(queryOptions); + const cacheKey = `${databaseName}.${tableName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${queryOptionsHash}.${timestampValueExpression}.getTextIndexKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { try { const sqlBranches: Array = []; @@ -1273,7 +1275,9 @@ export class Metadata { maxValuesPerKey: number; signal?: AbortSignal; }): Promise { - const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${JSON.stringify(metadataMVs)}.${JSON.stringify(Array.from(queryOptions.entries()).map(o => [o[0], Array.from(o[1])]))}.getMetadataMVKeyValues`; + const queryOptionsHash = objectHash(queryOptions); + const metadataMVsHash = objectHash(metadataMVs ?? {}); + const cacheKey = `${databaseName}.${connectionId}.${dateRange[0].toString()}.${dateRange[1].toString()}.${maxValuesPerKey}.${metadataMVsHash}.${queryOptionsHash}.getMetadataMVKeyValues`; return this.cache.getOrFetch(cacheKey, async () => { if (!metadataMVs) { console.warn('getMetadataMVKeyValues: metadataMVs is undefined'); From a33924cd977a02fdce7d879bde9dbf8a489e72cb Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:34:25 -0400 Subject: [PATCH 20/24] fix: keep same name to avoid issue of recreating an MV --- docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql b/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql index d5fc66cbc7..a890fdc54b 100644 --- a/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql +++ b/docker/otel-collector/schema/seed/00006_otel_logs_rollups.sql @@ -18,7 +18,7 @@ TTL Timestamp + ${TABLES_TTL} SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; -- Single MV: CTE with UNION ALL across all columns, then aggregate -CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_logs_kv_rollup_15m_mv TO ${DATABASE}.otel_logs_kv_rollup_15m +CREATE MATERIALIZED VIEW IF NOT EXISTS ${DATABASE}.otel_logs_attr_kv_rollup_15m_mv TO ${DATABASE}.otel_logs_kv_rollup_15m AS WITH elements AS ( SELECT 'NativeColumn' AS ColumnIdentifier, From abd9b08d6e35f9cb7d52f70ccb487d9b771349a9 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Thu, 16 Jul 2026 16:39:44 -0400 Subject: [PATCH 21/24] add changeset --- .changeset/sharp-fishes-yell.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/sharp-fishes-yell.md diff --git a/.changeset/sharp-fishes-yell.md b/.changeset/sharp-fishes-yell.md new file mode 100644 index 0000000000..b4a8eec282 --- /dev/null +++ b/.changeset/sharp-fishes-yell.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": minor +"@hyperdx/app": minor +--- + +feat: upgrade filters and autocomplete to intelligently route queries through the best rollup From 57a438da0dc8c03e19c8062d034883f44ef45d00 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 17 Jul 2026 09:46:18 -0400 Subject: [PATCH 22/24] pr feedback --- .../DBSearchPageFilters/hooks.test.tsx | 16 +- .../components/DBSearchPageFilters/hooks.ts | 12 +- .../Sources/SourceForm/MaterializedViews.tsx | 2 +- .../app/src/hooks/useAutoCompleteOptions.tsx | 2 +- .../src/__tests__/metadata.test.ts | 160 ++++++++++++++++++ packages/common-utils/src/core/metadata.ts | 30 +++- 6 files changed, 199 insertions(+), 23 deletions(-) diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx index 04b7e01746..0788e08e0e 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx +++ b/packages/app/src/components/DBSearchPageFilters/hooks.test.tsx @@ -245,12 +245,12 @@ describe('useFetchFacets', () => { }); }); - // Autocomplete opts into `deferLoadingKeyValues: true` so it can render + // Autocomplete opts into `disableValues: true` so it can render // field-name suggestions from `data.keys` without triggering the values // query — only firing that query once the user is actively searching on // a fully-formed key. Guard against a regression that couples the two. - describe('deferLoadingKeyValues', () => { - it('disables the useGetKeyValues query when deferLoadingKeyValues is true', () => { + describe('disableValues', () => { + it('disables the useGetKeyValues query when disableValues is true', () => { setupDefaultMocks({ withMVs: false }); const { wrapper } = makeWrapper(); @@ -261,7 +261,7 @@ describe('useFetchFacets', () => { sourceId: 'source1', dateRange: DATE_RANGE, mode: 'all', - deferLoadingKeyValues: true, + disableValues: true, }), { wrapper }, ); @@ -270,7 +270,7 @@ describe('useFetchFacets', () => { expect(call?.[1]?.enabled).toBe(false); }); - it('enables the useGetKeyValues query when deferLoadingKeyValues is false or omitted', () => { + it('enables the useGetKeyValues query when disableValues is false or omitted', () => { setupDefaultMocks({ withMVs: false }); const { wrapper } = makeWrapper(); @@ -281,7 +281,7 @@ describe('useFetchFacets', () => { sourceId: 'source1', dateRange: DATE_RANGE, mode: 'all', - deferLoadingKeyValues: false, + disableValues: false, }), { wrapper }, ); @@ -301,7 +301,7 @@ describe('useFetchFacets', () => { sourceId: 'source1', dateRange: DATE_RANGE, mode: 'all', - deferLoadingKeyValues: true, + disableValues: true, }), { wrapper }, ); @@ -321,7 +321,7 @@ describe('useFetchFacets', () => { sourceId: 'source1', dateRange: DATE_RANGE, mode: 'all', - deferLoadingKeyValues: true, + disableValues: true, }), { wrapper }, ); diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index dfa3cfdd86..f06543c6bd 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -36,7 +36,7 @@ function useFacets({ filterState, showMoreFields, enabled, - deferLoadingKeyValues, + disableValues, }: { chartConfig: BuilderChartConfigWithDateRange; sourceId: string | null; @@ -45,7 +45,7 @@ function useFacets({ filterState?: FilterState; showMoreFields?: boolean; enabled?: boolean; - deferLoadingKeyValues?: boolean; + disableValues?: boolean; }) { const { data: source } = useSource({ id: sourceId, @@ -154,7 +154,7 @@ function useFacets({ keys: escapedKeysToFetch, mode, }, - { enabled: enabled && !deferLoadingKeyValues }, + { enabled: enabled && !disableValues }, ); // Map the (escaped) result keys back to the original UI keys. @@ -250,7 +250,7 @@ export function useFetchFacets({ mode, filterState, showMoreFields, - deferLoadingKeyValues, + disableValues, }: { chartConfig: BuilderChartConfigWithDateRange; sourceId: string | null; @@ -258,7 +258,7 @@ export function useFetchFacets({ mode: 'all' | 'exact'; filterState?: FilterState; showMoreFields?: boolean; - deferLoadingKeyValues?: boolean; + disableValues?: boolean; }) { const facetsQuery = useFacets({ chartConfig, @@ -268,7 +268,7 @@ export function useFetchFacets({ filterState, showMoreFields, enabled: true, - deferLoadingKeyValues, + disableValues, }); const [extraFacets, setExtraFacets] = useState(null); diff --git a/packages/app/src/components/Sources/SourceForm/MaterializedViews.tsx b/packages/app/src/components/Sources/SourceForm/MaterializedViews.tsx index f7d3c78bed..07ccdea0f3 100644 --- a/packages/app/src/components/Sources/SourceForm/MaterializedViews.tsx +++ b/packages/app/src/components/Sources/SourceForm/MaterializedViews.tsx @@ -133,7 +133,7 @@ export function MetadataMaterializedViewsFormSection({ - Key Rollup Table + Key Rollup Table (optional, deprecated) { ).toBe(false); }); }); + + describe('doMetadataMVsAggregateColumn (backtick-quoted db/table names)', () => { + const asSelectWithStatusCode = `WITH elements AS ( + SELECT 'NativeColumn' AS ColumnIdentifier, 'StatusCode' AS Key, CAST(StatusCode, 'String') AS Value FROM \`my-db\`.\`otel-traces\` + ) SELECT Timestamp, ColumnIdentifier, Key, Value, count() AS count FROM elements GROUP BY Timestamp, ColumnIdentifier, Key, Value`; + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('matches when database name has a hyphen (backticked in create_table_query)', async () => { + const tableConn = { + databaseName: 'my-db', + tableName: 'otel_traces', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'my-db', + name: 'traces_kv_rollup_15m_mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW `my-db`.traces_kv_rollup_15m_mv TO `my-db`.otel_traces AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + }); + + it('matches when target table name has special chars (backticked in create_table_query)', async () => { + const tableConn = { + databaseName: 'default', + tableName: 'otel-traces', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'default', + name: 'traces_kv_rollup_15m_mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW default.traces_kv_rollup_15m_mv TO default.`otel-traces` AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + }); + + it('matches when MV name has special chars (backticked in create_table_query)', async () => { + const tableConn = { + databaseName: 'default', + tableName: 'otel_traces', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'default', + name: 'traces-kv-rollup-15m-mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW default.`traces-kv-rollup-15m-mv` TO default.otel_traces AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + }); + + it('matches when all identifiers are backticked', async () => { + const tableConn = { + databaseName: 'my-db', + tableName: 'otel-traces', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'my-db', + name: 'traces-kv-rollup-15m-mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW `my-db`.`traces-kv-rollup-15m-mv` TO `my-db`.`otel-traces` AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + }); + + it('does NOT match a different target table even when names are backticked', async () => { + const tableConn = { + databaseName: 'my-db', + tableName: 'otel_traces', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'my-db', + name: 'unrelated_mv', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW `my-db`.unrelated_mv TO `my-db`.some_other_table AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(false); + }); + + it('escapes regex metacharacters in identifiers when they appear backticked', async () => { + const tableConn = { + databaseName: 'default', + tableName: 'weird.name', + connectionId: 'test_connection', + }; + jest.spyOn(metadata, 'getAllTableMetadata').mockResolvedValue([ + { + database: 'default', + name: 'mv1', + engine: 'MaterializedView', + create_table_query: + 'CREATE MATERIALIZED VIEW default.mv1 TO default.`weird.name` AS SELECT ...', + as_select: asSelectWithStatusCode, + }, + ] as any); + + expect( + await (metadata as any).doMetadataMVsAggregateColumn( + tableConn, + 'StatusCode', + ), + ).toBe(true); + }); + }); }); describe('parseKeyPath', () => { diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index f5ad4ad71e..b560ad0359 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -109,6 +109,17 @@ const columnAppearsInMvSelect = (sql: string, columnName: string): boolean => { return pattern.test(sql); }; +// Builds a regex fragment that matches an identifier either as a raw +// unquoted token or wrapped in backticks. +const identifierPattern = (identifier: string): string => { + const escapeRegex = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const rawEscaped = escapeRegex(identifier); + // Backticks inside an identifier are escaped by doubling them in + // ClickHouse's formatted output. + const quotedEscaped = escapeRegex(identifier.replace(/`/g, '``')); + return `(?:\`${quotedEscaped}\`|${rawEscaped})`; +}; + const JSON_STRING_TYPE_SUFFIX = '.:String'; const renderJsonStringSubcolumn = ( @@ -2041,11 +2052,12 @@ export class Metadata { connectionId, }); for (const table of allTableMetadata) { + const expectedPrefix = new RegExp( + `^CREATE MATERIALIZED VIEW ${identifierPattern(databaseName)}\\.${identifierPattern(table.name)} TO ${identifierPattern(databaseName)}\\.${identifierPattern(tableName)}`, + ); if ( table.engine !== 'MaterializedView' || - !table.create_table_query.startsWith( - `CREATE MATERIALIZED VIEW ${databaseName}.${table.name} TO ${databaseName}.${tableName}`, - ) + !expectedPrefix.test(table.create_table_query) ) { continue; } @@ -2168,7 +2180,7 @@ export class Metadata { if (keyExpressions.length === 0) return []; if (keyExpressions.length > GET_ALL_KEY_VALUES_CHUNK_SIZE) { - const batched = await Promise.all( + const batched = await Promise.allSettled( chunk(keyExpressions, GET_ALL_KEY_VALUES_CHUNK_SIZE).map(batch => this.getAllKeyValues({ databaseName, @@ -2183,7 +2195,9 @@ export class Metadata { }), ), ); - return batched.flat(); + return batched + .filter(v => v.status === 'fulfilled') + .flatMap(v => v.value); } // Parse all keys into (rollupColumn, rollupKey) pairs @@ -2362,9 +2376,11 @@ export class Metadata { }), ); } - return (await Promise.all(promises)) + return (await Promise.allSettled(promises)) + .filter(res => res.status === 'fulfilled') + .map(v => v.value) .filter(v => v !== undefined) - .flatMap(v => v); + .flat(); } async getKeyValues({ From d40bf98089f6cba7ae6ecb9c8b310d9f45428aef Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:35:02 -0400 Subject: [PATCH 23/24] add integration tests for getAllKeyValues --- .../src/__tests__/metadata.int.test.ts | 195 ++++++++++++++++++ 1 file changed, 195 insertions(+) diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 668de7f950..1d26cc77e2 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -2,6 +2,7 @@ import { createClient } from '@clickhouse/client'; import { ClickHouseClient } from '@clickhouse/client'; import { ClickhouseClient as HdxClickhouseClient } from '@/clickhouse/node'; +import { supportsMergeTreeTextIndex } from '@/core/clickhouseVersion'; import { Metadata, MetadataCache } from '@/core/metadata'; import { parseKvItemsCastExpression, @@ -776,4 +777,198 @@ describe('Metadata Integration Tests', () => { expect(parsed!.separator).toBe('='); }); }); + + describe('getAllKeyValues (strategy routing)', () => { + // Exercises the four strategies `getAllKeyValues` routes to, against the + // production `default.otel_logs` schema (auto-created by the OTel + // Collector's migration on stack startup — see + // docker/otel-collector/schema/seed/00002_otel_logs.sql and + // 00006_otel_logs_rollups.sql). No custom tables or MVs are created here. + const connectionId = 'test_connection'; + const tag = `getAllKeyValues-routing-${Date.now()}`; + + // Anchor timestamps to now so they stay inside the otel_logs 1-day TTL. + // The 15-minute-bucketed MV rows for these inserts land in the bucket + // containing `testTime`, so the query dateRange below must cover it. + const testTime = new Date(); + const dateRange: [Date, Date] = [ + new Date(testTime.getTime() - 60 * 60 * 1000), + new Date(testTime.getTime() + 60 * 1000), + ]; + + const commonArgs = { + databaseName: 'default', + tableName: 'otel_logs', + connectionId, + dateRange, + timestampValueExpression: 'Timestamp', + metadataMVs: { + kvRollupTable: 'otel_logs_kv_rollup_15m', + granularity: '15 minute' as const, + }, + }; + + // Values unique to this suite so `expect.arrayContaining` still passes + // when the shared table already holds other rows (dev stacks with live + // telemetry). The MV's default maxValuesPerKey=20 could otherwise drop + // our values if the target key already has >20 distinct entries. + const podNameA = `pod-a-${tag}`; + const podNameB = `pod-b-${tag}`; + const traceIdA = `trace-a-${tag}`; + const traceIdB = `trace-b-${tag}`; + const traceIdC = `trace-c-${tag}`; + const bodyA = `Body A ${tag}`; + const bodyB = `Body B ${tag}`; + const bodyC = `Body C ${tag}`; + const schemaUrlA = `https://example.test/${tag}/a`; + const schemaUrlB = `https://example.test/${tag}/b`; + const mapKey = `pod.name.${tag}`; + + let metadata: Metadata; + // `mergeTreeTextIndex(...)` — used by both getMapTextIndexKeyValues and + // getTextIndexKeyValues — was introduced in 26.3. Older servers skip + // those code paths entirely, which would make the routing assertions + // meaningless. Detect once and skip those two cases on older servers. + let textIndexSupported = false; + + beforeAll(async () => { + const probe = new Metadata(hdxClient, new MetadataCache()); + const version = await probe.getServerVersion({ connectionId }); + textIndexSupported = supportsMergeTreeTextIndex(version); + + const timestamp = testTime.toISOString().replace('T', ' ').slice(0, 23); + await client.command({ + query: `INSERT INTO default.otel_logs + (Timestamp, TraceId, ServiceName, SeverityText, Body, ResourceSchemaUrl, ResourceAttributes) VALUES + ('${timestamp}', '${traceIdA}', 'api', 'info', '${bodyA}', '${schemaUrlA}', {'${mapKey}': '${podNameA}'}), + ('${timestamp}', '${traceIdB}', 'api', 'error', '${bodyB}', '${schemaUrlA}', {'${mapKey}': '${podNameB}'}), + ('${timestamp}', '${traceIdC}', 'web', 'warning', '${bodyC}', '${schemaUrlB}', {'${mapKey}': '${podNameA}'}) + `, + }); + }); + + beforeEach(() => { + metadata = new Metadata(hdxClient, new MetadataCache()); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + type StrategySpies = { + mapTextIndex: jest.SpyInstance; + textIndex: jest.SpyInstance; + mv: jest.SpyInstance; + raw: jest.SpyInstance; + }; + + const spyOnAllStrategies = (m: Metadata): StrategySpies => ({ + mapTextIndex: jest.spyOn(m as any, 'getMapTextIndexKeyValues'), + textIndex: jest.spyOn(m as any, 'getTextIndexKeyValues'), + mv: jest.spyOn(m as any, 'getMetadataMVKeyValues'), + raw: jest.spyOn(m, 'getKeyValues'), + }); + + const expectOnlyCalled = ( + spies: StrategySpies, + called: keyof StrategySpies, + ) => { + for (const [name, spy] of Object.entries(spies)) { + if (name === called) { + expect(spy).toHaveBeenCalledTimes(1); + } else { + expect(spy).not.toHaveBeenCalled(); + } + } + }; + + it(`routes ResourceAttributes[] through getMapTextIndexKeyValues`, async () => { + if (!textIndexSupported) { + console.warn( + 'Skipping: ClickHouse < 26.3 does not support mergeTreeTextIndex()', + ); + return; + } + const spies = spyOnAllStrategies(metadata); + + const result = await metadata.getAllKeyValues({ + ...commonArgs, + keyExpressions: [`ResourceAttributes['${mapKey}']`], + }); + + expectOnlyCalled(spies, 'mapTextIndex'); + + const podNames = result.find( + r => r.key === `ResourceAttributes['${mapKey}']`, + ); + expect(podNames).toBeDefined(); + expect(podNames!.value).toEqual( + expect.arrayContaining([podNameA, podNameB]), + ); + }); + + it('routes TraceId through getTextIndexKeyValues', async () => { + if (!textIndexSupported) { + console.warn( + 'Skipping: ClickHouse < 26.3 does not support mergeTreeTextIndex()', + ); + return; + } + const spies = spyOnAllStrategies(metadata); + + const result = await metadata.getAllKeyValues({ + ...commonArgs, + keyExpressions: ['TraceId'], + }); + + expectOnlyCalled(spies, 'textIndex'); + + const traceIds = result.find(r => r.key === 'TraceId'); + expect(traceIds).toBeDefined(); + expect(traceIds!.value).toEqual( + expect.arrayContaining([traceIdA, traceIdB, traceIdC]), + ); + }); + + it('routes ResourceSchemaUrl through getMetadataMVKeyValues', async () => { + // ResourceSchemaUrl has no text index but appears as a NativeColumn + // in the otel_logs_attr_kv_rollup_15m_mv SELECT, so it should be + // routed through the MV. + const spies = spyOnAllStrategies(metadata); + + const result = await metadata.getAllKeyValues({ + ...commonArgs, + keyExpressions: ['ResourceSchemaUrl'], + }); + + expectOnlyCalled(spies, 'mv'); + + const schemas = result.find(r => r.key === 'ResourceSchemaUrl'); + expect(schemas).toBeDefined(); + expect(schemas!.value).toEqual( + expect.arrayContaining([schemaUrlA, schemaUrlB]), + ); + }); + + it('routes Body through getKeyValues (raw table fallback)', async () => { + // The `idx_lower_body` skip index is on the expression `lower(Body)`, + // not the bare `Body` column, so getNativeArrayColumnTextIndexes will + // not match it. Body is also absent from the KV rollup MV SELECT. + // The only remaining route is the raw-table fallback via getKeyValues. + const spies = spyOnAllStrategies(metadata); + + const result = await metadata.getAllKeyValues({ + ...commonArgs, + keyExpressions: ['Body'], + }); + + expectOnlyCalled(spies, 'raw'); + + const bodies = result.find(r => r.key === 'Body'); + expect(bodies).toBeDefined(); + expect(bodies!.value).toEqual( + expect.arrayContaining([bodyA, bodyB, bodyC]), + ); + }); + }); }); From 95e5ec4593ea72cb2c520b78291deae141c4c3be Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Fri, 17 Jul 2026 13:57:28 -0400 Subject: [PATCH 24/24] fix ci --- packages/common-utils/src/core/metadata.ts | 14 ++++++++------ smoke-tests/otel-collector/docker-compose.yaml | 2 ++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index b560ad0359..eddd1675b2 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -101,12 +101,13 @@ const quoteIdentifierIfNeeded = (identifier: string): string => { const columnAppearsInMvSelect = (sql: string, columnName: string): boolean => { const escaped = columnName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); const isBareIdentifier = /^[A-Za-z_][A-Za-z0-9_]*$/.test(columnName); - const pattern = isBareIdentifier - ? new RegExp( - `\`${escaped}\`|(?