From 450277733388369205507cff271779333b5fe34b Mon Sep 17 00:00:00 2001 From: Karl Power Date: Thu, 5 Nov 2026 11:15:01 +0100 Subject: [PATCH 1/2] fix(app): require both block columns for the key-only row details lookup A single persisted block column combined with the primary key does not identify a row: _block_offset repeats across insert blocks, _block_number across the rows of one block, and exporter-default tables key on a second-precision TimestampTime. Since #3530 the block columns are also detected from server-level merge_tree_settings, so any table without explicit settings took the key-only path on servers that enable only one of them and could open the wrong row. Use the key-only lookup only when both block columns are persisted; a lone block column is still appended to the full-row lookup as a tiebreaker. --- .../row-lookup-requires-both-block-columns.md | 5 ++ packages/app/src/components/DBRowTable.tsx | 24 ++++--- .../components/__tests__/DBRowTable.test.tsx | 68 ++++++++++++++----- 3 files changed, 69 insertions(+), 28 deletions(-) create mode 100644 .changeset/row-lookup-requires-both-block-columns.md 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..d20a7e6893 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( @@ -1635,7 +1636,8 @@ export function useConfigWithAdditionalSelect( if (blockColumns.length > 0) { extraKeys = blockColumns; - hasBlockColumns = true; + hasBothBlockColumns = + blockColumns.length === BLOCK_COLUMN_SETTINGS.length; } else if (columns?.some(c => c.name === '__hdx_id')) { extraKeys = ['__hdx_id']; } @@ -1648,13 +1650,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..747966ce56 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,19 +963,10 @@ describe('useConfigWithAdditionalSelect', () => { '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); + expect(result.current?.rowKeyColumns).toBeUndefined(); }); - it('restricts the row lookup to PK + _block_number when only the number column is persisted', () => { + 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', @@ -988,11 +979,10 @@ describe('useConfigWithAdditionalSelect', () => { 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); + expect(result.current?.rowKeyColumns).toBeUndefined(); }); - it('uses both block columns when both are persisted', () => { + 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', @@ -1005,8 +995,52 @@ 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).toEqual( + new Set([ + 'toDate(Timestamp)', + 'toStartOfFiveMinutes(Timestamp)', + 'ServiceName', + 'Timestamp', + '_block_number', + '_block_offset', + ]), + ); + expect(result.current?.rowKeyColumns?.has('Body')).toBe(false); + }); + + // 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('falls back to __hdx_id and the full row when no block column is persisted', () => { From 02e09c8d9973d9f7c5937cdb8507448befba76bd Mon Sep 17 00:00:00 2001 From: Karl Power Date: Thu, 5 Nov 2026 11:51:47 +0100 Subject: [PATCH 2/2] fix(app): keep __hdx_id as a tiebreaker when only one block column is persisted --- packages/app/src/components/DBRowTable.tsx | 13 +++---- .../components/__tests__/DBRowTable.test.tsx | 34 +++++++++++++++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/packages/app/src/components/DBRowTable.tsx b/packages/app/src/components/DBRowTable.tsx index d20a7e6893..51ed633ac2 100644 --- a/packages/app/src/components/DBRowTable.tsx +++ b/packages/app/src/components/DBRowTable.tsx @@ -1634,12 +1634,13 @@ export function useConfigWithAdditionalSelect( needsMergeTreeSettings ? mergeTreeSettings : undefined, ); - if (blockColumns.length > 0) { - extraKeys = blockColumns; - hasBothBlockColumns = - blockColumns.length === BLOCK_COLUMN_SETTINGS.length; - } 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'); } } diff --git a/packages/app/src/components/__tests__/DBRowTable.test.tsx b/packages/app/src/components/__tests__/DBRowTable.test.tsx index 747966ce56..b6232dbbc6 100644 --- a/packages/app/src/components/__tests__/DBRowTable.test.tsx +++ b/packages/app/src/components/__tests__/DBRowTable.test.tsx @@ -1043,6 +1043,40 @@ describe('useConfigWithAdditionalSelect', () => { 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_offset_column = 1', + 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),_block_offset,__hdx_id', + ); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); + + 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(() => + 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('__hdx_id')).toBe(false); + }); + it('falls back to __hdx_id and the full row when no block column is persisted', () => { mockMetadata({ engineFull: