fix(app): require both block columns for the key-only row details lookup

.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.

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,19 +1626,21 @@ export function useConfigWithAdditionalSelect(
     }
 
     let extraKeys: string[] = [];
-    let hasBlockColumns = false;
+    let hasBothBlockColumns = false;
 
     if (sourceId) {
       const blockColumns = getPersistedBlockColumns(
         engineFull,
         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),

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,39 +963,108 @@ 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(() =>
       useConfigWithAdditionalSelect(config, 'source-id'),
     );
 
     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', () => {