diff --git a/.changeset/topk-exact-values.md b/.changeset/topk-exact-values.md new file mode 100644 index 0000000000..e90b2ae1d0 --- /dev/null +++ b/.changeset/topk-exact-values.md @@ -0,0 +1,16 @@ +--- +'@hyperdx/common-utils': minor +'@hyperdx/app': patch +--- + +feat: show the most frequent filter values instead of an arbitrary subset + +Value lookups that read table rows (the filter panel's default mode, dashboard +and Kubernetes filters, and the raw-table fallback for all values) used +`groupUniqArray(limit)`, which keeps whichever values it reads first, over a +sample of the first `max_rows_to_read` rows. They now run `topK(limit)` over +the whole time range, so a capped list holds the most common values in the +range, most common first. Missing JSON paths and Map keys no longer take a +slot. These lookups now read every row in the range and use more CPU per row. + +`getKeyValues` and `getKeyValuesWithMVs` no longer take `disableRowLimit`. diff --git a/packages/app/src/DBServiceMapPage.tsx b/packages/app/src/DBServiceMapPage.tsx index 56d954d62a..6357035a00 100644 --- a/packages/app/src/DBServiceMapPage.tsx +++ b/packages/app/src/DBServiceMapPage.tsx @@ -169,7 +169,6 @@ function DBServiceMapPage() { { chartConfig: serviceNamesChartConfig, keys: [serviceNameKey], - disableRowLimit: true, limit: 10000, }, { enabled: !!source }, diff --git a/packages/app/src/DashboardFilters.tsx b/packages/app/src/DashboardFilters.tsx index 5c15cdf9a5..a4581292b8 100644 --- a/packages/app/src/DashboardFilters.tsx +++ b/packages/app/src/DashboardFilters.tsx @@ -188,7 +188,7 @@ const DashboardFilters = ({ // "Link" mode (opt-in, off by default): each dropdown's values are narrowed by // the others' selections. Off by default because contingent value lookups // can't use the cheap per-key rollups and are more expensive at scale. When - // on, all of a source's facets are computed in a single groupUniqArrayIf scan. + // on, all of a source's facets are computed in a single topKIf scan. const [linked, setLinked] = useState(false); const { diff --git a/packages/app/src/NamespaceDetailsSidePanel.tsx b/packages/app/src/NamespaceDetailsSidePanel.tsx index 5dbb278bd6..e136024028 100644 --- a/packages/app/src/NamespaceDetailsSidePanel.tsx +++ b/packages/app/src/NamespaceDetailsSidePanel.tsx @@ -258,7 +258,6 @@ function NamespaceDetailsSidePanelInner({ }, keys: [logSource.serviceNameExpression ?? ''], limit: 10, - disableRowLimit: false, }, { enabled: diff --git a/packages/app/src/NodeDetailsSidePanel.tsx b/packages/app/src/NodeDetailsSidePanel.tsx index 20ab1ff272..cd2d7c801f 100644 --- a/packages/app/src/NodeDetailsSidePanel.tsx +++ b/packages/app/src/NodeDetailsSidePanel.tsx @@ -271,7 +271,6 @@ function NodeDetailsSidePanelInner({ }, keys: [logSource.serviceNameExpression ?? ''], limit: 10, - disableRowLimit: false, }, { enabled: diff --git a/packages/app/src/PodDetailsSidePanel.tsx b/packages/app/src/PodDetailsSidePanel.tsx index f0ec35707c..b697f35b5b 100644 --- a/packages/app/src/PodDetailsSidePanel.tsx +++ b/packages/app/src/PodDetailsSidePanel.tsx @@ -277,7 +277,6 @@ function PodDetailsSidePanelInner({ }, keys: [logSource.serviceNameExpression ?? ''], limit: 10, - disableRowLimit: false, }, { enabled: diff --git a/packages/app/src/components/DBSearchPageFilters/hooks.ts b/packages/app/src/components/DBSearchPageFilters/hooks.ts index 54267aeaf0..b76242da94 100644 --- a/packages/app/src/components/DBSearchPageFilters/hooks.ts +++ b/packages/app/src/components/DBSearchPageFilters/hooks.ts @@ -232,7 +232,6 @@ function useFacets({ }, keys: [sqlKey], limit: SINGLE_KEY_VALUES_LIMIT, - disableRowLimit: true, source, }); return { diff --git a/packages/app/src/components/KubernetesFilters.tsx b/packages/app/src/components/KubernetesFilters.tsx index f8a33fb538..e2fe20d028 100644 --- a/packages/app/src/components/KubernetesFilters.tsx +++ b/packages/app/src/components/KubernetesFilters.tsx @@ -203,7 +203,7 @@ export const KubernetesFilters: React.FC = ({ // Faceted (linked) lookups: build a per-field constraint from the OTHER // selected fields (exclude-self) so all five value lists are computed in a - // single `groupUniqArrayIf` scan. The free-text search is applied as a shared + // single `topKIf` scan. The free-text search is applied as a shared // WHERE. When unlinked there are no constraints and every dropdown lists all // values (still a single batched query). // @@ -282,7 +282,6 @@ export const KubernetesFilters: React.FC = ({ chartConfig, keys, keyConditions, - disableRowLimit: true, limit: 1000000, }); diff --git a/packages/app/src/hooks/__tests__/useMetadata.test.tsx b/packages/app/src/hooks/__tests__/useMetadata.test.tsx index 58fd549a08..b4f81bbdfc 100644 --- a/packages/app/src/hooks/__tests__/useMetadata.test.tsx +++ b/packages/app/src/hooks/__tests__/useMetadata.test.tsx @@ -206,8 +206,8 @@ describe('useGetKeyValues', () => { ).not.toHaveBeenCalled(); }); - // Test case: Custom limit and disableRowLimit - it('should pass custom limit and disableRowLimit', async () => { + // Test case: Custom limit + it('should pass a custom limit', async () => { // Arrange const mockChartConfig = createMockChartConfig(); const mockKeys = ['ResourceAttributes.service.name']; @@ -230,7 +230,6 @@ describe('useGetKeyValues', () => { chartConfig: mockChartConfig, keys: mockKeys, limit: 50, - disableRowLimit: true, }), { wrapper }, ); diff --git a/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx index ba40f07b2e..3f0c78c398 100644 --- a/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx +++ b/packages/app/src/hooks/__tests__/useQueriedDashboardFilterValues.test.tsx @@ -593,7 +593,6 @@ describe('useQueriedDashboardFilterValues', () => { }, keys: ['environment'], limit: 10000, - disableRowLimit: true, signal: expect.any(AbortSignal), source: { connection: 'clickhouse-conn', @@ -1214,7 +1213,7 @@ describe('useQueriedDashboardFilterValues', () => { await waitFor(() => expect(result.current.isFetching).toBe(false)); expect(mockMetadata.getKeyValues).toHaveBeenCalledTimes(1); - // No conditions → plain groupUniqArray (no keyConditions passed). + // No conditions → plain topK (no keyConditions passed). expect( callForKeys(['environment', 'status'])?.keyConditions, ).toBeUndefined(); diff --git a/packages/app/src/hooks/useAvailableMetricNames.ts b/packages/app/src/hooks/useAvailableMetricNames.ts index dbcb344da3..67294d240f 100644 --- a/packages/app/src/hooks/useAvailableMetricNames.ts +++ b/packages/app/src/hooks/useAvailableMetricNames.ts @@ -67,7 +67,6 @@ export function useAvailableMetricNames({ // The value universe is exactly the candidate list, so this cannot cut // off a name we asked about. limit: metricNames.length, - disableRowLimit: true, }, { enabled: enabled && !!chartConfig }, ); diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index 9a61524c3e..c38614fff8 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -468,9 +468,8 @@ async function settleAllOrThrow(promises: Promise[]): Promise { /** * Values for `keys` across one or more sources. * - * - `'exact'` reads metadata MVs, else the raw table, sampling the first - * `max_rows_to_read` rows unless `disableRowLimit` is set. Supports - * `keyConditions`. + * - `'exact'` reads metadata MVs, else the raw table, over the whole range. + * Supports `keyConditions`. * - `'all'` tries text indexes, then metadata MVs, then the raw table, so it is * cheaper for typeahead. It skips JSON-column keys, and its lookups return no * values instead of failing, so it rarely errors. When no values are showing @@ -487,7 +486,6 @@ export function useMultipleGetKeyValues( keys, keyConditions, limit, - disableRowLimit, mode = 'exact', metadataMVs: metadataMVsOverride, }: { @@ -498,7 +496,6 @@ export function useMultipleGetKeyValues( /** Per-key constraints for faceted ('exact' mode) value lookups. */ keyConditions?: (FilterState | undefined)[]; limit?: number; - disableRowLimit?: boolean; mode?: 'all' | 'exact'; /** * MVs for 'all' mode, used instead of resolving them from the config's @@ -540,7 +537,6 @@ export function useMultipleGetKeyValues( // Serialized: react-query hashes keys with JSON.stringify, which would // flatten every distinct Set selection to `{}`. keyConditions?.map(c => c && serializeFilterState(c)), - disableRowLimit, maxKeys, limit, ], @@ -593,7 +589,6 @@ export function useMultipleGetKeyValues( keys: limitedKeys, keyConditions: limitedKeyConditions, limit, - disableRowLimit, source, signal, }), @@ -680,16 +675,14 @@ export function useGetKeyValues( keys, keyConditions, limit, - disableRowLimit, mode, metadataMVs, }: { chartConfig?: BuilderChartConfigWithDateRange; keys: string[]; - /** Per-key constraints for faceted value lookups (groupUniqArrayIf). */ + /** Per-key constraints for faceted value lookups (topKIf). */ keyConditions?: (FilterState | undefined)[]; limit?: number; - disableRowLimit?: boolean; mode?: 'all' | 'exact'; metadataMVs?: MetadataMaterializedViews; }, @@ -701,7 +694,6 @@ export function useGetKeyValues( keys, keyConditions, limit, - disableRowLimit, mode, metadataMVs, }, @@ -712,8 +704,8 @@ export function useGetKeyValues( /** * List metric names for one metrics table, ordered and matched server-side. * - * Prefer this over `useGetKeyValues({ keys: ['MetricName'] })`, which samples an - * arbitrary subset via `groupUniqArray` and can silently omit metrics on + * Prefer this over `useGetKeyValues({ keys: ['MetricName'] })`, which keeps only + * the most frequent names (`topK`) and can silently omit rare metrics on * high-cardinality sources. */ export function useGetMetricNames( diff --git a/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx b/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx index 9023b91ac7..0b99216250 100644 --- a/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx +++ b/packages/app/src/hooks/useQueriedDashboardFilterValues.tsx @@ -131,7 +131,7 @@ function useOptimizedKeyValuesCalls({ // (so the constrained columns exist in the queried table), EXCLUDING the // filter's own expression (otherwise a multi-select would collapse to only // its already-selected values). Passing this down as a per-key constraint - // lets all of a source's filters resolve in one `groupUniqArrayIf` scan + // lets all of a source's filters resolve in one `topKIf` scan // instead of one query per filter. // // The constraint stays a FilterState rather than SQL: getKeyValues renders @@ -174,7 +174,7 @@ function useOptimizedKeyValuesCalls({ // Group filters by (source, metricType, where, whereLanguage). Every filter in // a group is resolved together: an unconstrained group goes through the MV // optimizer (one batched, rollup-eligible query); a constrained group runs a - // single faceted `groupUniqArrayIf` scan carrying a per-key condition. + // single faceted `topKIf` scan carrying a per-key condition. const filtersByGroupKey = useMemo(() => { const byGroupKey = new Map(); for (const filter of filters) { @@ -262,7 +262,7 @@ function useOptimizedKeyValuesCalls({ staleTime: 1000 * 60 * 5, // Cache every 5 min queryFn: async ({ signal }): Promise => { // Constrained: resolve every key in one faceted scan - // (groupUniqArrayIf). A per-key condition can't be split across + // (topKIf). A per-key condition can't be split across // single-key MVs, but it can run against one MV whose dimensions // cover every filter column (else the raw table). if (isFaceted) { @@ -393,7 +393,6 @@ export function useQueriedDashboardFilterValues({ keys, keyConditions, limit: 10000, - disableRowLimit: true, signal, source, }), diff --git a/packages/common-utils/src/__tests__/metadata.int.test.ts b/packages/common-utils/src/__tests__/metadata.int.test.ts index 55853a7421..5b8636e295 100644 --- a/packages/common-utils/src/__tests__/metadata.int.test.ts +++ b/packages/common-utils/src/__tests__/metadata.int.test.ts @@ -101,123 +101,132 @@ describe('Metadata Integration Tests', () => { }); }); - describe.each([true, false])('with disableRowLimit=%s', disableRowLimit => { - it('should return key-value pairs for a given metadata key', async () => { - const resultSeverityText = await metadata.getKeyValues({ - chartConfig, - keys: ['SeverityText'], - disableRowLimit, - source, - }); + it('should return key-value pairs for a given metadata key', async () => { + const resultSeverityText = await metadata.getKeyValues({ + chartConfig, + keys: ['SeverityText'], + source, + }); - expect(resultSeverityText).toHaveLength(1); - expect(resultSeverityText[0].key).toBe('SeverityText'); - expect(resultSeverityText[0].value).toHaveLength(3); - expect(resultSeverityText[0].value).toEqual( - expect.arrayContaining(['info', 'error', 'warning']), - ); + expect(resultSeverityText).toHaveLength(1); + expect(resultSeverityText[0].key).toBe('SeverityText'); + expect(resultSeverityText[0].value).toHaveLength(3); + expect(resultSeverityText[0].value).toEqual( + expect.arrayContaining(['info', 'error', 'warning']), + ); - const resultTraceId = await metadata.getKeyValues({ - chartConfig, - keys: ['TraceId'], - disableRowLimit, - source, - }); + const resultTraceId = await metadata.getKeyValues({ + chartConfig, + keys: ['TraceId'], + source, + }); + + expect(resultTraceId).toHaveLength(1); + expect(resultTraceId[0].key).toBe('TraceId'); + expect(resultTraceId[0].value).toHaveLength(3); + expect(resultTraceId[0].value).toEqual( + expect.arrayContaining([ + '1o2udn120d8n', + '67890-09098', + '11h9238re1h92', + ]), + ); + + const resultBoth = await metadata.getKeyValues({ + chartConfig, + keys: ['TraceId', 'SeverityText'], + source, + }); - expect(resultTraceId).toHaveLength(1); - expect(resultTraceId[0].key).toBe('TraceId'); - expect(resultTraceId[0].value).toHaveLength(3); - expect(resultTraceId[0].value).toEqual( - expect.arrayContaining([ + expect(resultBoth).toEqual([ + { + key: 'TraceId', + value: expect.arrayContaining([ '1o2udn120d8n', '67890-09098', '11h9238re1h92', ]), - ); - - const resultBoth = await metadata.getKeyValues({ - chartConfig, - keys: ['TraceId', 'SeverityText'], - disableRowLimit, - source, - }); + }, + { + key: 'SeverityText', + value: expect.arrayContaining(['info', 'error', 'warning']), + }, + ]); + }); - expect(resultBoth).toEqual([ - { - key: 'TraceId', - value: expect.arrayContaining([ - '1o2udn120d8n', - '67890-09098', - '11h9238re1h92', - ]), - }, - { - key: 'SeverityText', - value: expect.arrayContaining(['info', 'error', 'warning']), - }, - ]); + it('should handle materialized columns correctly', async () => { + const resultPodName = await metadata.getKeyValues({ + chartConfig, + keys: ['__hdx_materialized_k8s.pod.name'], + source, }); - it('should handle materialized columns correctly', async () => { - const resultPodName = await metadata.getKeyValues({ - chartConfig, - keys: ['__hdx_materialized_k8s.pod.name'], - disableRowLimit, - source, - }); + expect(resultPodName).toHaveLength(1); + expect(resultPodName[0].key).toBe('__hdx_materialized_k8s.pod.name'); + expect(resultPodName[0].value).toHaveLength(2); + expect(resultPodName[0].value).toEqual( + expect.arrayContaining(['pod1', 'pod2']), + ); + }); - expect(resultPodName).toHaveLength(1); - expect(resultPodName[0].key).toBe('__hdx_materialized_k8s.pod.name'); - expect(resultPodName[0].value).toHaveLength(2); - expect(resultPodName[0].value).toEqual( - expect.arrayContaining(['pod1', 'pod2']), - ); + it('should handle JSON columns correctly', async () => { + const resultLogAttributes = await metadata.getKeyValues({ + chartConfig, + keys: ['LogAttributes.user'], + source, }); - it('should handle JSON columns correctly', async () => { - const resultLogAttributes = await metadata.getKeyValues({ - chartConfig, - keys: ['LogAttributes.user'], - disableRowLimit, - source, - }); + expect(resultLogAttributes).toHaveLength(1); + expect(resultLogAttributes[0].key).toBe('LogAttributes.user'); + expect(resultLogAttributes[0].value).toHaveLength(3); + expect(resultLogAttributes[0].value).toEqual( + expect.arrayContaining(['john', 'jack', 'jane']), + ); + }); - expect(resultLogAttributes).toHaveLength(1); - expect(resultLogAttributes[0].key).toBe('LogAttributes.user'); - expect(resultLogAttributes[0].value).toHaveLength(3); - expect(resultLogAttributes[0].value).toEqual( - expect.arrayContaining(['john', 'jack', 'jane']), - ); + it('should return an empty list when no keys are provided', async () => { + const resultEmpty = await metadata.getKeyValues({ + chartConfig, + keys: [], + source, }); - it('should return an empty list when no keys are provided', async () => { - const resultEmpty = await metadata.getKeyValues({ - chartConfig, - keys: [], - source, - }); + expect(resultEmpty).toEqual([]); + }); - expect(resultEmpty).toEqual([]); + it('should correctly limit the number of returned values', async () => { + const resultLimited = await metadata.getKeyValues({ + chartConfig, + keys: ['SeverityText'], + limit: 2, + source, }); - it('should correctly limit the number of returned values', async () => { - const resultLimited = await metadata.getKeyValues({ - chartConfig, - keys: ['SeverityText'], - limit: 2, - source, - }); + expect(resultLimited).toHaveLength(1); + expect(resultLimited[0].key).toBe('SeverityText'); + expect(resultLimited[0].value).toHaveLength(2); + expect( + resultLimited[0].value.every( + v => + typeof v === 'string' && ['info', 'error', 'warning'].includes(v), + ), + ).toBeTruthy(); + }); - expect(resultLimited).toHaveLength(1); - expect(resultLimited[0].key).toBe('SeverityText'); - expect(resultLimited[0].value).toHaveLength(2); - expect( - resultLimited[0].value.every( - v => - typeof v === 'string' && ['info', 'error', 'warning'].includes(v), - ), - ).toBeTruthy(); + it('keeps the most frequent values when the limit truncates the list', async () => { + // The first row read has action 'ping'; NULL (no action) and '' (no + // pod name) are more frequent than any real value. + const result = await metadata.getKeyValues({ + chartConfig, + keys: ['LogAttributes.action', "ResourceAttributes['k8s.pod.name']"], + limit: 1, + source, }); + + expect(result).toEqual([ + { key: 'LogAttributes.action', value: ['login'] }, + { key: "ResourceAttributes['k8s.pod.name']", value: ['pod1'] }, + ]); }); }); @@ -491,52 +500,6 @@ describe('Metadata Integration Tests', () => { expect(result[0].key).toBe('service'); expect(result[0].value.length).toBeLessThanOrEqual(2); }); - - it('should work with disableRowLimit: true', async () => { - const source = { - id: 'test-source', - name: 'Test Logs', - kind: 'otel-logs', - from: { databaseName: 'default', tableName: baseTableName }, - timestampValueExpression: 'Timestamp', - connection: 'test_connection', - materializedViews: [ - { - databaseName: 'default', - tableName: mvTableName, - dimensionColumns: 'environment, service, status_code', - minGranularity: '1 minute', - timestampColumn: 'Timestamp', - aggregatedColumns: [{ aggFn: 'count', mvColumn: 'count' }], - }, - ], - }; - - // Should work with disableRowLimit: true (no row limits applied) - const result = await metadata.getKeyValuesWithMVs({ - chartConfig, - keys: ['environment', 'service', 'status_code'], - source: source as any, - disableRowLimit: true, - }); - - expect(result).toHaveLength(3); - - const environmentResult = result.find(r => r.key === 'environment'); - expect(environmentResult?.value).toEqual( - expect.arrayContaining(['production', 'staging']), - ); - - const serviceResult = result.find(r => r.key === 'service'); - expect(serviceResult?.value).toEqual( - expect.arrayContaining(['api', 'web', 'worker']), - ); - - const statusCodeResult = result.find(r => r.key === 'status_code'); - expect(statusCodeResult?.value).toEqual( - expect.arrayContaining(['200', '404', '500']), - ); - }); }); describe('Distributed tables support', () => { diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index f07e176c4a..393befc5d0 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -812,43 +812,33 @@ describe('Metadata', () => { }); }); - it('should apply row limit when disableRowLimit is false', async () => { - await metadata.getKeyValues({ - chartConfig: mockChartConfig, - keys: ['column1', 'column2'], - limit: 10, - disableRowLimit: false, - source, - }); - - expect(mockClickhouseClient.query).toHaveBeenCalledWith( - expect.objectContaining({ - clickhouse_settings: { - max_rows_to_read: '0', - timeout_overflow_mode: 'break', - max_execution_time: 15, - }, - }), + it('aggregates over the whole range without a sampling CTE', async () => { + const renderChartConfigSpy = jest.spyOn( + renderChartConfigModule, + 'renderChartConfig', ); - }); - it('should not apply row limit when disableRowLimit is true', async () => { await metadata.getKeyValues({ chartConfig: mockChartConfig, keys: ['column1', 'column2'], limit: 10, - disableRowLimit: true, source, }); + const actualConfig = renderChartConfigSpy.mock.calls.at(-1)![0]; + if (!isBuilderChartConfig(actualConfig)) + throw new Error('Expected builder config'); + expect(actualConfig.from).toEqual(mockChartConfig.from); + expect(actualConfig.with).toBeUndefined(); + expect(actualConfig.select).toBe( + 'topK(12)(column1) AS param0, topK(12)(column2) AS param1', + ); expect(mockClickhouseClient.query).toHaveBeenCalledWith( - expect.objectContaining({ - clickhouse_settings: undefined, - }), + expect.not.objectContaining({ clickhouse_settings: expect.anything() }), ); }); - it('uses groupUniqArrayIf for keys that have a condition (faceted lookup)', async () => { + it('uses topKIf for keys that have a condition (faceted lookup)', async () => { const renderChartConfigSpy = jest.spyOn( renderChartConfigModule, 'renderChartConfig', @@ -862,20 +852,17 @@ describe('Metadata', () => { { colA: { included: new Set(['x']), excluded: new Set() } }, ], limit: 10, - disableRowLimit: true, source, }); const actualConfig = renderChartConfigSpy.mock.calls.at(-1)![0]; if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); - // No condition → plain groupUniqArray; condition → groupUniqArrayIf so + // No condition → plain topK; condition → topKIf so // both facets resolve in a single scan. + expect(actualConfig.select).toContain('topK(12)(colA) AS param0'); expect(actualConfig.select).toContain( - 'groupUniqArray(10)(colA) AS param0', - ); - expect(actualConfig.select).toContain( - "groupUniqArrayIf(10)(colB, (colA IN ('x'))) AS param1", + "topKIf(12)(colB, (colA IN ('x'))) AS param1", ); }); @@ -911,7 +898,6 @@ describe('Metadata', () => { }, ], limit: 10, - disableRowLimit: true, source, }); @@ -919,32 +905,13 @@ describe('Metadata', () => { if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); expect(actualConfig.select).toContain( - 'groupUniqArrayIf(10)(Attributes.`namespace`.:String, ' + + 'topKIf(12)(Attributes.`namespace`.:String, ' + "(Attributes.`cluster`.:String IN ('prod'))) AS param1", ); // The raw bracket form must not survive into either half. expect(actualConfig.select).not.toContain("Attributes['"); }); - it('should apply row limit by default when disableRowLimit is not specified', async () => { - await metadata.getKeyValues({ - chartConfig: mockChartConfig, - keys: ['column1', 'column2'], - limit: 10, - source, - }); - - expect(mockClickhouseClient.query).toHaveBeenCalledWith( - expect.objectContaining({ - clickhouse_settings: { - max_rows_to_read: '0', - timeout_overflow_mode: 'break', - max_execution_time: 15, - }, - }), - ); - }); - it('should correctly transform the response data', async () => { const result = await metadata.getKeyValues({ chartConfig: mockChartConfig, @@ -974,15 +941,12 @@ describe('Metadata', () => { chartConfig: mockChartConfig, keys: ['column1'], limit, - disableRowLimit: true, source, }); const actualConfig = renderChartConfigSpy.mock.calls.at(-1)![0]; if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); - expect(actualConfig.select).toContain( - `groupUniqArray(${limit})(column1)`, - ); + expect(actualConfig.select).toContain(`topK(${limit + 2})(column1)`); } }); @@ -1010,6 +974,24 @@ describe('Metadata', () => { ]); }); + it('returns at most limit values once nulls and empty strings are dropped', async () => { + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: () => + Promise.resolve({ + data: [{ param0: [null, 'value1', '', 'value2', 'value3'] }], + }), + }); + + const result = await metadata.getKeyValues({ + chartConfig: mockChartConfig, + keys: ['column1'], + limit: 2, + source, + }); + + expect(result).toEqual([{ key: 'column1', value: ['value1', 'value2'] }]); + }); + it('should return an empty list when no keys are provided', async () => { const renderChartConfigSpy = jest.spyOn( renderChartConfigModule, @@ -1053,12 +1035,9 @@ describe('Metadata', () => { const actualConfig = renderChartConfigSpy.mock.calls[0][0]; if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); - expect(actualConfig.with?.[0]).toMatchObject({ - chartConfig: { - select: - 'ResourceAttributes.`k8s`.`namespace`.`name`.:String as param0', - }, - }); + expect(actualConfig.select).toBe( + 'topK(12)(ResourceAttributes.`k8s`.`namespace`.`name`.:String) AS param0', + ); }); it('quotes typed-looking bracket JSON keys instead of passing them through', async () => { @@ -1087,12 +1066,9 @@ describe('Metadata', () => { const actualConfig = renderChartConfigSpy.mock.calls[0][0]; if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); - expect(actualConfig.with?.[0]).toMatchObject({ - chartConfig: { - select: - 'ResourceAttributes.`foo`.`:String, count() AS injected`.:String as param0', - }, - }); + expect(actualConfig.select).toBe( + 'topK(12)(ResourceAttributes.`foo`.`:String, count() AS injected`.:String) AS param0', + ); }); it('keeps map attribute keys in bracket form', async () => { @@ -1121,11 +1097,9 @@ describe('Metadata', () => { const actualConfig = renderChartConfigSpy.mock.calls[0][0]; if (!isBuilderChartConfig(actualConfig)) throw new Error('Expected builder config'); - expect(actualConfig.with?.[0]).toMatchObject({ - chartConfig: { - select: "LogAttributes['k8s.namespace.name'] as param0", - }, - }); + expect(actualConfig.select).toBe( + "topK(12)(LogAttributes['k8s.namespace.name']) AS param0", + ); }); }); @@ -1530,7 +1504,7 @@ describe('Metadata', () => { const configArg = renderChartConfigSpy.mock.calls[ renderChartConfigSpy.mock.calls.length - 1 ][0] as any; - expect(configArg.select).toContain('groupUniqArray('); + expect(configArg.select).toContain('topK('); expect(configArg.select).toContain('param0'); }); @@ -1626,7 +1600,7 @@ describe('Metadata', () => { const configArg = renderChartConfigSpy.mock.calls[ renderChartConfigSpy.mock.calls.length - 1 ][0] as any; - expect(configArg.select).toContain('groupUniqArray('); + expect(configArg.select).toContain('topK('); }); it('routes ResourceAttributes["k8s.pod.name"] to the raw table when its map has no KV text index', async () => { @@ -1645,7 +1619,7 @@ describe('Metadata', () => { const configArg = renderChartConfigSpy.mock.calls[ renderChartConfigSpy.mock.calls.length - 1 ][0] as any; - expect(configArg.select).toContain('groupUniqArray('); + expect(configArg.select).toContain('topK('); expect(configArg.select).toContain('param0'); }); @@ -1667,11 +1641,11 @@ describe('Metadata', () => { 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(); + const select = configArg.select as string; + expect(select).toBeDefined(); - expect(innerSelect).toContain("foo\\'"); - expect(innerSelect).not.toMatch(/'foo'\); SELECT 1 FROM system\.tables/); + expect(select).toContain("foo\\'"); + expect(select).not.toMatch(/'foo'\); SELECT 1 FROM system\.tables/); }); // Guards against HTTP 431 from too many URL-encoded query_params. Passes @@ -1789,7 +1763,7 @@ describe('Metadata', () => { const configArg = renderChartConfigSpy.mock.calls[ renderChartConfigSpy.mock.calls.length - 1 ][0] as any; - expect(configArg.select).toContain(`groupUniqArray(${MAX_VALUES})`); + expect(configArg.select).toContain(`topK(${MAX_VALUES + 2})`); }); }); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 34425ac362..c064ff26c0 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -3064,7 +3064,6 @@ export class Metadata { keys, keyConditions, limit = 20, - disableRowLimit = false, signal, source, }: { @@ -3073,10 +3072,9 @@ export class Metadata { /** * Optional per-key constraint, aligned with `keys`: the selections that * should narrow this key's values. When provided for a key, its values are - * gathered with `groupUniqArrayIf(key, )` so several "faceted" + * gathered with `topKIf(key, )` so several "faceted" * value lists (each constrained differently) resolve in a single table - * scan. Only applied when `disableRowLimit` is true (the path used by - * filter dropdowns). + * scan. * * Keys inside the state are RAW expressions; they are rendered here the * same way as `keys`, so the predicate addresses the same physical column @@ -3084,7 +3082,6 @@ export class Metadata { */ keyConditions?: (FilterState | undefined)[]; limit?: number; - disableRowLimit?: boolean; signal?: AbortSignal; source: | Omit /* for overlap with ISource type */ @@ -3103,7 +3100,6 @@ export class Metadata { // Serialize rather than hashing the raw state: the selections are Sets. keyConditions: keyConditions?.map(s => s && serializeFilterState(s)), limit, - disableRowLimit, }; return this.cache.getOrFetch( `${objectHash(cacheKeyConfig)}.getKeyValues`, @@ -3120,69 +3116,31 @@ export class Metadata { keys, }); const renderKey = (key: string) => renderedByKey.get(key) ?? key; - const renderedKeys = keys.map(renderKey); - - // When disableRowLimit is true, query directly without CTE - // Otherwise, use CTE with row limits for sampling - const sqlConfig = disableRowLimit - ? { - ...chartConfig, - select: keys - .map((key, i) => { - const k = renderKey(key); - const state = keyConditions?.at(i); - // Render the constraint against the same expressions used in - // the SELECT, or the predicate would address a different - // physical column than the aggregate wrapping it. - const condition = - state && filterStateToPredicate(state, renderKey); - // `groupUniqArrayIf` lets each key be constrained by its own - // predicate, so multiple faceted value lists are computed in a - // single scan instead of one query per key. - return condition - ? `groupUniqArrayIf(${limit})(${k}, ${condition}) AS param${i}` - : `groupUniqArray(${limit})(${k}) AS param${i}`; - }) - .join(', '), - } - : await (async () => { - // Build select expression that includes each of the given keys. - // This avoids selecting entire JSON columns, which is significantly slower - // than selecting just the JSON paths corresponding to the given keys. - // paramN aliases are used to avoid issues with special characters or complex expressions in keys. - const selectExpr = - renderedKeys.map((k, i) => `${k} as param${i}`).join(', ') || - '*'; - return { - with: [ - { - name: 'sampledData', - chartConfig: { - ...chartConfig, - select: selectExpr, - limit: { - limit: this.getClickHouseSettings().max_rows_to_read - ? Number( - this.getClickHouseSettings().max_rows_to_read, - ) - : DEFAULT_METADATA_MAX_ROWS_TO_READ, - }, - }, - isSubquery: true, - }, - ], - select: keys - .map( - (_, i) => - `groupUniqArray(${limit})(param${i}) AS param${i}`, - ) - .join(', '), - connection: chartConfig.connection, - from: { databaseName: '', tableName: 'sampledData' }, - where: '', - }; - })(); + // topK returns the most frequent values, most frequent first. NULL (a + // missing JSON path) and '' (a missing Map key) are often the most + // frequent and are dropped below, so leave room for both. + const topKLimit = limit + 2; + const sqlConfig = { + ...chartConfig, + select: keys + .map((key, i) => { + const k = renderKey(key); + const state = keyConditions?.at(i); + // Render the constraint against the same expressions used in + // the SELECT, or the predicate would address a different + // physical column than the aggregate wrapping it. + const condition = + state && filterStateToPredicate(state, renderKey); + // `topKIf` lets each key be constrained by its own + // predicate, so multiple faceted value lists are computed in a + // single scan instead of one query per key. + return condition + ? `topKIf(${topKLimit})(${k}, ${condition}) AS param${i}` + : `topK(${topKLimit})(${k}) AS param${i}`; + }) + .join(', '), + }; const sql = await renderChartConfig( sqlConfig, @@ -3195,16 +3153,6 @@ export class Metadata { query: sql.sql, query_params: sql.params, connectionId: chartConfig.connection, - clickhouse_settings: !disableRowLimit - ? { - ...this.getClickHouseSettings(), - // Max 15 seconds to get keys - timeout_overflow_mode: 'break', - max_execution_time: 15, - // Set the value to 0 (unlimited) so that the LIMIT is used instead - max_rows_to_read: '0', - } - : undefined, abort_signal: signal, }) .then(res => res.json>()); @@ -3214,9 +3162,9 @@ export class Metadata { return Object.entries(json?.data?.[0]).map(([key, value]) => ({ key: keys[parseInt(key.replace('param', ''))], // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - value: value?.filter(v => v != null && v !== '') as // remove nulls and empty strings - | string[] - | number[], + value: value + ?.filter(v => v != null && v !== '') // remove nulls and empty strings + .slice(0, limit) as string[] | number[], })); }, ); @@ -3350,7 +3298,6 @@ export class Metadata { keyConditions, source, limit = 20, - disableRowLimit, signal, }: { chartConfig: BuilderChartConfigWithDateRange; @@ -3359,7 +3306,6 @@ export class Metadata { keyConditions?: (FilterState | undefined)[]; source: TSource | undefined; limit?: number; - disableRowLimit?: boolean; signal?: AbortSignal; }): Promise { const cacheKeyConfig = { @@ -3375,7 +3321,6 @@ export class Metadata { // Serialize rather than hashing the raw state: the selections are Sets. keyConditions: keyConditions?.map(s => s && serializeFilterState(s)), limit, - disableRowLimit, }; return this.cache.getOrFetch( `${objectHash(cacheKeyConfig)}.getKeyValuesWithMVs`, @@ -3401,7 +3346,6 @@ export class Metadata { keys, keyConditions, limit, - disableRowLimit, signal, source, }); @@ -3428,7 +3372,6 @@ export class Metadata { chartConfig, keys, limit, - disableRowLimit, signal, source, }),