diff --git a/.changeset/fix-series-limit-chunk-consistency.md b/.changeset/fix-series-limit-chunk-consistency.md new file mode 100644 index 0000000000..255666fc5a --- /dev/null +++ b/.changeset/fix-series-limit-chunk-consistency.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/common-utils': patch +'@hyperdx/app': patch +--- + +feat(charts): the time-chart series limit is now configured per chart in the Display Settings drawer instead of as a workspace-wide team setting (the team "Time Chart Series Limit" setting is removed). It is disabled by default — charts fetch every series and no `__hdx_series_limit` CTE is emitted — and is cleared back to disabled by emptying the field. The control only appears for builder line/bar charts; the limit and its Generated SQL preview now come from the chart's own config. When a limit is set, chunked time-chart queries keep a consistent top-N series set: previously each time-window chunk ranked its own top-N, so charts could render more series than the limit and adjacent windows disagreed; the ranking is now pinned to the newest chunk window for every chunk so the union across chunks equals the limit. diff --git a/packages/api/src/models/team.ts b/packages/api/src/models/team.ts index 15e92ec72c..4d6b620162 100644 --- a/packages/api/src/models/team.ts +++ b/packages/api/src/models/team.ts @@ -42,7 +42,6 @@ export default mongoose.model( fieldMetadataDisabled: Boolean, parallelizeWhenPossible: Boolean, filterKeysFetchLimit: Number, - seriesLimit: Number, }, { timestamps: true, diff --git a/packages/app/src/ChartUtils.tsx b/packages/app/src/ChartUtils.tsx index b284670182..b339e64650 100644 --- a/packages/app/src/ChartUtils.tsx +++ b/packages/app/src/ChartUtils.tsx @@ -108,9 +108,14 @@ export const MAX_TIME_CHART_SERIES = DEFAULT_SERIES_LIMIT; export function convertToTimeChartConfig( config: ChartConfigWithDateRange, - teamSeriesLimit?: number, ): ChartConfigWithDateRange { - const seriesLimit = Math.max(1, teamSeriesLimit ?? MAX_TIME_CHART_SERIES); + // Series capping is opt-in per tile via the chart's Display Settings; when + // unset, no __hdx_series_limit CTE is emitted and every series is fetched. + const seriesLimit = isBuilderChartConfig(config) + ? config.seriesLimit != null + ? Math.max(1, config.seriesLimit) + : undefined + : undefined; const granularity = getTimeChartGranularity( config.granularity, @@ -139,6 +144,8 @@ export function convertToTimeChartConfig( dateRangeEndInclusive, granularity, limit: { limit: 100000 }, + // Overwrite (not conditionally spread) so a cleared `null` from the + // source config is normalized to undefined rather than carried over. seriesLimit, } : { diff --git a/packages/app/src/__tests__/ChartUtils.test.ts b/packages/app/src/__tests__/ChartUtils.test.ts index ab9acf58e7..400c0ca6d8 100644 --- a/packages/app/src/__tests__/ChartUtils.test.ts +++ b/packages/app/src/__tests__/ChartUtils.test.ts @@ -11,7 +11,6 @@ import { formatResponseForPieChart, formatResponseForTimeChart, } from '@/ChartUtils'; -import { DEFAULT_SERIES_LIMIT } from '@/defaults'; import { COLORS } from '@/utils'; // Anchor info/error to concrete hexes rather than `getChartColorInfo()` / @@ -808,33 +807,34 @@ describe('ChartUtils', () => { expect(granularityFromFunction).toBe('5 minute'); }); - const seriesLimitConfig = { - granularity: '5 minute', - dateRange: [ - new Date('2025-11-26T00:00:00Z'), - new Date('2025-11-27T00:00:00Z'), - ], - } as BuilderChartConfigWithDateRange; - // seriesLimit lives on the builder member of the ChartConfigWithDateRange - // union, so narrow the result before reading it. - const seriesLimitOf = (teamSeriesLimit?: number) => + // union, so narrow the result before reading it. The per-tile value is + // read from the config itself (no team override anymore). + const seriesLimitOf = (seriesLimit?: number | null) => ( - convertToTimeChartConfig( - seriesLimitConfig, - teamSeriesLimit, - ) as BuilderChartConfigWithDateRange + convertToTimeChartConfig({ + granularity: '5 minute', + dateRange: [ + new Date('2025-11-26T00:00:00Z'), + new Date('2025-11-27T00:00:00Z'), + ], + ...(seriesLimit !== undefined ? { seriesLimit } : {}), + } as BuilderChartConfigWithDateRange) as BuilderChartConfigWithDateRange ).seriesLimit; - it('defaults seriesLimit to DEFAULT_SERIES_LIMIT when no team value is given', () => { - expect(seriesLimitOf()).toBe(DEFAULT_SERIES_LIMIT); + it('omits seriesLimit (capping disabled) when the tile has no limit', () => { + expect(seriesLimitOf()).toBeUndefined(); + }); + + it('normalizes a cleared (null) seriesLimit to undefined (disabled)', () => { + expect(seriesLimitOf(null)).toBeUndefined(); }); - it('uses the team seriesLimit when provided', () => { + it('uses the tile seriesLimit when provided', () => { expect(seriesLimitOf(5)).toBe(5); }); - it('passes a large team seriesLimit through unbounded', () => { + it('passes a large tile seriesLimit through unbounded', () => { expect(seriesLimitOf(100000)).toBe(100000); }); }); diff --git a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx index 04660d7be1..a1aa110401 100644 --- a/packages/app/src/components/ChartDisplaySettingsDrawer.tsx +++ b/packages/app/src/components/ChartDisplaySettingsDrawer.tsx @@ -13,11 +13,13 @@ import { Divider, Drawer, Group, + NumberInput, Stack, Text, } from '@mantine/core'; import { shouldFillNullsWithZero } from '@/ChartUtils'; +import { DEFAULT_SERIES_LIMIT } from '@/defaults'; import { FormatTime } from '@/useFormatTime'; import { @@ -41,6 +43,11 @@ export type ChartConfigDisplaySettings = Pick< | 'colorRules' > & { groupByColumnsOnLeft?: boolean; + // Per-tile cap on the number of series fetched for a group-by time chart. + // null/undefined = disabled (no __hdx_series_limit CTE; every series is + // fetched). The editor clears to `null` (not `undefined`) so the cleared + // state survives JSON round-tripping through the URL query state. + seriesLimit?: number | null; }; /** @@ -81,6 +88,9 @@ function applyDefaultSettings( compareToPreviousPeriod: settings.compareToPreviousPeriod ?? false, fitYAxisToData: settings.fitYAxisToData ?? false, groupByColumnsOnLeft: settings.groupByColumnsOnLeft ?? false, + // Coerce to null so `reset` clears the input; undefined leaves the + // previously registered field value in place. + seriesLimit: settings.seriesLimit ?? null, color: settings.color, colorRules: settings.colorRules ? attachLocalIds(settings.colorRules) @@ -144,6 +154,10 @@ export default function ChartDisplaySettingsDrawer({ const isTimeChart = displayType === DisplayType.Line || displayType === DisplayType.StackedBar; + // The series-limit CTE is only emitted for builder group-by time charts; + // raw SQL configs author their own LIMIT logic directly. + const showSeriesLimit = isTimeChart && configType !== 'sql'; + // Group By column ordering only applies to builder table charts; raw SQL // configs let the user author whatever column order they want directly. const showGroupByColumnsOnLeft = @@ -203,6 +217,28 @@ export default function ChartDisplaySettingsDrawer({ label="Fit Y-Axis to Data" description="Start the y-axis at the minimum of the displayed data instead of zero. Only applicable to line charts." /> + {showSeriesLimit && ( + + ( + + onChange(v === '' || v == null ? null : Number(v)) + } + /> + )} + /> + + )} )} diff --git a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx index 5a763b4057..b40e0da570 100644 --- a/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx +++ b/packages/app/src/components/DBEditTimeChartForm/EditTimeChartForm.tsx @@ -243,6 +243,7 @@ export default function EditTimeChartForm({ fitYAxisToData, numberFormat, groupByColumnsOnLeft, + seriesLimit, color, colorRules, ] = useWatch({ @@ -254,6 +255,7 @@ export default function EditTimeChartForm({ 'fitYAxisToData', 'numberFormat', 'groupByColumnsOnLeft', + 'seriesLimit', 'color', 'colorRules', ], @@ -276,6 +278,7 @@ export default function EditTimeChartForm({ fitYAxisToData, numberFormat, groupByColumnsOnLeft, + seriesLimit, color, colorRules, }), @@ -286,6 +289,7 @@ export default function EditTimeChartForm({ fitYAxisToData, numberFormat, groupByColumnsOnLeft, + seriesLimit, color, colorRules, ], @@ -568,6 +572,7 @@ export default function EditTimeChartForm({ compareToPreviousPeriod, fitYAxisToData, groupByColumnsOnLeft, + seriesLimit, color, colorRules, }: ChartConfigDisplaySettings) => { @@ -577,6 +582,10 @@ export default function EditTimeChartForm({ setValue('compareToPreviousPeriod', compareToPreviousPeriod); setValue('fitYAxisToData', fitYAxisToData); setValue('groupByColumnsOnLeft', groupByColumnsOnLeft); + // Persist `null` (not undefined) when cleared so the disabled state + // survives JSON round-tripping through the URL query state — otherwise + // the dropped key lets RHF's `values` sync restore the stale value. + setValue('seriesLimit', seriesLimit ?? null); setValue('color', color); setValue('colorRules', colorRules); onSubmit(); diff --git a/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts b/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts index 67f96b4aca..ef8f02a269 100644 --- a/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts +++ b/packages/app/src/components/DBEditTimeChartForm/__tests__/utils.test.ts @@ -411,6 +411,39 @@ describe('buildChartConfigForExplanations', () => { expect(result).toBeDefined(); }); + it("applies the tile's series limit so the SQL preview matches the chart query", () => { + const result = buildChartConfigForExplanations({ + ...baseParams, + queriedConfig: builderConfig, + queriedSourceId: logSource.id, + tableSource: logSource, + activeTab: 'time', + dbTimeChartConfig: { + ...builderConfig, + seriesLimit: 3, + } as ChartConfigWithDateRange, + }); + + expect(result).toBeDefined(); + // @ts-expect-error union types.. + expect(result!.seriesLimit).toBe(3); + }); + + it('omits seriesLimit (capping disabled) when the tile has no limit', () => { + const result = buildChartConfigForExplanations({ + ...baseParams, + queriedConfig: builderConfig, + queriedSourceId: logSource.id, + tableSource: logSource, + activeTab: 'time', + dbTimeChartConfig: builderConfig, + }); + + expect(result).toBeDefined(); + // @ts-expect-error union types.. + expect(result!.seriesLimit).toBeUndefined(); + }); + it.each(['table', 'number', 'pie'] as const)( 'uses queriedConfig for activeTab=%s and applies tab transform', activeTab => { diff --git a/packages/app/src/components/DBTimeChart.tsx b/packages/app/src/components/DBTimeChart.tsx index 9ae6f1a54c..6783eb8b6e 100644 --- a/packages/app/src/components/DBTimeChart.tsx +++ b/packages/app/src/components/DBTimeChart.tsx @@ -290,8 +290,8 @@ function DBTimeChartComponent({ const { data: me, isLoading: isLoadingMe } = api.useMe(); const queriedConfig = useMemo( - () => convertToTimeChartConfig(config, me?.team?.seriesLimit), - [config, me?.team?.seriesLimit], + () => convertToTimeChartConfig(config), + [config], ); // Determine whether the config can be optimized with an MV, to determine whether diff --git a/packages/app/src/components/SearchTotalCountChart.tsx b/packages/app/src/components/SearchTotalCountChart.tsx index a6be03f89d..ec42f131fa 100644 --- a/packages/app/src/components/SearchTotalCountChart.tsx +++ b/packages/app/src/components/SearchTotalCountChart.tsx @@ -39,8 +39,8 @@ export function useSearchTotalCount( // queriedConfig, queryKey, and enableQueryChunking match DBTimeChart so that react query can de-dupe these queries. const queriedConfig = useMemo( - () => convertToTimeChartConfig(config, me?.team?.seriesLimit), - [config, me?.team?.seriesLimit], + () => convertToTimeChartConfig(config), + [config], ); const { data: totalCountData, diff --git a/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx b/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx index 2cfc66c9b4..de850d2d6f 100644 --- a/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx +++ b/packages/app/src/components/TeamSettings/TeamQueryConfigSection.tsx @@ -24,7 +24,6 @@ import { DEFAULT_FILTER_KEYS_FETCH_LIMIT_WITH_MVS, DEFAULT_QUERY_TIMEOUT, DEFAULT_SEARCH_ROW_LIMIT, - DEFAULT_SERIES_LIMIT, } from '@/defaults'; import { useBrandDisplayName } from '@/theme/ThemeProvider'; @@ -344,16 +343,6 @@ export default function TeamQueryConfigSection() { type="boolean" displayValue={value => (value ? 'Enabled' : 'Disabled')} /> - diff --git a/packages/app/src/components/__tests__/ChartDisplaySettingsDrawer.test.tsx b/packages/app/src/components/__tests__/ChartDisplaySettingsDrawer.test.tsx index ec6b681bcd..fa388de29f 100644 --- a/packages/app/src/components/__tests__/ChartDisplaySettingsDrawer.test.tsx +++ b/packages/app/src/components/__tests__/ChartDisplaySettingsDrawer.test.tsx @@ -175,4 +175,92 @@ describe('ChartDisplaySettingsDrawer', () => { }); }); }); + + describe('series limit setting', () => { + const builderProps = { ...baseProps, configType: 'builder' as const }; + + it('shows the Series Limit input for builder line charts', () => { + renderWithMantine( + , + ); + + expect( + screen.getByRole('textbox', { name: /series limit/i }), + ).toBeInTheDocument(); + }); + + it('does not show the Series Limit input for raw SQL line charts', () => { + renderWithMantine( + , + ); + + expect( + screen.queryByRole('textbox', { name: /series limit/i }), + ).not.toBeInTheDocument(); + }); + + it('does not show the Series Limit input for table charts', () => { + renderWithMantine( + , + ); + + expect( + screen.queryByRole('textbox', { name: /series limit/i }), + ).not.toBeInTheDocument(); + }); + + it('calls onChange with the entered seriesLimit when applied', async () => { + const onChange = jest.fn(); + const user = userEvent.setup(); + + renderWithMantine( + , + ); + + await user.type( + screen.getByRole('textbox', { name: /series limit/i }), + '25', + ); + await user.click(screen.getByRole('button', { name: /apply/i })); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0]).toMatchObject({ seriesLimit: 25 }); + }); + + // Emits null (not undefined) so the cleared/disabled state survives JSON + // round-tripping through the URL query state; undefined would be dropped, + // letting RHF's `values` sync restore the stale value. + it('clears seriesLimit to null (disabled) when emptied', async () => { + const onChange = jest.fn(); + const user = userEvent.setup(); + + renderWithMantine( + , + ); + + await user.clear(screen.getByRole('textbox', { name: /series limit/i })); + await user.click(screen.getByRole('button', { name: /apply/i })); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0].seriesLimit).toBeNull(); + }); + }); }); diff --git a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx index 5371cf9c1a..40981a1e7b 100644 --- a/packages/app/src/hooks/__tests__/useChartConfig.test.tsx +++ b/packages/app/src/hooks/__tests__/useChartConfig.test.tsx @@ -802,6 +802,68 @@ describe('useChartConfig', () => { expect(result.current.isPending).toBe(false); }); + it('pins the series-limit ranking to the newest window on each chunk', async () => { + const config = createMockChartConfig({ + dateRange: [ + new Date('2025-10-01 00:00:00Z'), + new Date('2025-10-02 00:00:00Z'), + ], + granularity: '3 hour', + seriesLimit: 3, + }); + + mockClickhouseClient.queryChartConfig.mockResolvedValue( + createMockQueryResponse([]), + ); + + const { result } = renderHook( + () => useQueriedChartConfig(config, { enableQueryChunking: true }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + // Each chunk queries its own window but must rank top-N series over + // the same fixed range, or the union across chunks exceeds the limit. + // The newest window (first 6h mock window) bounds the ranking scan. + const newestWindow: [Date, Date] = [ + new Date('2025-10-01T18:00:00.000Z'), + new Date('2025-10-02T00:00:00.000Z'), + ]; + const calls = mockClickhouseClient.queryChartConfig.mock.calls; + expect(calls).toHaveLength(3); + for (const [{ config: windowed }] of calls) { + expect(windowed.seriesLimitDateRange).toEqual(newestWindow); + } + }); + + it('does not set seriesLimitDateRange when the query is not chunked', async () => { + const config = createMockChartConfig({ + dateRange: [ + new Date('2025-10-01 00:00:00Z'), + new Date('2025-10-02 00:00:00Z'), + ], + granularity: '3 hour', + seriesLimit: 3, + }); + + mockClickhouseClient.queryChartConfig.mockResolvedValue( + createMockQueryResponse([]), + ); + + const { result } = renderHook(() => useQueriedChartConfig(config), { + wrapper, + }); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + const calls = mockClickhouseClient.queryChartConfig.mock.calls; + expect(calls).toHaveLength(1); + expect('seriesLimitDateRange' in calls[0][0].config).toBe(false); + }); + it('remains in a fetching state, with partial data until all data is loaded', async () => { const config = createMockChartConfig({ dateRange: [ @@ -1327,6 +1389,37 @@ describe('useChartConfig', () => { }); }); + it('pins the series-limit ranking to the newest window with parallel queries', async () => { + const { config } = setupParallelQueries(); + const configWithLimit = { ...config, seriesLimit: 3 }; + + mockClickhouseClient.queryChartConfig.mockResolvedValue( + createMockQueryResponse([]), + ); + + const { result } = renderHook( + () => + useQueriedChartConfig(configWithLimit, { + enableQueryChunking: true, + enableParallelQueries: true, + }), + { wrapper }, + ); + + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + await waitFor(() => expect(result.current.isFetching).toBe(false)); + + const newestWindow: [Date, Date] = [ + new Date('2025-10-01T18:00:00.000Z'), + new Date('2025-10-02T00:00:00.000Z'), + ]; + const calls = mockClickhouseClient.queryChartConfig.mock.calls; + expect(calls).toHaveLength(3); + for (const [{ config: windowed }] of calls) { + expect(windowed.seriesLimitDateRange).toEqual(newestWindow); + } + }); + it('should not execute query while useMVOptimizationExplanation is in loading state', async () => { const config = createMockChartConfig({ dateRange: [ diff --git a/packages/app/src/hooks/useChartConfig.tsx b/packages/app/src/hooks/useChartConfig.tsx index 26c1b1fd36..e2cb3c3c14 100644 --- a/packages/app/src/hooks/useChartConfig.tsx +++ b/packages/app/src/hooks/useChartConfig.tsx @@ -153,6 +153,24 @@ async function* fetchDataInChunks({ ? getGranularityAlignedTimeWindows(config) : [undefined]; + // Every chunk must rank the __hdx_series_limit CTE over the same fixed + // range, or each window keeps its own top-N and the union across chunks + // exceeds seriesLimit. The newest window is used (rather than the full + // chart range) to bound the ranking scan; the trade-off is that series + // are picked by recent activity, so groups with no events in the newest + // window are dropped from the chart. + const rankingDateRange = windows[0]?.dateRange; + const seriesLimit = isBuilderChartConfig(config) + ? config.seriesLimit + : undefined; + const windowedConfigFor = (w: (typeof windows)[number]) => ({ + ...config, + ...(w ?? {}), + ...(w != null && seriesLimit != null && rankingDateRange != null + ? { seriesLimitDateRange: rankingDateRange } + : {}), + }); + if (IS_MTVIEWS_ENABLED && isBuilderChartConfig(config)) { const { dataTableDDL, mtViewDDL, renderMTViewConfig } = await buildMTViewSelectQuery(config, metadata, querySettings); @@ -172,10 +190,7 @@ async function* fetchDataInChunks({ if (enableParallelQueries) { // fetch in parallel const promises = windows.map(async (w, index) => { - const windowedConfig = { - ...config, - ...(w ?? {}), - }; + const windowedConfig = windowedConfigFor(w); return { index, queryResult: await clickhouseClient.queryChartConfig({ @@ -214,12 +229,7 @@ async function* fetchDataInChunks({ // fetch in series for (let i = 0; i < windows.length; i++) { - const window = windows[i]; - - const windowedConfig = { - ...config, - ...(window ?? {}), - }; + const windowedConfig = windowedConfigFor(windows[i]); const result = await clickhouseClient.queryChartConfig({ config: windowedConfig, diff --git a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts index fc50abd854..50015052e3 100644 --- a/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts +++ b/packages/common-utils/src/__tests__/queryChartConfig.int.test.ts @@ -461,4 +461,98 @@ describe('queryChartConfig Integration Tests', () => { }); } }); + + // Chunked fetches narrow dateRange per window; seriesLimitDateRange pins the + // top-N ranking to one shared range (the newest window) so every chunk keeps + // the SAME group set — otherwise the union of per-window top-N sets exceeds + // the limit. + it('keeps a consistent top-N group set across chunked windows via seriesLimitDateRange', async () => { + const TABLE = 'logs_chunked_series_limit_int_test'; + await client.command({ + query: `CREATE OR REPLACE TABLE ${DATABASE}.${TABLE} ( + Timestamp DateTime CODEC(ZSTD(1)), + ServiceName String CODEC(ZSTD(1)) + ) ENGINE = MergeTree ORDER BY (ServiceName, Timestamp)`, + }); + + // Older window (00:00-00:30): svcA dominates. Newest window (00:30-01:00): + // svcB dominates — the ranking is pinned to the newest window, so svcB + // must win in BOTH chunks even though svcA's older peak is larger. + const rows = [ + ...Array.from({ length: 100 }, () => ({ + Timestamp: '2025-04-15 00:10:00', + ServiceName: 'svcA', + })), + { Timestamp: '2025-04-15 00:10:00', ServiceName: 'svcB' }, + { Timestamp: '2025-04-15 00:40:00', ServiceName: 'svcA' }, + ...Array.from({ length: 50 }, () => ({ + Timestamp: '2025-04-15 00:40:00', + ServiceName: 'svcB', + })), + ]; + await client.insert({ + table: `${DATABASE}.${TABLE}`, + values: rows, + format: 'JSONEachRow', + }); + + try { + const newestWindow: [Date, Date] = [ + new Date('2025-04-15T00:30:00Z'), + new Date('2025-04-15T01:00:00Z'), + ]; + const windows: Array<{ + dateRange: [Date, Date]; + dateRangeEndInclusive: boolean; + }> = [ + { + dateRange: newestWindow, + dateRangeEndInclusive: true, + }, + { + dateRange: [new Date('2025-04-15T00:00:00Z'), newestWindow[0]], + dateRangeEndInclusive: false, + }, + ]; + + const groupsPerWindow = await Promise.all( + windows.map(async window => { + const config: ChartConfigWithOptDateRange = { + displayType: DisplayType.Line, + connection: 'test-connection', + from: { databaseName: DATABASE, tableName: TABLE }, + select: [{ aggFn: 'count', aggCondition: '', valueExpression: '' }], + groupBy: [{ aggCondition: '', valueExpression: 'ServiceName' }], + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'Timestamp', + granularity: '5 minute', + seriesLimit: 1, + ...window, + seriesLimitDateRange: newestWindow, + }; + const result = await hdxClient.queryChartConfig({ + config, + metadata, + querySettings: undefined, + }); + return new Set( + (result.data as Array<{ ServiceName: string }>).map( + r => r.ServiceName, + ), + ); + }), + ); + + // Both windows keep the newest-window winner only — without the pinned + // range, the older window would keep svcA (its local top-1) and the + // union would be 2. + expect(groupsPerWindow[0]).toEqual(new Set(['svcB'])); + expect(groupsPerWindow[1]).toEqual(new Set(['svcB'])); + } finally { + await client.command({ + query: `DROP TABLE IF EXISTS ${DATABASE}.${TABLE}`, + }); + } + }); }); diff --git a/packages/common-utils/src/__tests__/renderChartConfig.test.ts b/packages/common-utils/src/__tests__/renderChartConfig.test.ts index 96e26dc805..7408c3b297 100644 --- a/packages/common-utils/src/__tests__/renderChartConfig.test.ts +++ b/packages/common-utils/src/__tests__/renderChartConfig.test.ts @@ -511,6 +511,78 @@ describe('renderChartConfig', () => { await renderChartConfig(baseLogsConfig, mockMetadata, querySettings), ); expect(sql).not.toContain('__hdx_series_limit'); + + // seriesLimitDateRange alone (no seriesLimit) must not emit a CTE either. + const sqlWithRangeOnly = parameterizedQueryToSql( + await renderChartConfig( + { + ...baseLogsConfig, + seriesLimitDateRange: baseLogsConfig.dateRange, + }, + mockMetadata, + querySettings, + ), + ); + expect(sqlWithRangeOnly).not.toContain('__hdx_series_limit'); + }); + + it('pins the series-limit CTE to seriesLimitDateRange while the outer query stays windowed', async () => { + // The chunking caller pins all chunks to one shared ranking range + // (the newest window); the render layer is agnostic to which range. + const rankingRange: [Date, Date] = [ + new Date('2025-02-12T00:00:00Z'), + new Date('2025-02-13T00:00:00Z'), + ]; + const renderWindow = async ( + dateRange: [Date, Date], + dateRangeEndInclusive: boolean, + ) => + parameterizedQueryToSql( + await renderChartConfig( + { + ...baseLogsConfig, + seriesLimit: 60, + dateRange, + dateRangeEndInclusive, + seriesLimitDateRange: rankingRange, + }, + mockMetadata, + querySettings, + ), + ); + + // Two chunked windows of the same chart (most recent window first, + // older windows are end-exclusive — mirrors fetchDataInChunks). + const recentChunk = await renderWindow( + [new Date('2025-02-12T18:00:00Z'), rankingRange[1]], + true, + ); + const olderChunk = await renderWindow( + [rankingRange[0], new Date('2025-02-12T18:00:00Z')], + false, + ); + + // The CTE end is the first `) SELECT` — the outer query starts there. + const cteOf = (sql: string) => { + const start = sql.indexOf('`__hdx_series_limit` AS ('); + const end = sql.indexOf(') SELECT '); + expect(start).toBeGreaterThanOrEqual(0); + expect(end).toBeGreaterThan(start); + return sql.slice(start, end); + }; + + // Both chunks rank over the identical pinned range, so they keep the + // same top-N set; the windowed range only applies to the outer query. + expect(cteOf(recentChunk)).toBe(cteOf(olderChunk)); + expect(cteOf(recentChunk)).toContain(String(rankingRange[0].getTime())); + expect(cteOf(recentChunk)).toContain(String(rankingRange[1].getTime())); + const outerOf = (sql: string) => sql.slice(sql.indexOf(') SELECT ')); + expect(outerOf(olderChunk)).toContain( + String(new Date('2025-02-12T18:00:00Z').getTime()), + ); + expect(outerOf(olderChunk)).not.toContain( + String(rankingRange[1].getTime()), + ); }); it('does not emit a series-limit CTE without a group-by', async () => { diff --git a/packages/common-utils/src/core/renderChartConfig.ts b/packages/common-utils/src/core/renderChartConfig.ts index fe432a0985..2632c997f3 100644 --- a/packages/common-utils/src/core/renderChartConfig.ts +++ b/packages/common-utils/src/core/renderChartConfig.ts @@ -1237,6 +1237,28 @@ async function renderSeriesLimitCte( return undefined; } + // When the query was chunked into time windows, rank over the shared + // range the caller pinned (the newest window) instead of each chunk's own + // window — otherwise each chunk keeps its own top-N and the union across + // chunks exceeds N. Inclusivity is normalized so all chunks emit an + // identical CTE (non-first windows set dateRangeEndInclusive=false). + const cteConfig = chartConfig.seriesLimitDateRange + ? { + ...chartConfig, + dateRange: chartConfig.seriesLimitDateRange, + dateRangeStartInclusive: true, + dateRangeEndInclusive: true, + } + : undefined; + // groupBy is re-rendered (not reused) because timeBucketExpr derives the + // bucket size from dateRange when granularity is 'auto'. + const [cteWhere = where, cteGroupBy = groupBy] = cteConfig + ? await Promise.all([ + renderWhere(cteConfig, metadata), + renderGroupBy(cteConfig, metadata), + ]) + : []; + // One ChSql per group-by column (groupBy may be an array or a comma-separated // string). splitAndTrimWithBracket respects []/()/quotes so it won't split // inside Map['a,b']; the per-column null filter below needs them separated. @@ -1277,8 +1299,8 @@ async function renderSeriesLimitCte( ' AND ', groupByCols.map(g => chSql`${g} IS NOT NULL`), ); - const innerWhere = where.sql - ? concatChSql(' AND ', where, groupByNotNullFilter) + const innerWhere = cteWhere.sql + ? concatChSql(' AND ', cteWhere, groupByNotNullFilter) : groupByNotNullFilter; // Per-(group, bucket) aggregate, then max per group, keeping the top N. @@ -1288,7 +1310,7 @@ async function renderSeriesLimitCte( SELECT tuple(${groupByTuple}) AS \`group\`, ${rankValue} AS \`__hdx_series_rank\` FROM ${from} WHERE ${innerWhere} - GROUP BY ${groupBy} + GROUP BY ${cteGroupBy} ) GROUP BY \`group\` ORDER BY max(\`__hdx_series_rank\`) DESC, \`group\` diff --git a/packages/common-utils/src/types.ts b/packages/common-utils/src/types.ts index 28d38ec500..8094912780 100644 --- a/packages/common-utils/src/types.ts +++ b/packages/common-utils/src/types.ts @@ -275,7 +275,10 @@ export const SelectSQLStatementSchema = z.object({ havingLanguage: SearchConditionLanguageSchema.optional(), orderBy: SortSpecificationListSchema.optional(), limit: LimitSchema.optional(), - seriesLimit: z.number().int().positive().optional(), + // Nullish (not just optional): the chart editor clears the value to `null` + // so the cleared state survives JSON round-tripping (e.g. through the URL + // query state). `null` and `undefined` both mean "disabled" downstream. + seriesLimit: z.number().int().positive().nullish(), }); export type SQLInterval = z.infer; @@ -1263,6 +1266,11 @@ export type DateRange = { dateRange: [Date, Date]; dateRangeStartInclusive?: boolean; // default true dateRangeEndInclusive?: boolean; // default true + // Runtime-only, set by query chunking when dateRange is narrowed to a + // window: a fixed ranking range (the newest chunk window) used by the + // `__hdx_series_limit` CTE so every chunk ranks (and keeps) the same + // top-N series. Never persisted. + seriesLimitDateRange?: [Date, Date]; }; export type ChartConfigWithDateRange = ChartConfig & DateRange; @@ -1557,7 +1565,6 @@ export const TeamClickHouseSettingsSchema = z.object({ metadataMaxRowsToRead: z.number().optional(), parallelizeWhenPossible: z.boolean().optional(), filterKeysFetchLimit: z.number().optional(), - seriesLimit: z.number().int().positive().optional(), }); /** Accepts null to unset (reset to default) a setting. */ @@ -1568,7 +1575,6 @@ export const TeamClickHouseSettingsUpdateSchema = z.object({ metadataMaxRowsToRead: z.number().nullish(), parallelizeWhenPossible: z.boolean().nullish(), filterKeysFetchLimit: z.number().nullish(), - seriesLimit: z.number().int().positive().nullish(), }); export type TeamClickHouseSettingsUpdate = z.infer< typeof TeamClickHouseSettingsUpdateSchema