Skip to content

Commit 2a44a78

Browse files
adityareddy-devadityareddy-dev
committed
fix: preserve search pagination under result caps
1 parent bdac94d commit 2a44a78

3 files changed

Lines changed: 333 additions & 5 deletions

File tree

‎.changeset/skip-exhausted-search-pages.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33
---
44

55
Skip redundant offset queries after a search returns fewer rows than the
6-
requested page size.
6+
requested page size when result settings rule out truncation.

‎packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx‎

Lines changed: 300 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { ClickHouseQueryError } from '@hyperdx/common-utils/dist/clickhouse';
33
import {
44
ChartConfigWithDateRange,
55
PromqlConfigWithDateRange,
6+
QuerySettings,
67
} from '@hyperdx/common-utils/dist/types';
78
import {
89
QueryClient,
@@ -75,6 +76,7 @@ import {
7576
MVOptimizationExplanationResult,
7677
useMVOptimizationExplanation,
7778
} from '@/hooks/useMVOptimizationExplanation';
79+
import { useSource } from '@/source';
7880
import { queryPromqlChartConfig } from '@/utils/promqlChartQuery';
7981

8082
// Create a mock ChartConfig based on the Zod schema
@@ -109,7 +111,7 @@ describe('useOffsetPaginatedQuery', () => {
109111
let mockClickhouseClient: any;
110112
let mockStream: any;
111113
let mockReader: any;
112-
let mockMetadata: { getSetting: jest.Mock };
114+
let mockMetadata: { getSetting: jest.Mock; getSettings: jest.Mock };
113115

114116
beforeEach(() => {
115117
// Reset mocks
@@ -161,8 +163,19 @@ describe('useOffsetPaginatedQuery', () => {
161163
// Mock metadata with getSetting returning null by default
162164
mockMetadata = {
163165
getSetting: jest.fn().mockResolvedValue(null),
166+
getSettings: jest.fn().mockResolvedValue(
167+
new Map([
168+
['result_overflow_mode', 'throw'],
169+
['max_result_rows', '0'],
170+
['max_result_bytes', '0'],
171+
]),
172+
),
164173
};
165174
jest.mocked(useMetadataWithSettings).mockReturnValue(mockMetadata as any);
175+
jest.mocked(useSource).mockReturnValue({
176+
data: undefined,
177+
isLoading: false,
178+
} as ReturnType<typeof useSource>);
166179
});
167180

168181
describe('Time Window Generation', () => {
@@ -394,6 +407,292 @@ describe('useOffsetPaginatedQuery', () => {
394407
});
395408

396409
describe('Pagination Within Time Windows', () => {
410+
it.each(
411+
[
412+
{
413+
name: 'source row cap',
414+
server: {
415+
max_result_rows: '0',
416+
max_result_bytes: '0',
417+
result_overflow_mode: 'throw',
418+
},
419+
source: { max_result_rows: '1', result_overflow_mode: 'break' },
420+
},
421+
{
422+
name: 'source byte cap',
423+
server: {
424+
max_result_rows: '0',
425+
max_result_bytes: '0',
426+
result_overflow_mode: 'throw',
427+
},
428+
source: { max_result_bytes: '1', result_overflow_mode: 'break' },
429+
},
430+
{
431+
name: 'inherited row cap',
432+
server: { max_result_rows: '1', result_overflow_mode: 'break' },
433+
source: {},
434+
},
435+
{
436+
name: 'inherited byte cap',
437+
server: { max_result_bytes: '1', result_overflow_mode: 'break' },
438+
source: {},
439+
},
440+
{ name: 'unknown settings', server: {}, source: {} },
441+
{
442+
name: 'unknown byte cap',
443+
server: { max_result_rows: '0', result_overflow_mode: 'break' },
444+
source: {},
445+
},
446+
{
447+
name: 'inherited byte cap with source row cap disabled',
448+
server: {
449+
max_result_rows: '1',
450+
max_result_bytes: '1',
451+
result_overflow_mode: 'break',
452+
},
453+
source: { max_result_rows: '0' },
454+
},
455+
].flatMap(testCase =>
456+
['Timestamp DESC', 'Timestamp ASC'].map(orderBy => ({
457+
...testCase,
458+
orderBy,
459+
})),
460+
),
461+
)(
462+
'keeps offset paging after a short page with $name ($orderBy)',
463+
async ({ orderBy, server, source }) => {
464+
const config = createMockChartConfig({ orderBy });
465+
mockMetadata.getSettings.mockResolvedValue(
466+
new Map(Object.entries(server)),
467+
);
468+
const querySettings: QuerySettings = Object.entries(source).map(
469+
([setting, value]) => ({ setting, value }),
470+
);
471+
jest.mocked(useSource).mockReturnValue({
472+
data: { querySettings },
473+
isLoading: false,
474+
} as ReturnType<typeof useSource>);
475+
for (const messages of [
476+
['first row'],
477+
['second row'],
478+
[],
479+
['next window'],
480+
]) {
481+
mockReader.read
482+
.mockResolvedValueOnce({
483+
done: false,
484+
value: [
485+
{ json: () => ['message'] },
486+
{ json: () => ['String'] },
487+
...messages.map(message => ({ json: () => [message] })),
488+
],
489+
})
490+
.mockResolvedValueOnce({ done: true });
491+
}
492+
493+
const { result } = renderHook(() => useOffsetPaginatedQuery(config), {
494+
wrapper,
495+
});
496+
await waitFor(() => expect(result.current.isLoading).toBe(false));
497+
await act(async () => {
498+
await result.current.fetchNextPage();
499+
});
500+
await waitFor(() => {
501+
expect(result.current.data?.data).toEqual([
502+
{ message: 'first row' },
503+
{ message: 'second row' },
504+
]);
505+
});
506+
expect(result.current.data?.window.windowIndex).toBe(0);
507+
await act(async () => {
508+
await result.current.fetchNextPage();
509+
});
510+
await waitFor(() =>
511+
expect(mockClickhouseClient.query).toHaveBeenCalledTimes(3),
512+
);
513+
expect(result.current.hasNextPage).toBe(true);
514+
await act(async () => {
515+
await result.current.fetchNextPage();
516+
});
517+
await waitFor(() =>
518+
expect(result.current.data?.window.windowIndex).toBe(1),
519+
);
520+
expect(result.current.data?.data).toEqual([
521+
{ message: 'first row' },
522+
{ message: 'second row' },
523+
{ message: 'next window' },
524+
]);
525+
expect(
526+
jest
527+
.mocked(renderChartConfig)
528+
.mock.calls.map(([config]) =>
529+
isBuilderChartConfig(config) ? config.limit : undefined,
530+
),
531+
).toEqual([
532+
{ limit: 100, offset: 0 },
533+
{ limit: 100, offset: 1 },
534+
{ limit: 100, offset: 2 },
535+
{ limit: 100, offset: 0 },
536+
]);
537+
expect(mockMetadata.getSettings).toHaveBeenCalledWith({
538+
connectionId: 'foo',
539+
});
540+
expect(jest.mocked(renderChartConfig).mock.calls[0][2]).toEqual(
541+
querySettings,
542+
);
543+
expect(
544+
mockClickhouseClient.query.mock.calls.every(
545+
([query]: [{ clickhouse_settings: object }]) =>
546+
Object.keys(query.clickhouse_settings).length === 0,
547+
),
548+
).toBe(true);
549+
},
550+
);
551+
552+
it.each([
553+
{
554+
name: 'uncapped break mode',
555+
server: {
556+
max_result_rows: '0',
557+
max_result_bytes: '0',
558+
result_overflow_mode: 'break',
559+
},
560+
source: {},
561+
},
562+
{
563+
name: 'source caps disabled',
564+
server: {
565+
max_result_rows: '1',
566+
max_result_bytes: '1',
567+
result_overflow_mode: 'break',
568+
},
569+
source: { max_result_rows: '0', max_result_bytes: '0' },
570+
},
571+
{
572+
name: 'source throw mode',
573+
server: {
574+
max_result_rows: '1',
575+
max_result_bytes: '1',
576+
result_overflow_mode: 'break',
577+
},
578+
source: { result_overflow_mode: 'throw' },
579+
},
580+
{
581+
name: 'throw mode with unknown caps',
582+
server: { result_overflow_mode: 'throw' },
583+
source: {},
584+
},
585+
])('finishes a short page with $name', async ({ server, source }) => {
586+
const config = createMockChartConfig({ orderBy: 'message DESC' });
587+
mockMetadata.getSettings.mockResolvedValue(
588+
new Map(Object.entries(server)),
589+
);
590+
jest.mocked(useSource).mockReturnValue({
591+
data: {
592+
querySettings: Object.entries(source).map(([setting, value]) => ({
593+
setting,
594+
value,
595+
})),
596+
},
597+
isLoading: false,
598+
} as ReturnType<typeof useSource>);
599+
mockReader.read
600+
.mockResolvedValueOnce({
601+
done: false,
602+
value: [
603+
{ json: () => ['message'] },
604+
{ json: () => ['String'] },
605+
{ json: () => ['only row'] },
606+
],
607+
})
608+
.mockResolvedValueOnce({ done: true });
609+
const { result } = renderHook(() => useOffsetPaginatedQuery(config), {
610+
wrapper,
611+
});
612+
await waitFor(() => expect(result.current.isLoading).toBe(false));
613+
expect(result.current.data?.data).toEqual([{ message: 'only row' }]);
614+
expect(result.current.hasNextPage).toBe(false);
615+
expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1);
616+
});
617+
618+
it.each(['message DESC', 'Timestamp DESC', 'Timestamp ASC'])(
619+
'finishes after an empty page when settings are unknown (%s)',
620+
async orderBy => {
621+
const config = createMockChartConfig({
622+
orderBy,
623+
dateRange: [
624+
new Date('2024-01-01T00:00:00Z'),
625+
new Date('2024-01-01T00:05:00Z'),
626+
],
627+
});
628+
mockMetadata.getSettings.mockResolvedValue(new Map());
629+
for (const messages of [['only row'], []]) {
630+
mockReader.read
631+
.mockResolvedValueOnce({
632+
done: false,
633+
value: [
634+
{ json: () => ['message'] },
635+
{ json: () => ['String'] },
636+
...messages.map(message => ({ json: () => [message] })),
637+
],
638+
})
639+
.mockResolvedValueOnce({ done: true });
640+
}
641+
const { result } = renderHook(() => useOffsetPaginatedQuery(config), {
642+
wrapper,
643+
});
644+
await waitFor(() => expect(result.current.isLoading).toBe(false));
645+
expect(result.current.hasNextPage).toBe(true);
646+
await act(async () => {
647+
await result.current.fetchNextPage();
648+
});
649+
await waitFor(() => expect(result.current.hasNextPage).toBe(false));
650+
expect(result.current.data?.data).toEqual([{ message: 'only row' }]);
651+
expect(
652+
jest
653+
.mocked(renderChartConfig)
654+
.mock.calls.map(([config]) =>
655+
isBuilderChartConfig(config) ? config.limit : undefined,
656+
),
657+
).toEqual([
658+
{ limit: 100, offset: 0 },
659+
{ limit: 100, offset: 1 },
660+
]);
661+
},
662+
);
663+
664+
it.each([
665+
{ limit: undefined, orderBy: 'message DESC' },
666+
{ limit: 0, orderBy: 'Timestamp DESC' },
667+
{ limit: 0, orderBy: 'Timestamp ASC' },
668+
])(
669+
'does not offset page with limit $limit when settings are unknown ($orderBy)',
670+
async ({ limit, orderBy }) => {
671+
const config = createMockChartConfig({
672+
orderBy,
673+
limit: { limit, offset: 0 },
674+
});
675+
mockMetadata.getSettings.mockResolvedValue(new Map());
676+
mockReader.read
677+
.mockResolvedValueOnce({
678+
done: false,
679+
value: [
680+
{ json: () => ['message'] },
681+
{ json: () => ['String'] },
682+
...(limit === 0 ? [] : [{ json: () => ['only row'] }]),
683+
],
684+
})
685+
.mockResolvedValueOnce({ done: true });
686+
const { result } = renderHook(() => useOffsetPaginatedQuery(config), {
687+
wrapper,
688+
});
689+
await waitFor(() => expect(result.current.isLoading).toBe(false));
690+
expect(result.current.hasNextPage).toBe(false);
691+
expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1);
692+
expect(mockMetadata.getSettings).not.toHaveBeenCalled();
693+
},
694+
);
695+
397696
it.each([
398697
{ orderBy: 'Timestamp DESC', limit: 100 },
399698
{ orderBy: 'Timestamp ASC', limit: 100 },

0 commit comments

Comments
 (0)