From 338c9af9d3d932ff03fac7769f6724bcc3e086e3 Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:08:17 -0600 Subject: [PATCH 1/8] fix: skip exhausted search pages --- .changeset/skip-exhausted-search-pages.md | 6 ++ .../useOffsetPaginatedQuery.test.tsx | 76 ++++++++++++++++++- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 9 ++- 3 files changed, 87 insertions(+), 4 deletions(-) create mode 100644 .changeset/skip-exhausted-search-pages.md diff --git a/.changeset/skip-exhausted-search-pages.md b/.changeset/skip-exhausted-search-pages.md new file mode 100644 index 0000000000..8eacdbeff9 --- /dev/null +++ b/.changeset/skip-exhausted-search-pages.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +--- + +Skip redundant offset queries after a search returns fewer rows than the +requested page size. diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 2f2e4ace05..9130c6ac2c 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -388,12 +388,84 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.data).toBeDefined(); expect(result.current.data?.window.windowIndex).toBe(0); - // Should have more pages available due to large time range - expect(result.current.hasNextPage).toBe(true); + // The only window has fewer rows than the requested limit. + expect(result.current.hasNextPage).toBe(false); }); }); describe('Pagination Within Time Windows', () => { + it.each(['Timestamp DESC', 'Timestamp ASC'])( + 'moves straight to the next window after a partial page (%s)', + async orderBy => { + const config = createMockChartConfig({ + orderBy, + limit: { limit: 100, offset: 0 }, + }); + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + { json: () => ['first window'] }, + ], + }) + .mockResolvedValueOnce({ done: true }); + + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.data?.data).toHaveLength(1); + expect(result.current.hasNextPage).toBe(true); + + mockReader.read.mockResolvedValueOnce({ done: true }); + await act(async () => { + await result.current.fetchNextPage(); + }); + + await waitFor(() => { + expect(result.current.data?.window.windowIndex).toBe(1); + }); + expect(jest.mocked(renderChartConfig).mock.calls[1][0]).toMatchObject({ + limit: { limit: 100, offset: 0 }, + }); + }, + ); + + it.each([ + ['a search without windowing', { orderBy: 'message DESC' }], + ['a query without LIMIT', { orderBy: 'message DESC', limit: undefined }], + [ + 'the final time window', + { + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-01T00:05:00Z'), + ] as [Date, Date], + }, + ], + ])('stops after a partial page for %s', async (_name, overrides) => { + const config = createMockChartConfig(overrides); + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + { json: () => ['only result'] }, + ], + }) + .mockResolvedValueOnce({ done: true }); + + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.data?.data).toHaveLength(1); + expect(result.current.hasNextPage).toBe(false); + }); + it('should paginate within the same time window', async () => { const config = createMockChartConfig({ limit: { limit: 2, offset: 0 }, diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index e8b5b573f6..5fd513ae31 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -159,8 +159,13 @@ function getNextPageParam( 0, ); - // If we have results in the current window, continue paginating within it - if (lastPage.data.length > 0) { + // A short page, or a query without LIMIT, has exhausted this window. + const requestedLimit = config.limit?.limit; + if ( + lastPage.data.length > 0 && + requestedLimit != null && + lastPage.data.length >= requestedLimit + ) { return { windowIndex: currentWindow.windowIndex, offset: currentWindowResults, From a2114da8e9f856fee9f4e0e8ba55915d027a08ed Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:56:55 -0600 Subject: [PATCH 2/8] test: cover search window pagination after a partial page --- .../useOffsetPaginatedQuery.test.tsx | 71 +++++++++++++++++++ .../app/src/hooks/useOffsetPaginatedQuery.tsx | 3 +- 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 9130c6ac2c..dc9b8b5ea0 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -515,10 +515,81 @@ describe('useOffsetPaginatedQuery', () => { await waitFor(() => { expect(result.current.data?.data).toHaveLength(4); }); + expect(jest.mocked(renderChartConfig).mock.calls[1][0]).toMatchObject({ + limit: { limit: 2, offset: 2 }, + }); + expect(result.current.data?.window.windowIndex).toBe(0); // Should still have more pages available in current window expect(result.current.hasNextPage).toBe(true); }); + + it.each(['Timestamp DESC', 'Timestamp ASC'])( + 'moves to the next window after a full page then a partial page (%s)', + async orderBy => { + const config = createMockChartConfig({ + orderBy, + limit: { limit: 2, offset: 0 }, + }); + for (const messages of [ + ['first row', 'second row'], + ['third row'], + ['next window'], + ]) { + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + ...messages.map(message => ({ json: () => [message] })), + ], + }) + .mockResolvedValueOnce({ done: true }); + } + + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.data?.window.windowIndex).toBe(0); + + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => { + expect(result.current.data?.data).toEqual([ + { message: 'first row' }, + { message: 'second row' }, + { message: 'third row' }, + ]); + }); + expect(result.current.data?.window.windowIndex).toBe(0); + expect(result.current.hasNextPage).toBe(true); + + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => { + expect(result.current.data?.window.windowIndex).toBe(1); + }); + expect(result.current.data?.data).toEqual([ + { message: 'first row' }, + { message: 'second row' }, + { message: 'third row' }, + { message: 'next window' }, + ]); + expect( + jest + .mocked(renderChartConfig) + .mock.calls.map(([config]) => config.limit), + ).toEqual([ + { limit: 2, offset: 0 }, + { limit: 2, offset: 2 }, + { limit: 2, offset: 0 }, + ]); + }, + ); }); describe('Moving Between Time Windows', () => { diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index 5fd513ae31..7ac469b415 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -159,7 +159,6 @@ function getNextPageParam( 0, ); - // A short page, or a query without LIMIT, has exhausted this window. const requestedLimit = config.limit?.limit; if ( lastPage.data.length > 0 && @@ -172,7 +171,7 @@ function getNextPageParam( }; } - // If no more results in current window, move to next window (if windowing is being used) + // A short page, or a query without LIMIT, has exhausted this window. const shouldUseWindowing = isBuilderChartConfig(config) && isTimestampExpressionInFirstOrderBy(config); const nextWindowIndex = currentWindow.windowIndex + 1; From e55101fd7d2ea31e550da6ba8050fc7cbae6342e Mon Sep 17 00:00:00 2001 From: adityareddy-dev <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 13:53:52 -0600 Subject: [PATCH 3/8] test: cover unlimited and empty search pages --- .../useOffsetPaginatedQuery.test.tsx | 61 ++++++++++++++----- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index dc9b8b5ea0..0fc3d8ca3a 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -394,12 +394,17 @@ describe('useOffsetPaginatedQuery', () => { }); describe('Pagination Within Time Windows', () => { - it.each(['Timestamp DESC', 'Timestamp ASC'])( - 'moves straight to the next window after a partial page (%s)', - async orderBy => { + it.each([ + { orderBy: 'Timestamp DESC', limit: 100 }, + { orderBy: 'Timestamp ASC', limit: 100 }, + { orderBy: 'Timestamp DESC', limit: undefined }, + { orderBy: 'Timestamp ASC', limit: undefined }, + ])( + 'moves straight to the next window after a nonempty page ($orderBy, limit: $limit)', + async ({ orderBy, limit }) => { const config = createMockChartConfig({ orderBy, - limit: { limit: 100, offset: 0 }, + limit: limit == null ? undefined : { limit, offset: 0 }, }); mockReader.read .mockResolvedValueOnce({ @@ -416,10 +421,22 @@ describe('useOffsetPaginatedQuery', () => { wrapper, }); await waitFor(() => expect(result.current.isLoading).toBe(false)); - expect(result.current.data?.data).toHaveLength(1); + expect(result.current.data?.window.windowIndex).toBe(0); + expect(result.current.data?.data).toEqual([ + { message: 'first window' }, + ]); expect(result.current.hasNextPage).toBe(true); - mockReader.read.mockResolvedValueOnce({ done: true }); + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + { json: () => ['second window'] }, + ], + }) + .mockResolvedValueOnce({ done: true }); await act(async () => { await result.current.fetchNextPage(); }); @@ -427,9 +444,18 @@ describe('useOffsetPaginatedQuery', () => { await waitFor(() => { expect(result.current.data?.window.windowIndex).toBe(1); }); - expect(jest.mocked(renderChartConfig).mock.calls[1][0]).toMatchObject({ - limit: { limit: 100, offset: 0 }, - }); + expect(result.current.data?.data).toEqual([ + { message: 'first window' }, + { message: 'second window' }, + ]); + expect( + jest + .mocked(renderChartConfig) + .mock.calls.map(([config]) => config.limit), + ).toEqual([ + { limit, offset: 0 }, + { limit, offset: 0 }, + ]); }, ); @@ -524,16 +550,21 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.hasNextPage).toBe(true); }); - it.each(['Timestamp DESC', 'Timestamp ASC'])( - 'moves to the next window after a full page then a partial page (%s)', - async orderBy => { + it.each([ + { orderBy: 'Timestamp DESC', remainingMessages: ['third row'] }, + { orderBy: 'Timestamp ASC', remainingMessages: ['third row'] }, + { orderBy: 'Timestamp DESC', remainingMessages: [] }, + { orderBy: 'Timestamp ASC', remainingMessages: [] }, + ])( + 'moves to the next window after a full page then an exhausted page ($orderBy, remaining: $remainingMessages)', + async ({ orderBy, remainingMessages }) => { const config = createMockChartConfig({ orderBy, limit: { limit: 2, offset: 0 }, }); for (const messages of [ ['first row', 'second row'], - ['third row'], + remainingMessages, ['next window'], ]) { mockReader.read @@ -561,7 +592,7 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.data?.data).toEqual([ { message: 'first row' }, { message: 'second row' }, - { message: 'third row' }, + ...remainingMessages.map(message => ({ message })), ]); }); expect(result.current.data?.window.windowIndex).toBe(0); @@ -576,7 +607,7 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.data?.data).toEqual([ { message: 'first row' }, { message: 'second row' }, - { message: 'third row' }, + ...remainingMessages.map(message => ({ message })), { message: 'next window' }, ]); expect( From 7f49343e5aa4b3cea00ccc72e16de789b50f4b87 Mon Sep 17 00:00:00 2001 From: adityareddy-dev <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:23:36 -0600 Subject: [PATCH 4/8] fix: cover final-window pagination and cumulative offsets --- .../useOffsetPaginatedQuery.test.tsx | 122 ++++++++++-------- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 18 +-- 2 files changed, 78 insertions(+), 62 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 0fc3d8ca3a..5e72640100 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -492,63 +492,79 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.hasNextPage).toBe(false); }); - it('should paginate within the same time window', async () => { - const config = createMockChartConfig({ - limit: { limit: 2, offset: 0 }, - }); - - // Mock the reader to return first batch - mockReader.read - .mockResolvedValueOnce({ - done: false, - value: [ - { json: () => ['timestamp', 'message'] }, - { json: () => ['DateTime', 'String'] }, - { json: () => ['2024-01-01T01:00:00Z', 'test log 1'] }, - { json: () => ['2024-01-01T02:00:00Z', 'test log 2'] }, - ], - }) - .mockResolvedValueOnce({ done: true }); - - const { result } = renderHook(() => useOffsetPaginatedQuery(config), { - wrapper, - }); - - await waitFor(() => expect(result.current.isLoading).toBe(false)); - - // Should have 2 results from first page - expect(result.current.data?.data).toHaveLength(2); - expect(result.current.hasNextPage).toBe(true); - - // Mock next page data - mockReader.read - .mockResolvedValueOnce({ - done: false, - value: [ - { json: () => ['timestamp', 'message'] }, - { json: () => ['DateTime', 'String'] }, - { json: () => ['2024-01-01T03:00:00Z', 'test log 3'] }, - { json: () => ['2024-01-01T04:00:00Z', 'test log 4'] }, + it.each([ + { orderBy: 'Timestamp DESC', fullPages: 1 }, + { orderBy: 'Timestamp ASC', fullPages: 1 }, + { orderBy: 'Timestamp DESC', fullPages: 3 }, + { orderBy: 'Timestamp ASC', fullPages: 3 }, + ])( + 'stops after an empty probe in the final window ($orderBy, full pages: $fullPages)', + async ({ orderBy, fullPages }) => { + const config = createMockChartConfig({ + orderBy, + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-01T00:05:00Z'), ], - }) - .mockResolvedValueOnce({ done: true }); + limit: { limit: 2, offset: 0 }, + }); + const messages = Array.from( + { length: fullPages * 2 }, + (_, index) => `row ${index + 1}`, + ); + for (let page = 0; page <= fullPages; page++) { + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + ...messages.slice(page * 2, page * 2 + 2).map(message => ({ + json: () => [message], + })), + ], + }) + .mockResolvedValueOnce({ done: true }); + } - // Fetch next page - await act(async () => { - await result.current.fetchNextPage(); - }); + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); - await waitFor(() => { - expect(result.current.data?.data).toHaveLength(4); - }); - expect(jest.mocked(renderChartConfig).mock.calls[1][0]).toMatchObject({ - limit: { limit: 2, offset: 2 }, - }); - expect(result.current.data?.window.windowIndex).toBe(0); + for (let page = 0; page < fullPages; page++) { + await waitFor(() => { + expect(result.current.data?.data).toEqual( + messages.slice(0, (page + 1) * 2).map(message => ({ message })), + ); + }); + expect(result.current.data?.window.windowIndex).toBe(0); + expect(result.current.hasNextPage).toBe(true); + + await act(async () => { + await result.current.fetchNextPage(); + }); + } - // Should still have more pages available in current window - expect(result.current.hasNextPage).toBe(true); - }); + await waitFor(() => expect(result.current.hasNextPage).toBe(false)); + expect(result.current.data?.data).toEqual( + messages.map(message => ({ message })), + ); + expect(result.current.data?.window.windowIndex).toBe(0); + expect( + jest.mocked(renderChartConfig).mock.calls.map(([config]) => ({ + dateRange: config.dateRange, + limit: config.limit, + })), + ).toEqual( + Array.from({ length: fullPages + 1 }, (_, page) => ({ + dateRange: config.dateRange, + limit: { limit: 2, offset: page * 2 }, + })), + ); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(fullPages + 1); + }, + ); it.each([ { orderBy: 'Timestamp DESC', remainingMessages: ['third row'] }, diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index 7ac469b415..e83b6cc3c9 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -150,21 +150,21 @@ function getNextPageParam( : generateTimeWindowsDescending(startDate, endDate, windowDurationsSeconds); const currentWindow = lastPage.window; - // Calculate total results from all pages in current window - const currentWindowPages = allPages.filter( - p => p.window.windowIndex === currentWindow.windowIndex, - ); - const currentWindowResults = currentWindowPages.reduce( - (sum, page) => sum + page.data.length, - 0, - ); - const requestedLimit = config.limit?.limit; if ( lastPage.data.length > 0 && requestedLimit != null && lastPage.data.length >= requestedLimit ) { + // Calculate total results from all pages in current window + const currentWindowPages = allPages.filter( + p => p.window.windowIndex === currentWindow.windowIndex, + ); + const currentWindowResults = currentWindowPages.reduce( + (sum, page) => sum + page.data.length, + 0, + ); + return { windowIndex: currentWindow.windowIndex, offset: currentWindowResults, From bdac94d5cc2e1abf07efd19068d845f3a4b32f31 Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 14:52:45 -0600 Subject: [PATCH 5/8] test: narrow pagination mock configs --- .../hooks/__tests__/useOffsetPaginatedQuery.test.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 5e72640100..d26aefb9c6 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -451,7 +451,9 @@ describe('useOffsetPaginatedQuery', () => { expect( jest .mocked(renderChartConfig) - .mock.calls.map(([config]) => config.limit), + .mock.calls.map(([config]) => + isBuilderChartConfig(config) ? config.limit : undefined, + ), ).toEqual([ { limit, offset: 0 }, { limit, offset: 0 }, @@ -554,7 +556,7 @@ describe('useOffsetPaginatedQuery', () => { expect( jest.mocked(renderChartConfig).mock.calls.map(([config]) => ({ dateRange: config.dateRange, - limit: config.limit, + limit: isBuilderChartConfig(config) ? config.limit : undefined, })), ).toEqual( Array.from({ length: fullPages + 1 }, (_, page) => ({ @@ -629,7 +631,9 @@ describe('useOffsetPaginatedQuery', () => { expect( jest .mocked(renderChartConfig) - .mock.calls.map(([config]) => config.limit), + .mock.calls.map(([config]) => + isBuilderChartConfig(config) ? config.limit : undefined, + ), ).toEqual([ { limit: 2, offset: 0 }, { limit: 2, offset: 2 }, From aa6623c43295436bdd64bbbb68e4c003f27d2c5b Mon Sep 17 00:00:00 2001 From: Aditya Reddy <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:30:08 -0600 Subject: [PATCH 6/8] fix: preserve search pagination under result caps --- .changeset/skip-exhausted-search-pages.md | 2 +- .../useOffsetPaginatedQuery.test.tsx | 301 +++++++++++++++++- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 35 +- 3 files changed, 333 insertions(+), 5 deletions(-) diff --git a/.changeset/skip-exhausted-search-pages.md b/.changeset/skip-exhausted-search-pages.md index 8eacdbeff9..68768b6ea6 100644 --- a/.changeset/skip-exhausted-search-pages.md +++ b/.changeset/skip-exhausted-search-pages.md @@ -3,4 +3,4 @@ --- Skip redundant offset queries after a search returns fewer rows than the -requested page size. +requested page size when result settings rule out truncation. diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index d26aefb9c6..715c3c6560 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -3,6 +3,7 @@ import { ClickHouseQueryError } from '@hyperdx/common-utils/dist/clickhouse'; import { ChartConfigWithDateRange, PromqlConfigWithDateRange, + QuerySettings, } from '@hyperdx/common-utils/dist/types'; import { QueryClient, @@ -75,6 +76,7 @@ import { MVOptimizationExplanationResult, useMVOptimizationExplanation, } from '@/hooks/useMVOptimizationExplanation'; +import { useSource } from '@/source'; import { queryPromqlChartConfig } from '@/utils/promqlChartQuery'; // Create a mock ChartConfig based on the Zod schema @@ -109,7 +111,7 @@ describe('useOffsetPaginatedQuery', () => { let mockClickhouseClient: any; let mockStream: any; let mockReader: any; - let mockMetadata: { getSetting: jest.Mock }; + let mockMetadata: { getSetting: jest.Mock; getSettings: jest.Mock }; beforeEach(() => { // Reset mocks @@ -161,8 +163,19 @@ describe('useOffsetPaginatedQuery', () => { // Mock metadata with getSetting returning null by default mockMetadata = { getSetting: jest.fn().mockResolvedValue(null), + getSettings: jest.fn().mockResolvedValue( + new Map([ + ['result_overflow_mode', 'throw'], + ['max_result_rows', '0'], + ['max_result_bytes', '0'], + ]), + ), }; jest.mocked(useMetadataWithSettings).mockReturnValue(mockMetadata as any); + jest.mocked(useSource).mockReturnValue({ + data: undefined, + isLoading: false, + } as ReturnType); }); describe('Time Window Generation', () => { @@ -394,6 +407,292 @@ describe('useOffsetPaginatedQuery', () => { }); describe('Pagination Within Time Windows', () => { + it.each( + [ + { + name: 'source row cap', + server: { + max_result_rows: '0', + max_result_bytes: '0', + result_overflow_mode: 'throw', + }, + source: { max_result_rows: '1', result_overflow_mode: 'break' }, + }, + { + name: 'source byte cap', + server: { + max_result_rows: '0', + max_result_bytes: '0', + result_overflow_mode: 'throw', + }, + source: { max_result_bytes: '1', result_overflow_mode: 'break' }, + }, + { + name: 'inherited row cap', + server: { max_result_rows: '1', result_overflow_mode: 'break' }, + source: {}, + }, + { + name: 'inherited byte cap', + server: { max_result_bytes: '1', result_overflow_mode: 'break' }, + source: {}, + }, + { name: 'unknown settings', server: {}, source: {} }, + { + name: 'unknown byte cap', + server: { max_result_rows: '0', result_overflow_mode: 'break' }, + source: {}, + }, + { + name: 'inherited byte cap with source row cap disabled', + server: { + max_result_rows: '1', + max_result_bytes: '1', + result_overflow_mode: 'break', + }, + source: { max_result_rows: '0' }, + }, + ].flatMap(testCase => + ['Timestamp DESC', 'Timestamp ASC'].map(orderBy => ({ + ...testCase, + orderBy, + })), + ), + )( + 'keeps offset paging after a short page with $name ($orderBy)', + async ({ orderBy, server, source }) => { + const config = createMockChartConfig({ orderBy }); + mockMetadata.getSettings.mockResolvedValue( + new Map(Object.entries(server)), + ); + const querySettings: QuerySettings = Object.entries(source).map( + ([setting, value]) => ({ setting, value }), + ); + jest.mocked(useSource).mockReturnValue({ + data: { querySettings }, + isLoading: false, + } as ReturnType); + for (const messages of [ + ['first row'], + ['second row'], + [], + ['next window'], + ]) { + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + ...messages.map(message => ({ json: () => [message] })), + ], + }) + .mockResolvedValueOnce({ done: true }); + } + + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => { + expect(result.current.data?.data).toEqual([ + { message: 'first row' }, + { message: 'second row' }, + ]); + }); + expect(result.current.data?.window.windowIndex).toBe(0); + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(3), + ); + expect(result.current.hasNextPage).toBe(true); + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => + expect(result.current.data?.window.windowIndex).toBe(1), + ); + expect(result.current.data?.data).toEqual([ + { message: 'first row' }, + { message: 'second row' }, + { message: 'next window' }, + ]); + expect( + jest + .mocked(renderChartConfig) + .mock.calls.map(([config]) => + isBuilderChartConfig(config) ? config.limit : undefined, + ), + ).toEqual([ + { limit: 100, offset: 0 }, + { limit: 100, offset: 1 }, + { limit: 100, offset: 2 }, + { limit: 100, offset: 0 }, + ]); + expect(mockMetadata.getSettings).toHaveBeenCalledWith({ + connectionId: 'foo', + }); + expect(jest.mocked(renderChartConfig).mock.calls[0][2]).toEqual( + querySettings, + ); + expect( + mockClickhouseClient.query.mock.calls.every( + ([query]: [{ clickhouse_settings: object }]) => + Object.keys(query.clickhouse_settings).length === 0, + ), + ).toBe(true); + }, + ); + + it.each([ + { + name: 'uncapped break mode', + server: { + max_result_rows: '0', + max_result_bytes: '0', + result_overflow_mode: 'break', + }, + source: {}, + }, + { + name: 'source caps disabled', + server: { + max_result_rows: '1', + max_result_bytes: '1', + result_overflow_mode: 'break', + }, + source: { max_result_rows: '0', max_result_bytes: '0' }, + }, + { + name: 'source throw mode', + server: { + max_result_rows: '1', + max_result_bytes: '1', + result_overflow_mode: 'break', + }, + source: { result_overflow_mode: 'throw' }, + }, + { + name: 'throw mode with unknown caps', + server: { result_overflow_mode: 'throw' }, + source: {}, + }, + ])('finishes a short page with $name', async ({ server, source }) => { + const config = createMockChartConfig({ orderBy: 'message DESC' }); + mockMetadata.getSettings.mockResolvedValue( + new Map(Object.entries(server)), + ); + jest.mocked(useSource).mockReturnValue({ + data: { + querySettings: Object.entries(source).map(([setting, value]) => ({ + setting, + value, + })), + }, + isLoading: false, + } as ReturnType); + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + { json: () => ['only row'] }, + ], + }) + .mockResolvedValueOnce({ done: true }); + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.data?.data).toEqual([{ message: 'only row' }]); + expect(result.current.hasNextPage).toBe(false); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1); + }); + + it.each(['message DESC', 'Timestamp DESC', 'Timestamp ASC'])( + 'finishes after an empty page when settings are unknown (%s)', + async orderBy => { + const config = createMockChartConfig({ + orderBy, + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-01T00:05:00Z'), + ], + }); + mockMetadata.getSettings.mockResolvedValue(new Map()); + for (const messages of [['only row'], []]) { + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + ...messages.map(message => ({ json: () => [message] })), + ], + }) + .mockResolvedValueOnce({ done: true }); + } + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.hasNextPage).toBe(true); + await act(async () => { + await result.current.fetchNextPage(); + }); + await waitFor(() => expect(result.current.hasNextPage).toBe(false)); + expect(result.current.data?.data).toEqual([{ message: 'only row' }]); + expect( + jest + .mocked(renderChartConfig) + .mock.calls.map(([config]) => + isBuilderChartConfig(config) ? config.limit : undefined, + ), + ).toEqual([ + { limit: 100, offset: 0 }, + { limit: 100, offset: 1 }, + ]); + }, + ); + + it.each([ + { limit: undefined, orderBy: 'message DESC' }, + { limit: 0, orderBy: 'Timestamp DESC' }, + { limit: 0, orderBy: 'Timestamp ASC' }, + ])( + 'does not offset page with limit $limit when settings are unknown ($orderBy)', + async ({ limit, orderBy }) => { + const config = createMockChartConfig({ + orderBy, + limit: { limit, offset: 0 }, + }); + mockMetadata.getSettings.mockResolvedValue(new Map()); + mockReader.read + .mockResolvedValueOnce({ + done: false, + value: [ + { json: () => ['message'] }, + { json: () => ['String'] }, + ...(limit === 0 ? [] : [{ json: () => ['only row'] }]), + ], + }) + .mockResolvedValueOnce({ done: true }); + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); + await waitFor(() => expect(result.current.isLoading).toBe(false)); + expect(result.current.hasNextPage).toBe(false); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1); + expect(mockMetadata.getSettings).not.toHaveBeenCalled(); + }, + ); + it.each([ { orderBy: 'Timestamp DESC', limit: 100 }, { orderBy: 'Timestamp ASC', limit: 100 }, diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index e83b6cc3c9..59e6a28402 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -17,6 +17,7 @@ import { renderChartConfig } from '@hyperdx/common-utils/dist/core/renderChartCo import { isFirstOrderByAscending, isTimestampExpressionInFirstOrderBy, + parseToNumber, } from '@hyperdx/common-utils/dist/core/utils'; import { isBuilderChartConfig, @@ -84,6 +85,7 @@ type TQueryFnData = { meta: ColumnMetaType[]; chSql: ChSql; window: TimeWindow; + canSkipShortPage?: boolean; }; type TData = { @@ -151,10 +153,15 @@ function getNextPageParam( const currentWindow = lastPage.window; const requestedLimit = config.limit?.limit; + if (requestedLimit === 0) { + return undefined; + } + if ( lastPage.data.length > 0 && requestedLimit != null && - lastPage.data.length >= requestedLimit + requestedLimit > 0 && + (lastPage.data.length >= requestedLimit || !lastPage.canSkipShortPage) ) { // Calculate total results from all pages in current window const currentWindowPages = allPages.filter( @@ -171,7 +178,8 @@ function getNextPageParam( }; } - // A short page, or a query without LIMIT, has exhausted this window. + // Short pages prove exhaustion only when result caps cannot truncate them. + // Without LIMIT, the rendered query has no OFFSET, so it cannot page. const shouldUseWindowing = isBuilderChartConfig(config) && isTimestampExpressionInFirstOrderBy(config); const nextWindowIndex = currentWindow.windowIndex + 1; @@ -264,6 +272,26 @@ const queryFn: QueryFunction = async ({ source?.querySettings, ); + let canSkipShortPage = false; + if (isBuilderChartConfig(config) && (config.limit?.limit ?? 0) > 0) { + const serverSettings = await metadata.getSettings({ + connectionId: config.connection, + }); + const sourceSettings = new Map( + source?.querySettings + ?.filter(({ setting, value }) => setting.length && value.length) + .map(({ setting, value }) => [setting, value]), + ); + const getSetting = (name: string) => + sourceSettings.get(name) ?? serverSettings.get(name); + canSkipShortPage = + getSetting('result_overflow_mode') === 'throw' || + ['max_result_rows', 'max_result_bytes'].every(name => { + const value = getSetting(name); + return value != null && parseToNumber(value) === 0; + }); + } + // Create abort signal from timeout if provided const abortController = queryTimeout ? new AbortController() : undefined; if (abortController && queryTimeout) { @@ -448,6 +476,7 @@ const queryFn: QueryFunction = async ({ meta: queryResultMeta, chSql: query, window: timeWindow, + canSkipShortPage, }; } @@ -461,7 +490,7 @@ const queryFn: QueryFunction = async ({ deleteProgressCache(); - return lastPage; + return { ...lastPage, canSkipShortPage }; }; function flattenPages(pages: TQueryFnData[]) { From 01081a84c27ac6e0d8be8592e1554a8684d594de Mon Sep 17 00:00:00 2001 From: adityareddy-dev <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:49:00 -0600 Subject: [PATCH 7/8] fix: preserve paging under partial query limits --- .../useOffsetPaginatedQuery.test.tsx | 152 ++++++++++++++++-- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 50 +++++- 2 files changed, 186 insertions(+), 16 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 715c3c6560..e56754dcd0 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -14,12 +14,14 @@ import { renderHook, waitFor } from '@testing-library/react'; import useOffsetPaginatedQuery from '@/hooks/useOffsetPaginatedQuery'; +let mockQueryTimeout = 30; + // Mock the API module jest.mock('@/api', () => ({ useMe: () => ({ data: { team: { - queryTimeout: 30, + queryTimeout: mockQueryTimeout, }, }, }), @@ -102,6 +104,18 @@ const createMockChartConfig = ( ...overrides, }) as ChartConfigWithDateRange; +const nonTruncatingSettings = { + result_overflow_mode: 'throw', + read_overflow_mode: 'throw', + timeout_overflow_mode: 'throw', + sort_overflow_mode: 'throw', + group_by_overflow_mode: 'throw', + distinct_overflow_mode: 'throw', + set_overflow_mode: 'throw', + join_overflow_mode: 'throw', + transfer_overflow_mode: 'throw', +}; + describe('useOffsetPaginatedQuery', () => { // Increase timeout for complex async operations jest.setTimeout(15000); @@ -116,6 +130,7 @@ describe('useOffsetPaginatedQuery', () => { beforeEach(() => { // Reset mocks jest.clearAllMocks(); + mockQueryTimeout = 30; // Create a new QueryClient for each test queryClient = new QueryClient({ @@ -163,13 +178,15 @@ describe('useOffsetPaginatedQuery', () => { // Mock metadata with getSetting returning null by default mockMetadata = { getSetting: jest.fn().mockResolvedValue(null), - getSettings: jest.fn().mockResolvedValue( - new Map([ - ['result_overflow_mode', 'throw'], - ['max_result_rows', '0'], - ['max_result_bytes', '0'], - ]), - ), + getSettings: jest + .fn() + .mockResolvedValue( + new Map([ + ...Object.entries(nonTruncatingSettings), + ['max_result_rows', '0'], + ['max_result_bytes', '0'], + ]), + ), }; jest.mocked(useMetadataWithSettings).mockReturnValue(mockMetadata as any); jest.mocked(useSource).mockReturnValue({ @@ -437,6 +454,66 @@ describe('useOffsetPaginatedQuery', () => { server: { max_result_bytes: '1', result_overflow_mode: 'break' }, source: {}, }, + ...[ + ['read_overflow_mode', 'max_rows_to_read'], + ['read_overflow_mode', 'max_bytes_to_read'], + ['read_overflow_mode_leaf', 'max_rows_to_read_leaf'], + ['read_overflow_mode_leaf', 'max_bytes_to_read_leaf'], + ['timeout_overflow_mode', 'max_execution_time'], + ['timeout_overflow_mode_leaf', 'max_execution_time_leaf'], + ['sort_overflow_mode', 'max_rows_to_sort'], + ['sort_overflow_mode', 'max_bytes_to_sort'], + ['group_by_overflow_mode', 'max_rows_to_group_by'], + ['distinct_overflow_mode', 'max_rows_in_distinct'], + ['set_overflow_mode', 'max_rows_in_set'], + ['join_overflow_mode', 'max_rows_in_join'], + ['transfer_overflow_mode', 'max_rows_to_transfer'], + ].flatMap(([mode, limit]) => + ['inherited', 'source'].map(location => ({ + name: `${location} ${limit}`, + server: { + ...nonTruncatingSettings, + ...(location === 'inherited' + ? { [mode]: 'break', [limit]: '1' } + : {}), + }, + source: + location === 'source' ? { [mode]: 'break', [limit]: '1' } : {}, + })), + ), + { + name: 'inherited timeout without team injection', + queryTimeout: 0, + server: { + ...nonTruncatingSettings, + max_execution_time: '1', + timeout_overflow_mode: 'break', + }, + source: {}, + }, + { + name: 'team timeout overriding inherited zero', + server: { + ...nonTruncatingSettings, + max_execution_time: '0', + timeout_overflow_mode: 'break', + }, + source: {}, + }, + { + name: 'unknown read cap', + server: { + ...nonTruncatingSettings, + read_overflow_mode: 'break', + max_rows_to_read: '0', + }, + source: {}, + }, + { + name: 'unknown leaf timeout mode', + server: { ...nonTruncatingSettings, max_execution_time_leaf: '1' }, + source: {}, + }, { name: 'unknown settings', server: {}, source: {} }, { name: 'unknown byte cap', @@ -454,16 +531,24 @@ describe('useOffsetPaginatedQuery', () => { }, ].flatMap(testCase => ['Timestamp DESC', 'Timestamp ASC'].map(orderBy => ({ + queryTimeout: 30, ...testCase, orderBy, })), ), )( 'keeps offset paging after a short page with $name ($orderBy)', - async ({ orderBy, server, source }) => { + async ({ orderBy, server, source, queryTimeout }) => { + mockQueryTimeout = queryTimeout; const config = createMockChartConfig({ orderBy }); mockMetadata.getSettings.mockResolvedValue( - new Map(Object.entries(server)), + new Map( + Object.entries( + Object.keys(server).length + ? { ...nonTruncatingSettings, ...server } + : server, + ), + ), ); const querySettings: QuerySettings = Object.entries(source).map( ([setting, value]) => ({ setting, value }), @@ -550,6 +635,51 @@ describe('useOffsetPaginatedQuery', () => { ); it.each([ + { + name: 'uncapped reads and sort with leaf limits unavailable', + server: { + read_overflow_mode: 'break', + max_rows_to_read: '0', + max_bytes_to_read: '0', + sort_overflow_mode: 'break', + max_rows_to_sort: '0', + max_bytes_to_sort: '0', + }, + source: {}, + }, + { + name: 'source timeout disabled despite team timeout', + server: { timeout_overflow_mode: 'break', max_execution_time: '1' }, + source: { max_execution_time: '0' }, + }, + { + name: 'source read caps disabled', + server: { + read_overflow_mode: 'break', + max_rows_to_read: '1', + max_bytes_to_read: '1', + }, + source: { max_rows_to_read: '0', max_bytes_to_read: '0' }, + }, + { + name: 'leaf caps disabled', + server: { + read_overflow_mode_leaf: 'break', + max_rows_to_read_leaf: '0', + max_bytes_to_read_leaf: '0', + timeout_overflow_mode_leaf: 'break', + max_execution_time_leaf: '0', + }, + source: {}, + }, + { + name: 'leaf throw modes', + server: { + read_overflow_mode_leaf: 'throw', + timeout_overflow_mode_leaf: 'throw', + }, + source: {}, + }, { name: 'uncapped break mode', server: { @@ -585,7 +715,7 @@ describe('useOffsetPaginatedQuery', () => { ])('finishes a short page with $name', async ({ server, source }) => { const config = createMockChartConfig({ orderBy: 'message DESC' }); mockMetadata.getSettings.mockResolvedValue( - new Map(Object.entries(server)), + new Map(Object.entries({ ...nonTruncatingSettings, ...server })), ); jest.mocked(useSource).mockReturnValue({ data: { diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index 59e6a28402..3caa9f0b48 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -178,7 +178,7 @@ function getNextPageParam( }; } - // Short pages prove exhaustion only when result caps cannot truncate them. + // Short pages prove exhaustion only when query limits cannot truncate them. // Without LIMIT, the rendered query has no OFFSET, so it cannot page. const shouldUseWindowing = isBuilderChartConfig(config) && isTimestampExpressionInFirstOrderBy(config); @@ -282,14 +282,54 @@ const queryFn: QueryFunction = async ({ ?.filter(({ setting, value }) => setting.length && value.length) .map(({ setting, value }) => [setting, value]), ); + // SQL SETTINGS override the timeout injected by the client, then server defaults. const getSetting = (name: string) => - sourceSettings.get(name) ?? serverSettings.get(name); - canSkipShortPage = - getSetting('result_overflow_mode') === 'throw' || - ['max_result_rows', 'max_result_bytes'].every(name => { + sourceSettings.get(name) ?? + (name === 'max_execution_time' && (queryTimeout ?? 0) > 0 + ? String(queryTimeout) + : serverSettings.get(name)); + const cannotTruncate = (mode: string, limits: string[]) => + getSetting(mode) === 'throw' || + limits.every(name => { const value = getSetting(name); return value != null && parseToNumber(value) === 0; }); + const limitsByMode: [string, string[]][] = [ + ['result_overflow_mode', ['max_result_rows', 'max_result_bytes']], + ['read_overflow_mode', ['max_rows_to_read', 'max_bytes_to_read']], + ['timeout_overflow_mode', ['max_execution_time']], + ['sort_overflow_mode', ['max_rows_to_sort', 'max_bytes_to_sort']], + ['group_by_overflow_mode', ['max_rows_to_group_by']], + [ + 'distinct_overflow_mode', + ['max_rows_in_distinct', 'max_bytes_in_distinct'], + ], + ['set_overflow_mode', ['max_rows_in_set', 'max_bytes_in_set']], + ['join_overflow_mode', ['max_rows_in_join', 'max_bytes_in_join']], + [ + 'transfer_overflow_mode', + ['max_rows_to_transfer', 'max_bytes_to_transfer'], + ], + ]; + canSkipShortPage = limitsByMode.every(([mode, limits]) => + cannotTruncate(mode, limits), + ); + + // Older servers do not expose all of the leaf settings. + const leafLimitsByMode: [string, string[]][] = [ + [ + 'read_overflow_mode_leaf', + ['max_rows_to_read_leaf', 'max_bytes_to_read_leaf'], + ], + ['timeout_overflow_mode_leaf', ['max_execution_time_leaf']], + ]; + canSkipShortPage &&= leafLimitsByMode.every(([mode, limits]) => { + const names = [mode, ...limits]; + return ( + names.every(name => getSetting(name) == null) || + cannotTruncate(mode, limits) + ); + }); } // Create abort signal from timeout if provided From 930687156558f3de63fa359d464efc7011728284 Mon Sep 17 00:00:00 2001 From: adityareddy-dev <82123567+adityareddy-dev@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:25:28 -0600 Subject: [PATCH 8/8] fix: keep paging when settings are unavailable or capped --- .../useOffsetPaginatedQuery.test.tsx | 86 +++++++++++++++---- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 17 ++-- 2 files changed, 80 insertions(+), 23 deletions(-) diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index e56754dcd0..090e3218fa 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -454,6 +454,16 @@ describe('useOffsetPaginatedQuery', () => { server: { max_result_bytes: '1', result_overflow_mode: 'break' }, source: {}, }, + { + name: 'inherited ClickHouse limit', + server: { ...nonTruncatingSettings, limit: '1' }, + source: {}, + }, + { + name: 'source ClickHouse limit', + server: { ...nonTruncatingSettings, limit: '100' }, + source: { limit: '1' }, + }, ...[ ['read_overflow_mode', 'max_rows_to_read'], ['read_overflow_mode', 'max_bytes_to_read'], @@ -515,6 +525,18 @@ describe('useOffsetPaginatedQuery', () => { source: {}, }, { name: 'unknown settings', server: {}, source: {} }, + { + name: 'failed settings lookup', + server: {}, + source: {}, + settingsLookupFails: true, + }, + { + name: 'failed settings lookup with source overrides', + server: {}, + source: nonTruncatingSettings, + settingsLookupFails: true, + }, { name: 'unknown byte cap', server: { max_result_rows: '0', result_overflow_mode: 'break' }, @@ -532,13 +554,20 @@ describe('useOffsetPaginatedQuery', () => { ].flatMap(testCase => ['Timestamp DESC', 'Timestamp ASC'].map(orderBy => ({ queryTimeout: 30, + settingsLookupFails: false, ...testCase, orderBy, })), ), )( 'keeps offset paging after a short page with $name ($orderBy)', - async ({ orderBy, server, source, queryTimeout }) => { + async ({ + orderBy, + server, + source, + queryTimeout, + settingsLookupFails, + }) => { mockQueryTimeout = queryTimeout; const config = createMockChartConfig({ orderBy }); mockMetadata.getSettings.mockResolvedValue( @@ -550,6 +579,11 @@ describe('useOffsetPaginatedQuery', () => { ), ), ); + if (settingsLookupFails) { + mockMetadata.getSettings.mockRejectedValue( + new Error('Settings lookup failed'), + ); + } const querySettings: QuerySettings = Object.entries(source).map( ([setting, value]) => ({ setting, value }), ); @@ -635,6 +669,18 @@ describe('useOffsetPaginatedQuery', () => { ); it.each([ + ...['0', '100', '200'].flatMap(limit => [ + { + name: `inherited ClickHouse limit=${limit}`, + server: { limit }, + source: {}, + }, + { + name: `source ClickHouse limit=${limit} overriding inherited cap`, + server: { limit: '1' }, + source: { limit }, + }, + ]), { name: 'uncapped reads and sort with leaf limits unavailable', server: { @@ -1291,25 +1337,33 @@ describe('useOffsetPaginatedQuery', () => { }); describe('Error Handling', () => { - it('should handle ClickHouse query errors gracefully', async () => { - const config = createMockChartConfig(); + it.each([false, true])( + 'should surface ClickHouse query errors when settings lookup fails: %s', + async settingsLookupFails => { + const config = createMockChartConfig(); + if (settingsLookupFails) { + mockMetadata.getSettings.mockRejectedValue( + new Error('Settings lookup failed'), + ); + } - // Mock the clickhouse client to throw an error during query execution - mockClickhouseClient.query.mockRejectedValue( - new ClickHouseQueryError('Query failed', 'SELECT * FROM traces'), - ); + // Mock the clickhouse client to throw an error during query execution + mockClickhouseClient.query.mockRejectedValue( + new ClickHouseQueryError('Query failed', 'SELECT * FROM traces'), + ); - const { result } = renderHook(() => useOffsetPaginatedQuery(config), { - wrapper, - }); + const { result } = renderHook(() => useOffsetPaginatedQuery(config), { + wrapper, + }); - await waitFor(() => expect(result.current.isError).toBe(true), { - timeout: 5000, - }); + await waitFor(() => expect(result.current.isError).toBe(true), { + timeout: 5000, + }); - expect(result.current.error).toBeInstanceOf(ClickHouseQueryError); - expect(result.current.error?.message).toBe('Query failed'); - }); + expect(result.current.error).toBeInstanceOf(ClickHouseQueryError); + expect(result.current.error?.message).toBe('Query failed'); + }, + ); it('should handle invalid time window errors', async () => { const config = createMockChartConfig({ diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index 3caa9f0b48..44e7dd62f7 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -274,9 +274,9 @@ const queryFn: QueryFunction = async ({ let canSkipShortPage = false; if (isBuilderChartConfig(config) && (config.limit?.limit ?? 0) > 0) { - const serverSettings = await metadata.getSettings({ - connectionId: config.connection, - }); + const serverSettings = await metadata + .getSettings({ connectionId: config.connection }) + .catch(() => undefined); const sourceSettings = new Map( source?.querySettings ?.filter(({ setting, value }) => setting.length && value.length) @@ -287,7 +287,7 @@ const queryFn: QueryFunction = async ({ sourceSettings.get(name) ?? (name === 'max_execution_time' && (queryTimeout ?? 0) > 0 ? String(queryTimeout) - : serverSettings.get(name)); + : serverSettings?.get(name)); const cannotTruncate = (mode: string, limits: string[]) => getSetting(mode) === 'throw' || limits.every(name => { @@ -311,9 +311,12 @@ const queryFn: QueryFunction = async ({ ['max_rows_to_transfer', 'max_bytes_to_transfer'], ], ]; - canSkipShortPage = limitsByMode.every(([mode, limits]) => - cannotTruncate(mode, limits), - ); + const limit = parseToNumber(getSetting('limit') ?? '0'); + canSkipShortPage = + serverSettings != null && + limit != null && + (limit === 0 || limit >= (config.limit?.limit ?? 0)) && + limitsByMode.every(([mode, limits]) => cannotTruncate(mode, limits)); // Older servers do not expose all of the leaf settings. const leafLimitsByMode: [string, string[]][] = [