diff --git a/.changeset/row-lookup-requires-both-block-columns.md b/.changeset/row-lookup-requires-both-block-columns.md new file mode 100644 index 0000000000..bf8d093bf8 --- /dev/null +++ b/.changeset/row-lookup-requires-both-block-columns.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/app': patch +--- + +fix: only use the key-only row details lookup when the table persists both `_block_number` and `_block_offset`. A lone block column (e.g. a server-wide `enable_block_offset_column`) is not unique with a second-precision primary key and could open the wrong row; it is now only added as a tiebreaker to the full-row lookup. diff --git a/packages/app/src/components/DBRowTable.tsx b/packages/app/src/components/DBRowTable.tsx index 9f81aac672..51ed633ac2 100644 --- a/packages/app/src/components/DBRowTable.tsx +++ b/packages/app/src/components/DBRowTable.tsx @@ -1534,11 +1534,12 @@ 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. +// (the virtual versions are renumbered on merge). Only the pair identifies a +// row: _block_offset repeats across insert blocks and _block_number across the +// rows of one block, and the primary key often does not break the tie (the +// OTel exporter's default otel_logs keys on a second-precision TimestampTime). +// A lone block column is still worth adding to the full-row lookup as a +// tiebreaker for visually identical rows. const BLOCK_COLUMN_SETTINGS = [ { column: '_block_number', setting: 'enable_block_number_column' }, { column: '_block_offset', setting: 'enable_block_offset_column' }, @@ -1625,7 +1626,7 @@ export function useConfigWithAdditionalSelect( } let extraKeys: string[] = []; - let hasBlockColumns = false; + let hasBothBlockColumns = false; if (sourceId) { const blockColumns = getPersistedBlockColumns( @@ -1633,11 +1634,13 @@ export function useConfigWithAdditionalSelect( needsMergeTreeSettings ? mergeTreeSettings : undefined, ); - if (blockColumns.length > 0) { - extraKeys = blockColumns; - hasBlockColumns = true; - } else if (columns?.some(c => c.name === '__hdx_id')) { - extraKeys = ['__hdx_id']; + hasBothBlockColumns = + blockColumns.length === BLOCK_COLUMN_SETTINGS.length; + extraKeys = [...blockColumns]; + // Without a unique block pair, the full-row lookup takes every + // tiebreaker it can get. + if (!hasBothBlockColumns && columns?.some(c => c.name === '__hdx_id')) { + extraKeys.push('__hdx_id'); } } @@ -1648,13 +1651,13 @@ export function useConfigWithAdditionalSelect( extraKeys, ); - // When block columns are available, the PK + partition + block columns - // uniquely identify a row. Compute the full set of key column names + // When both block columns are available, the PK + partition + block + // columns uniquely identify a row. Compute the full set of key column names // (both raw expressions like toStartOfFiveMinutes(Timestamp) and bare // column references like Timestamp, ServiceName) so the row WHERE clause // can use only these, avoiding expensive index loading on large columns - // like Body. - const rowKeyColumns = hasBlockColumns + // like Body. Otherwise the WHERE clause uses every selected column. + const rowKeyColumns = hasBothBlockColumns ? new Set([ ...splitAndTrimWithBracket(partitionKey), ...splitAndTrimWithBracket(primaryKey), diff --git a/packages/app/src/components/__tests__/DBRowTable.test.tsx b/packages/app/src/components/__tests__/DBRowTable.test.tsx index 19b18d6f0b..b6232dbbc6 100644 --- a/packages/app/src/components/__tests__/DBRowTable.test.tsx +++ b/packages/app/src/components/__tests__/DBRowTable.test.tsx @@ -949,7 +949,7 @@ describe('useConfigWithAdditionalSelect', () => { mockUseMergeTreeSettings.mockReset(); }); - it('restricts the row lookup to PK + _block_offset when only the offset column is persisted', () => { + it('keeps the full-row lookup, with _block_offset as a tiebreaker, 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', @@ -963,22 +963,91 @@ describe('useConfigWithAdditionalSelect', () => { 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_offset', ); expect(result.current?.additionalKeysLength).toBe(3); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); + + it('keeps the full-row lookup, with _block_number as a tiebreaker, 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).toBeUndefined(); + }); + + it('restricts the row lookup to PK + 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).toEqual( new Set([ 'toDate(Timestamp)', 'toStartOfFiveMinutes(Timestamp)', 'ServiceName', 'Timestamp', + '_block_number', '_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', () => { + // The OTel exporter's default otel_logs omits both settings and keys on a + // second-precision TimestampTime, so PK + a server-enabled _block_offset is + // not unique. + it('keeps the full-row lookup when only the server enables _block_offset', () => { + mockUseTableMetadata.mockReturnValue({ + data: { + primary_key: 'ServiceName, TimestampTime', + partition_key: 'toDate(TimestampTime)', + engine: 'SharedMergeTree', + engine_full: + "SharedMergeTree('/clickhouse/tables/{uuid}/{shard}', '{replica}') PARTITION BY toDate(TimestampTime) PRIMARY KEY (ServiceName, TimestampTime) ORDER BY (ServiceName, TimestampTime, Timestamp) SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1", + }, + }); + mockUseColumns.mockReturnValue({ data: [] }); + mockUseMergeTreeSettings.mockReturnValue({ + data: new Map([ + ['enable_block_number_column', '0'], + ['enable_block_offset_column', '1'], + ]), + isLoading: false, + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(mockUseMergeTreeSettings).toHaveBeenCalledWith('conn', { + enabled: true, + }); + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,TimestampTime,toDate(TimestampTime),_block_offset', + ); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); + + it('keeps __hdx_id alongside a lone block column in the full-row lookup', () => { mockMetadata({ engineFull: - 'MergeTree ORDER BY Timestamp SETTINGS enable_block_number_column = 1', + 'MergeTree ORDER BY Timestamp SETTINGS enable_block_offset_column = 1', + columns: [{ name: '__hdx_id', type: 'String' }], }); const { result } = renderHook(() => @@ -986,16 +1055,16 @@ describe('useConfigWithAdditionalSelect', () => { ); expect(result.current?.select).toBe( - 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_number', + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_offset,__hdx_id', ); - expect(result.current?.rowKeyColumns?.has('_block_number')).toBe(true); - expect(result.current?.rowKeyColumns?.has('Body')).toBe(false); + expect(result.current?.rowKeyColumns).toBeUndefined(); }); - it('uses both block columns when both are persisted', () => { + it('does not add __hdx_id when both block columns are persisted', () => { mockMetadata({ engineFull: 'MergeTree ORDER BY Timestamp SETTINGS enable_block_number_column = 1, enable_block_offset_column = 1', + columns: [{ name: '__hdx_id', type: 'String' }], }); const { result } = renderHook(() => @@ -1005,8 +1074,7 @@ describe('useConfigWithAdditionalSelect', () => { 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); + expect(result.current?.rowKeyColumns?.has('__hdx_id')).toBe(false); }); it('falls back to __hdx_id and the full row when no block column is persisted', () => {