From e63c565a0b6cb8e13c0f3c0bbe9df17e024e57f9 Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Tue, 16 Sep 2025 14:34:09 -0400 Subject: [PATCH 1/2] fix: Fix ascending order in windowed searches --- .changeset/smooth-schools-ring.md | 6 + packages/app/src/components/DBRowTable.tsx | 7 +- .../useOffsetPaginatedQuery.test.tsx | 84 +++++++- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 59 +++++- .../common-utils/src/__tests__/utils.test.ts | 194 +++++++++++++++++- packages/common-utils/src/utils.ts | 47 ++++- 6 files changed, 388 insertions(+), 9 deletions(-) create mode 100644 .changeset/smooth-schools-ring.md diff --git a/.changeset/smooth-schools-ring.md b/.changeset/smooth-schools-ring.md new file mode 100644 index 000000000..2e62003a3 --- /dev/null +++ b/.changeset/smooth-schools-ring.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/common-utils": patch +"@hyperdx/app": patch +--- + +fix: Fix ascending order in windowed searches diff --git a/packages/app/src/components/DBRowTable.tsx b/packages/app/src/components/DBRowTable.tsx index 74cc250ca..87d1d8102 100644 --- a/packages/app/src/components/DBRowTable.tsx +++ b/packages/app/src/components/DBRowTable.tsx @@ -1261,6 +1261,11 @@ function DBSqlRowTableComponent({ groupedPatterns.isLoading : isFetching; + const loadingDate = + data?.window?.direction === 'ASC' + ? data?.window?.endTime + : data?.window?.startTime; + return ( <> {denoiseResults && ( @@ -1299,7 +1304,7 @@ function DBSqlRowTableComponent({ error={error ?? undefined} columnTypeMap={columnMap} dateRange={config.dateRange} - loadingDate={data?.window?.startTime} + loadingDate={loadingDate} config={config} onChildModalOpen={onChildModalOpen} source={source} diff --git a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx index 98c4c8388..bea9a71e6 100644 --- a/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx +++ b/packages/app/src/hooks/__tests__/useOffsetPaginatedQuery.test.tsx @@ -41,7 +41,7 @@ const createMockChartConfig = ( overrides: Partial = {}, ): ChartConfigWithDateRange => ({ - timestampValueExpression: '', + timestampValueExpression: 'Timestamp', connection: 'foo', from: { databaseName: 'telemetry', @@ -55,6 +55,7 @@ const createMockChartConfig = ( limit: 100, offset: 0, }, + orderBy: 'Timestamp DESC', ...overrides, }) as ChartConfigWithDateRange; @@ -153,6 +154,87 @@ describe('useOffsetPaginatedQuery', () => { expect(result.current.data?.window.endTime).toEqual( new Date('2024-01-02T00:00:00Z'), // endDate ); + expect(result.current.data?.window.direction).toEqual('DESC'); + }); + + it('should generate correct time windows for 24-hour range with ascending sort order', async () => { + const config = createMockChartConfig({ + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-02T00:00:00Z'), + ] as [Date, Date], + orderBy: 'Timestamp ASC', + }); + + // Mock the reader to return data for first window + 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 data from the first 6-hour window (working forwards from start date) + expect(result.current.data).toBeDefined(); + expect(result.current.data?.window.windowIndex).toBe(0); + expect(result.current.data?.window.startTime).toEqual( + new Date('2024-01-01T00:00:00Z'), // startDate + ); + expect(result.current.data?.window.endTime).toEqual( + new Date('2024-01-01T06:00:00Z'), // endDate + 6h + ); + expect(result.current.data?.window.direction).toEqual('ASC'); + }); + + it('should not use time windows if first ordering is not on timestamp', async () => { + const config = createMockChartConfig({ + dateRange: [ + new Date('2024-01-01T00:00:00Z'), + new Date('2024-01-02T00:00:00Z'), + ] as [Date, Date], + orderBy: 'ServiceName', + }); + + // Mock the reader to return data for first window + 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 data from the entire range, without windowing + expect(result.current.data).toBeDefined(); + expect(result.current.data?.window.windowIndex).toBe(0); + expect(result.current.data?.window.startTime).toEqual( + new Date('2024-01-01T00:00:00Z'), // startDate + ); + expect(result.current.data?.window.endTime).toEqual( + new Date('2024-01-02T00:00:00Z'), // endDate + 6h + ); + expect(result.current.data?.window.direction).toEqual('DESC'); }); it('should handle very large time ranges with progressive bucketing', async () => { diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index f052a8e21..6b61f4348 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -8,6 +8,10 @@ import { } from '@hyperdx/common-utils/dist/clickhouse'; import { renderChartConfig } from '@hyperdx/common-utils/dist/renderChartConfig'; import { ChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { + isFirstOrderByAscending, + isTimestampExpressionInFirstOrderBy, +} from '@hyperdx/common-utils/dist/utils'; import { QueryClient, QueryFunction, @@ -45,6 +49,7 @@ type TimeWindow = { startTime: Date; endTime: Date; windowIndex: number; + direction: 'ASC' | 'DESC'; }; type TPageParam = { @@ -64,8 +69,11 @@ type TData = { pageParams: TPageParam[]; }; -// Generate time windows from date range using progressive bucketing -function generateTimeWindows(startDate: Date, endDate: Date): TimeWindow[] { +// Generate time windows from date range using progressive bucketing, starting at the end of the date range +function generateTimeWindowsDescending( + startDate: Date, + endDate: Date, +): TimeWindow[] { const windows: TimeWindow[] = []; let currentEnd = new Date(endDate); let windowIndex = 0; @@ -82,6 +90,7 @@ function generateTimeWindows(startDate: Date, endDate: Date): TimeWindow[] { endTime: new Date(currentEnd), startTime: windowStart, windowIndex, + direction: 'DESC', }); currentEnd = windowStart; @@ -91,13 +100,43 @@ function generateTimeWindows(startDate: Date, endDate: Date): TimeWindow[] { return windows; } +// Generate time windows from date range using progressive bucketing, starting at the beginning of the date range +function generateTimeWindowsAscending(startDate: Date, endDate: Date) { + const windows: TimeWindow[] = []; + let currentStart = new Date(startDate); + let windowIndex = 0; + + while (currentStart < endDate) { + const windowSize = + TIME_WINDOWS_MS[windowIndex] || + TIME_WINDOWS_MS[TIME_WINDOWS_MS.length - 1]; // use largest window size + const windowEnd = new Date( + Math.min(currentStart.getTime() + windowSize, endDate.getTime()), + ); + + windows.push({ + startTime: new Date(currentStart), + endTime: windowEnd, + windowIndex, + direction: 'ASC', + }); + + currentStart = windowEnd; + windowIndex++; + } + + return windows; +} + // Get time window from page param function getTimeWindowFromPageParam( config: ChartConfigWithDateRange, pageParam: TPageParam, ): TimeWindow { const [startDate, endDate] = config.dateRange; - const windows = generateTimeWindows(startDate, endDate); + const windows = isFirstOrderByAscending(config.orderBy) + ? generateTimeWindowsAscending(startDate, endDate) + : generateTimeWindowsDescending(startDate, endDate); const window = windows[pageParam.windowIndex]; if (window == null) { throw new Error('Invalid time window for page param'); @@ -116,7 +155,9 @@ function getNextPageParam( } const [startDate, endDate] = config.dateRange; - const windows = generateTimeWindows(startDate, endDate); + const windows = isFirstOrderByAscending(config.orderBy) + ? generateTimeWindowsAscending(startDate, endDate) + : generateTimeWindowsDescending(startDate, endDate); const currentWindow = lastPage.window; // Calculate total results from all pages in current window @@ -169,7 +210,15 @@ const queryFn: QueryFunction = async ({ const config = queryKey[1]; // Get the time window for this page - const timeWindow = getTimeWindowFromPageParam(config, pageParam); + const useWindowing = isTimestampExpressionInFirstOrderBy(config); + const timeWindow = useWindowing + ? getTimeWindowFromPageParam(config, pageParam) + : { + startTime: config.dateRange[0], + endTime: config.dateRange[1], + windowIndex: 0, + direction: 'DESC' as const, + }; // Create config with windowed date range const windowedConfig = { diff --git a/packages/common-utils/src/__tests__/utils.test.ts b/packages/common-utils/src/__tests__/utils.test.ts index d09ccc674..1137f4f56 100644 --- a/packages/common-utils/src/__tests__/utils.test.ts +++ b/packages/common-utils/src/__tests__/utils.test.ts @@ -1,4 +1,13 @@ -import { formatDate, splitAndTrimCSV, splitAndTrimWithBracket } from '../utils'; +import { ChartConfigWithDateRange } from '@/types'; + +import { + formatDate, + getFirstOrderingItem, + isFirstOrderByAscending, + isTimestampExpressionInFirstOrderBy, + splitAndTrimCSV, + splitAndTrimWithBracket, +} from '../utils'; describe('utils', () => { describe('formatDate', () => { @@ -208,5 +217,188 @@ describe('utils', () => { ]; expect(splitAndTrimWithBracket(input)).toEqual(expected); }); + + it('should handle order-by clauses with order directions', () => { + const input = 'toDate(Timestamp) ASC, Time ASC, ServiceName DESC'; + const expected = [ + 'toDate(Timestamp) ASC', + 'Time ASC', + 'ServiceName DESC', + ]; + expect(splitAndTrimWithBracket(input)).toEqual(expected); + }); + }); + + describe('getFirstOrderingItem', () => { + it('should return undefined for undefined input', () => { + expect(getFirstOrderingItem(undefined)).toBeUndefined(); + }); + + it('should return the first column name for a single column string input', () => { + expect(getFirstOrderingItem('column1 DESC')).toBe('column1 DESC'); + }); + + it('should return the first column name for a simple string input', () => { + expect(getFirstOrderingItem('column1, column2 DESC, column3 ASC')).toBe( + 'column1', + ); + }); + + it('should return the first column name for a simple string input', () => { + expect( + getFirstOrderingItem('column1 ASC, column2 DESC, column3 ASC'), + ).toBe('column1 ASC'); + }); + + it('should return the first column name for an array of objects input', () => { + const orderBy: Exclude = [ + { valueExpression: 'column1', ordering: 'ASC' }, + { valueExpression: 'column2', ordering: 'ASC' }, + ]; + expect(getFirstOrderingItem(orderBy)).toEqual({ + valueExpression: 'column1', + ordering: 'ASC', + }); + }); + }); + + describe('isFirstOrderingOnTimestampExpression', () => { + it('should return false if no orderBy is provided', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: undefined, + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(false); + }); + + it('should return false if the first ordering column is not in the timestampValueExpression', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: 'ServiceName', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(false); + }); + + it('should return false if the second ordering column is in the timestampValueExpression but the first is not', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: 'ServiceName ASC, Timestamp', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(false); + }); + + it('should return true if the first ordering column is the timestampValueExpression', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: 'Timestamp', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + + it('should return true if the first ordering column is a string and has a direction', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: 'Timestamp DESC, ServiceName', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + + it('should return true if the first ordering column is a string and has a lowercase direction', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: 'Timestamp desc, ServiceName', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + + it('should return true if the first ordering column is an object and is in the timestampValueExpression', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: [ + { valueExpression: 'Timestamp', ordering: 'ASC' }, + { valueExpression: 'ServiceName', ordering: 'ASC' }, + ], + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + + it('should support toStartOf() timestampValueExpressions', () => { + const config = { + timestampValueExpression: 'toStartOfDay(Timestamp), Timestamp', + orderBy: '(toStartOfDay(Timestamp)) DESC, Timestamp', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + + it('should support toStartOf() timestampValueExpressions in tuples', () => { + const config = { + timestampValueExpression: 'toStartOfDay(Timestamp), Timestamp', + orderBy: '(toStartOfHour(TimestampTime), TimestampTime) DESC', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); + }); + + describe('isFirstOrderingAscending', () => { + it('should return true for ascending order in string input', () => { + expect(isFirstOrderByAscending('column1 ASC, column2 DESC')).toBe(true); + }); + + it('should return true for lowercase, non-trimmed ascending order in string input', () => { + expect(isFirstOrderByAscending(' column1 asc , column2 DESC')).toBe(true); + }); + + it('should return true for ascending order without explicit direction in string input', () => { + expect(isFirstOrderByAscending('column1, column2 DESC')).toBe(true); + }); + + it('should return false for descending order in string input', () => { + expect(isFirstOrderByAscending('column1 DESC, column2 ASC')).toBe(false); + }); + + it('should return false for lowercase, non-trimmed descending order in string input', () => { + expect(isFirstOrderByAscending(' column1 desc , column2 ASC')).toBe( + false, + ); + }); + + it('should return true for ascending order in object input', () => { + const orderBy: Exclude = [ + { valueExpression: 'column1', ordering: 'ASC' }, + { valueExpression: 'column2', ordering: 'DESC' }, + ]; + expect(isFirstOrderByAscending(orderBy)).toBe(true); + }); + + it('should return false for descending order in object input', () => { + const orderBy: Exclude = [ + { valueExpression: 'column1', ordering: 'DESC' }, + { valueExpression: 'column2', ordering: 'ASC' }, + ]; + expect(isFirstOrderByAscending(orderBy)).toBe(false); + }); + + it('should return false if no orderBy is provided', () => { + expect(isFirstOrderByAscending(undefined)).toBe(false); + }); + + it('should support toStartOf() timestampValueExpressions in tuples', () => { + const orderBy = '(toStartOfHour(TimestampTime), TimestampTime) DESC'; + expect(isFirstOrderByAscending(orderBy)).toBe(false); + }); + + it('should support toStartOf() timestampValueExpressions in tuples', () => { + const orderBy = '(toStartOfHour(TimestampTime), TimestampTime) ASC'; + expect(isFirstOrderByAscending(orderBy)).toBe(true); + }); }); }); diff --git a/packages/common-utils/src/utils.ts b/packages/common-utils/src/utils.ts index 77983460e..c3dfd687e 100644 --- a/packages/common-utils/src/utils.ts +++ b/packages/common-utils/src/utils.ts @@ -2,7 +2,7 @@ import { add as fnsAdd, format as fnsFormat } from 'date-fns'; import { formatInTimeZone } from 'date-fns-tz'; -import type { SQLInterval } from '@/types'; +import type { ChartConfigWithDateRange, SQLInterval } from '@/types'; export const isBrowser: boolean = typeof window !== 'undefined' && typeof window.document !== 'undefined'; @@ -298,3 +298,48 @@ export const formatDate = ( ? formatInTimeZone(date, 'Etc/UTC', formatStr) : fnsFormat(date, formatStr); }; + +export const getFirstOrderingItem = ( + orderBy: ChartConfigWithDateRange['orderBy'], +) => { + if (!orderBy || orderBy.length === 0) return undefined; + + return typeof orderBy === 'string' + ? splitAndTrimWithBracket(orderBy)[0] + : orderBy[0]; +}; + +export const isTimestampExpressionInFirstOrderBy = ( + config: ChartConfigWithDateRange, +) => { + const firstOrderingItem = getFirstOrderingItem(config.orderBy); + if (!firstOrderingItem) return false; + + const firstOrderingExpression = + typeof firstOrderingItem === 'string' + ? firstOrderingItem.replace(/\s+(ASC|DESC)$/i, '') + : firstOrderingItem.valueExpression; + + const timestampValueExpressions = splitAndTrimWithBracket( + config.timestampValueExpression, + ); + + return timestampValueExpressions.some(tve => + firstOrderingExpression.includes(tve), + ); +}; + +export const isFirstOrderByAscending = ( + orderBy: ChartConfigWithDateRange['orderBy'], +): boolean => { + const primaryOrderingItem = getFirstOrderingItem(orderBy); + + if (!primaryOrderingItem) return false; + + const isDescending = + typeof primaryOrderingItem === 'string' + ? /DESC$/.test(primaryOrderingItem.trim().toUpperCase()) + : primaryOrderingItem.ordering === 'DESC'; + + return !isDescending; +}; From 06656eac23a68673c1814062e4b733d796d7e4ea Mon Sep 17 00:00:00 2001 From: Drew Davis Date: Tue, 16 Sep 2025 17:23:58 -0400 Subject: [PATCH 2/2] review: Remove regexes --- .../app/src/hooks/useOffsetPaginatedQuery.tsx | 4 ++-- .../common-utils/src/__tests__/utils.test.ts | 20 +++++++++++++++++++ packages/common-utils/src/utils.ts | 15 ++++++++++++-- 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx index 6b61f4348..b92a91f7d 100644 --- a/packages/app/src/hooks/useOffsetPaginatedQuery.tsx +++ b/packages/app/src/hooks/useOffsetPaginatedQuery.tsx @@ -210,8 +210,8 @@ const queryFn: QueryFunction = async ({ const config = queryKey[1]; // Get the time window for this page - const useWindowing = isTimestampExpressionInFirstOrderBy(config); - const timeWindow = useWindowing + const shouldUseWindowing = isTimestampExpressionInFirstOrderBy(config); + const timeWindow = shouldUseWindowing ? getTimeWindowFromPageParam(config, pageParam) : { startTime: config.dateRange[0], diff --git a/packages/common-utils/src/__tests__/utils.test.ts b/packages/common-utils/src/__tests__/utils.test.ts index 1137f4f56..5bc065f8f 100644 --- a/packages/common-utils/src/__tests__/utils.test.ts +++ b/packages/common-utils/src/__tests__/utils.test.ts @@ -5,6 +5,7 @@ import { getFirstOrderingItem, isFirstOrderByAscending, isTimestampExpressionInFirstOrderBy, + removeTrailingDirection, splitAndTrimCSV, splitAndTrimWithBracket, } from '../utils'; @@ -272,6 +273,15 @@ describe('utils', () => { expect(isTimestampExpressionInFirstOrderBy(config)).toBe(false); }); + it('should return false if empty orderBy is provided', () => { + const config = { + timestampValueExpression: 'Timestamp', + orderBy: '', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(false); + }); + it('should return false if the first ordering column is not in the timestampValueExpression', () => { const config = { timestampValueExpression: 'Timestamp', @@ -346,6 +356,16 @@ describe('utils', () => { expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); }); + + it('should support functions with multiple parameters in the order by', () => { + const config = { + timestampValueExpression: + 'toStartOfInterval(TimestampTime, INTERVAL 1 DAY)', + orderBy: 'toStartOfInterval(TimestampTime, INTERVAL 1 DAY) DESC', + } as ChartConfigWithDateRange; + + expect(isTimestampExpressionInFirstOrderBy(config)).toBe(true); + }); }); describe('isFirstOrderingAscending', () => { diff --git a/packages/common-utils/src/utils.ts b/packages/common-utils/src/utils.ts index c3dfd687e..e642c5989 100644 --- a/packages/common-utils/src/utils.ts +++ b/packages/common-utils/src/utils.ts @@ -309,6 +309,17 @@ export const getFirstOrderingItem = ( : orderBy[0]; }; +export const removeTrailingDirection = (s: string) => { + const upper = s.trim().toUpperCase(); + if (upper.endsWith('DESC')) { + return s.slice(0, upper.lastIndexOf('DESC')).trim(); + } else if (upper.endsWith('ASC')) { + return s.slice(0, upper.lastIndexOf('ASC')).trim(); + } + + return s; +}; + export const isTimestampExpressionInFirstOrderBy = ( config: ChartConfigWithDateRange, ) => { @@ -317,7 +328,7 @@ export const isTimestampExpressionInFirstOrderBy = ( const firstOrderingExpression = typeof firstOrderingItem === 'string' - ? firstOrderingItem.replace(/\s+(ASC|DESC)$/i, '') + ? removeTrailingDirection(firstOrderingItem) : firstOrderingItem.valueExpression; const timestampValueExpressions = splitAndTrimWithBracket( @@ -338,7 +349,7 @@ export const isFirstOrderByAscending = ( const isDescending = typeof primaryOrderingItem === 'string' - ? /DESC$/.test(primaryOrderingItem.trim().toUpperCase()) + ? primaryOrderingItem.trim().toUpperCase().endsWith('DESC') : primaryOrderingItem.ordering === 'DESC'; return !isDescending;