diff --git a/.changeset/row-lookup-block-offset.md b/.changeset/row-lookup-block-offset.md new file mode 100644 index 0000000000..5328e0257b --- /dev/null +++ b/.changeset/row-lookup-block-offset.md @@ -0,0 +1,8 @@ +--- +'@hyperdx/app': patch +--- + +fix: resolve row details with the primary key plus whichever of `_block_number` +/ `_block_offset` the table persists, instead of falling back to the slow +full-row lookup when only one of `enable_block_number_column` / +`enable_block_offset_column` is set diff --git a/packages/app/src/components/DBRowTable.tsx b/packages/app/src/components/DBRowTable.tsx index e25d11bc5e..3e9a176803 100644 --- a/packages/app/src/components/DBRowTable.tsx +++ b/packages/app/src/components/DBRowTable.tsx @@ -1482,6 +1482,23 @@ export function appendSelectWithAdditionalKeys( } } +// Block columns are only stable row identifiers when the table persists them +// (the virtual versions are renumbered on merge). Either one combined with the +// primary key is unique enough: two rows would need identical key values +// (incl. a nanosecond timestamp) plus the same insert block or the same offset +// within their blocks. So a table that persists only one of them still skips +// the slow full-row lookup. +export function getPersistedBlockColumns(engineFull: string): string[] { + const columns: string[] = []; + if (engineFull.includes('enable_block_number_column = 1')) { + columns.push('_block_number'); + } + if (engineFull.includes('enable_block_offset_column = 1')) { + columns.push('_block_offset'); + } + return columns; +} + function getSelectLength(select: SelectList): number { if (typeof select === 'string') { return select.split(',').filter(s => s.trim().length > 0).length; @@ -1520,16 +1537,16 @@ export function useConfigWithAdditionalSelect( } let extraKeys: string[] = []; + let hasBlockColumns = false; if (sourceId) { - const engineFull = tableMetadata?.engine_full ?? ''; - - const hasBlockColumns = - engineFull.includes('enable_block_number_column = 1') && - engineFull.includes('enable_block_offset_column = 1'); + const blockColumns = getPersistedBlockColumns( + tableMetadata?.engine_full ?? '', + ); - if (hasBlockColumns) { - extraKeys = ['_block_number', '_block_offset']; + if (blockColumns.length > 0) { + extraKeys = blockColumns; + hasBlockColumns = true; } else if (columns?.some(c => c.name === '__hdx_id')) { extraKeys = ['__hdx_id']; } @@ -1548,10 +1565,6 @@ export function useConfigWithAdditionalSelect( // column references like Timestamp, ServiceName) so the row WHERE clause // can use only these, avoiding expensive index loading on large columns // like Body. - const hasBlockColumns = - extraKeys.includes('_block_number') && - extraKeys.includes('_block_offset'); - const rowKeyColumns = hasBlockColumns ? new Set([ ...splitAndTrimWithBracket(partitionKey), diff --git a/packages/app/src/components/__tests__/DBRowTable.test.tsx b/packages/app/src/components/__tests__/DBRowTable.test.tsx index 408fc50b2f..c4cf31d189 100644 --- a/packages/app/src/components/__tests__/DBRowTable.test.tsx +++ b/packages/app/src/components/__tests__/DBRowTable.test.tsx @@ -1,11 +1,14 @@ import { Provider } from 'jotai'; import type { ReactElement } from 'react'; -import { screen, waitFor, within } from '@testing-library/react'; +import type { BuilderChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import { renderHook, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { appendSelectWithAdditionalKeys, + getPersistedBlockColumns, RawLogTable, + useConfigWithAdditionalSelect, } from '@/components/DBRowTable'; import * as useChartConfigModule from '@/hooks/useChartConfig'; import { RowWhereResult } from '@/hooks/useRowWhere'; @@ -13,6 +16,22 @@ import { RowClickAction, UserPreferences } from '@/useUserPreferences'; const mockSetQueryState = jest.fn(); +// useConfigWithAdditionalSelect reads table metadata through react-query +// hooks that return a large UseQueryResult; the hook only reads `data`. +// Reading the handles back through `requireMock` keeps them typed as mocks +// without asserting a complete UseQueryResult. +jest.mock('@/hooks/useMetadata', () => ({ + ...jest.requireActual('@/hooks/useMetadata'), + useTableMetadata: jest.fn(), + useColumns: jest.fn(), +})); + +const { useTableMetadata: mockUseTableMetadata, useColumns: mockUseColumns } = + jest.requireMock<{ + useTableMetadata: jest.Mock; + useColumns: jest.Mock; + }>('@/hooks/useMetadata'); + // The inline expanded row reads the row query params through nuqs, which needs // a mounted Next router. Routing every setter through one mock lets tests assert // which param the expanded row's side panel button writes. @@ -774,3 +793,160 @@ describe('appendSelectWithAdditionalKeys', () => { }); }); }); + +describe('getPersistedBlockColumns', () => { + const engine = (settings: string) => + `MergeTree PARTITION BY toDate(Timestamp) ORDER BY (ServiceName, Timestamp) SETTINGS index_granularity = 8192${settings}`; + + it('returns both block columns when both are persisted', () => { + expect( + getPersistedBlockColumns( + engine( + ', enable_block_number_column = 1, enable_block_offset_column = 1', + ), + ), + ).toEqual(['_block_number', '_block_offset']); + }); + + it('returns only _block_offset when only enable_block_offset_column is set', () => { + expect( + getPersistedBlockColumns(engine(', enable_block_offset_column = 1')), + ).toEqual(['_block_offset']); + }); + + it('returns only _block_number when only enable_block_number_column is set', () => { + expect( + getPersistedBlockColumns(engine(', enable_block_number_column = 1')), + ).toEqual(['_block_number']); + }); + + it('returns nothing when neither setting is present', () => { + expect(getPersistedBlockColumns(engine(''))).toEqual([]); + expect(getPersistedBlockColumns('')).toEqual([]); + }); +}); + +describe('useConfigWithAdditionalSelect', () => { + const config: BuilderChartConfigWithDateRange = { + connection: 'conn', + from: { databaseName: 'default', tableName: 'otel_logs' }, + select: 'Timestamp, ServiceName, Body', + where: '', + whereLanguage: 'sql', + timestampValueExpression: 'Timestamp', + dateRange: [new Date(0), new Date(1)], + }; + + const mockMetadata = ({ + engineFull, + columns = [], + }: { + engineFull: string; + columns?: { name: string; type: string }[]; + }) => { + mockUseTableMetadata.mockReturnValue({ + data: { + primary_key: 'toStartOfFiveMinutes(Timestamp), ServiceName, Timestamp', + partition_key: 'toDate(Timestamp)', + engine_full: engineFull, + }, + }); + mockUseColumns.mockReturnValue({ data: columns }); + }; + + afterEach(() => { + mockUseTableMetadata.mockReset(); + mockUseColumns.mockReset(); + }); + + it('restricts the row lookup to PK + _block_offset when only the offset column is persisted', () => { + mockMetadata({ + engineFull: + 'MergeTree PARTITION BY toDate(Timestamp) ORDER BY (toStartOfFiveMinutes(Timestamp), ServiceName, Timestamp) SETTINGS index_granularity = 8192, enable_block_offset_column = 1', + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_offset', + ); + expect(result.current?.additionalKeysLength).toBe(3); + expect(result.current?.rowKeyColumns).toEqual( + new Set([ + 'toDate(Timestamp)', + 'toStartOfFiveMinutes(Timestamp)', + 'ServiceName', + 'Timestamp', + '_block_offset', + ]), + ); + expect(result.current?.rowKeyColumns?.has('Body')).toBe(false); + }); + + it('restricts the row lookup to PK + _block_number when only the number column is persisted', () => { + mockMetadata({ + engineFull: + 'MergeTree ORDER BY Timestamp SETTINGS enable_block_number_column = 1', + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_number', + ); + expect(result.current?.rowKeyColumns?.has('_block_number')).toBe(true); + expect(result.current?.rowKeyColumns?.has('Body')).toBe(false); + }); + + it('uses both block columns when both are persisted', () => { + mockMetadata({ + engineFull: + 'MergeTree ORDER BY Timestamp SETTINGS enable_block_number_column = 1, enable_block_offset_column = 1', + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_number,_block_offset', + ); + expect(result.current?.rowKeyColumns?.has('_block_number')).toBe(true); + expect(result.current?.rowKeyColumns?.has('_block_offset')).toBe(true); + }); + + it('falls back to __hdx_id and the full row when no block column is persisted', () => { + mockMetadata({ + engineFull: + 'MergeTree ORDER BY Timestamp SETTINGS index_granularity = 8192', + columns: [{ name: '__hdx_id', type: 'String' }], + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),__hdx_id', + ); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); + + it('ignores block columns for aggregate queries without a sourceId', () => { + mockMetadata({ + engineFull: + 'MergeTree ORDER BY Timestamp SETTINGS enable_block_offset_column = 1', + }); + + const { result } = renderHook(() => useConfigWithAdditionalSelect(config)); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp)', + ); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); +});