From 4696fae949cd9592cb1767589df05bc894649d9c Mon Sep 17 00:00:00 2001 From: Karl Power Date: Tue, 3 Nov 2026 15:06:28 +0100 Subject: [PATCH] feat(app): fall back to server MergeTree settings for block columns --- .../merge-tree-settings-block-columns.md | 6 + packages/app/src/components/DBRowTable.tsx | 78 +++++++-- .../components/__tests__/DBRowTable.test.tsx | 156 +++++++++++++++++- .../src/hooks/__tests__/useMetadata.test.tsx | 57 +++++++ packages/app/src/hooks/useMetadata.tsx | 21 +++ .../src/__tests__/metadata.test.ts | 55 ++++++ packages/common-utils/src/core/metadata.ts | 31 ++++ 7 files changed, 386 insertions(+), 18 deletions(-) create mode 100644 .changeset/merge-tree-settings-block-columns.md diff --git a/.changeset/merge-tree-settings-block-columns.md b/.changeset/merge-tree-settings-block-columns.md new file mode 100644 index 0000000000..766428e3b6 --- /dev/null +++ b/.changeset/merge-tree-settings-block-columns.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/app': patch +'@hyperdx/common-utils': patch +--- + +feat: fall back to server MergeTree settings to detect persisted `_block_number` and `_block_offset` columns when the table's CREATE query omits them diff --git a/packages/app/src/components/DBRowTable.tsx b/packages/app/src/components/DBRowTable.tsx index 587b796c47..9f81aac672 100644 --- a/packages/app/src/components/DBRowTable.tsx +++ b/packages/app/src/components/DBRowTable.tsx @@ -78,7 +78,11 @@ import { useRenderedSqlChartConfig, } from '@/hooks/useChartConfig'; import { CsvColumn, useCsvExport } from '@/hooks/useCsvExport'; -import { useColumns, useTableMetadata } from '@/hooks/useMetadata'; +import { + useColumns, + useMergeTreeSettings, + useTableMetadata, +} from '@/hooks/useMetadata'; import useOffsetPaginatedQuery from '@/hooks/useOffsetPaginatedQuery'; import { useGroupedPatterns } from '@/hooks/usePatterns'; import { useRowSelection } from '@/hooks/useRowSelection'; @@ -1535,15 +1539,36 @@ export function appendSelectWithAdditionalKeys( // (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; +const BLOCK_COLUMN_SETTINGS = [ + { column: '_block_number', setting: 'enable_block_number_column' }, + { column: '_block_offset', setting: 'enable_block_offset_column' }, +]; + +// engine_full ends with the SETTINGS clause. +function getEngineSetting(engineFull: string, name: string) { + const settingsClause = engineFull.match(/ SETTINGS (.*)$/)?.[1] ?? ''; + return splitAndTrimWithBracket(settingsClause) + .map(setting => setting.split(' = ')) + .find(([key]) => key === name)?.[1]; +} + +function omitsBlockColumnSetting(engineFull: string) { + return BLOCK_COLUMN_SETTINGS.some( + ({ setting }) => getEngineSetting(engineFull, setting) == null, + ); +} + +// Falls back to server values for omitted settings; pass them for MergeTree. +export function getPersistedBlockColumns( + engineFull: string, + serverMergeTreeSettings?: Map | null, +): string[] { + return BLOCK_COLUMN_SETTINGS.filter(({ setting }) => { + const value = + getEngineSetting(engineFull, setting) ?? + serverMergeTreeSettings?.get(setting); + return value === '1' || value === 'true'; + }).map(({ column }) => column); } function getSelectLength(select: SelectList): number { @@ -1575,11 +1600,27 @@ export function useConfigWithAdditionalSelect( { enabled: !!sourceId }, ); + const engineFull = tableMetadata?.engine_full ?? ''; + const needsMergeTreeSettings = + !!sourceId && + !!tableMetadata?.engine?.includes('MergeTree') && + omitsBlockColumnSetting(engineFull); + + const { data: mergeTreeSettings, isLoading: isMergeTreeSettingsLoading } = + useMergeTreeSettings(config.connection, { + enabled: needsMergeTreeSettings && !!config.connection, + }); + const primaryKey = tableMetadata?.primary_key; const partitionKey = tableMetadata?.partition_key; return useMemo(() => { - if (primaryKey == null || partitionKey == null) { + // Wait for settings to avoid querying twice. + if ( + primaryKey == null || + partitionKey == null || + isMergeTreeSettingsLoading + ) { return undefined; } @@ -1588,7 +1629,8 @@ export function useConfigWithAdditionalSelect( if (sourceId) { const blockColumns = getPersistedBlockColumns( - tableMetadata?.engine_full ?? '', + engineFull, + needsMergeTreeSettings ? mergeTreeSettings : undefined, ); if (blockColumns.length > 0) { @@ -1623,7 +1665,17 @@ export function useConfigWithAdditionalSelect( : undefined; return { ...config, select, additionalKeysLength, rowKeyColumns }; - }, [primaryKey, partitionKey, config, tableMetadata, columns, sourceId]); + }, [ + primaryKey, + partitionKey, + isMergeTreeSettingsLoading, + config, + engineFull, + needsMergeTreeSettings, + columns, + mergeTreeSettings, + sourceId, + ]); } function selectColumnMapWithoutAdditionalKeys( diff --git a/packages/app/src/components/__tests__/DBRowTable.test.tsx b/packages/app/src/components/__tests__/DBRowTable.test.tsx index 61730b121e..19b18d6f0b 100644 --- a/packages/app/src/components/__tests__/DBRowTable.test.tsx +++ b/packages/app/src/components/__tests__/DBRowTable.test.tsx @@ -24,13 +24,18 @@ jest.mock('@/hooks/useMetadata', () => ({ ...jest.requireActual('@/hooks/useMetadata'), useTableMetadata: jest.fn(), useColumns: jest.fn(), + useMergeTreeSettings: jest.fn(), })); -const { useTableMetadata: mockUseTableMetadata, useColumns: mockUseColumns } = - jest.requireMock<{ - useTableMetadata: jest.Mock; - useColumns: jest.Mock; - }>('@/hooks/useMetadata'); +const { + useTableMetadata: mockUseTableMetadata, + useColumns: mockUseColumns, + useMergeTreeSettings: mockUseMergeTreeSettings, +} = jest.requireMock<{ + useTableMetadata: jest.Mock; + useColumns: jest.Mock; + useMergeTreeSettings: 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 @@ -859,6 +864,41 @@ describe('getPersistedBlockColumns', () => { expect(getPersistedBlockColumns(engine(''))).toEqual([]); expect(getPersistedBlockColumns('')).toEqual([]); }); + + it('accepts true as an enabled value', () => { + expect( + getPersistedBlockColumns(engine(', enable_block_offset_column = true')), + ).toEqual(['_block_offset']); + }); + + it('falls back to the server setting when the table does not set it', () => { + const serverSettings = new Map([ + ['enable_block_number_column', '1'], + ['enable_block_offset_column', '1'], + ]); + expect(getPersistedBlockColumns(engine(''), serverSettings)).toEqual([ + '_block_number', + '_block_offset', + ]); + expect( + getPersistedBlockColumns( + engine(', enable_block_number_column = 1'), + new Map([['enable_block_offset_column', '1']]), + ), + ).toEqual(['_block_number', '_block_offset']); + }); + + it('prefers the table setting over the server setting', () => { + expect( + getPersistedBlockColumns( + engine(', enable_block_number_column = 0'), + new Map([ + ['enable_block_number_column', '1'], + ['enable_block_offset_column', '0'], + ]), + ), + ).toEqual([]); + }); }); describe('useConfigWithAdditionalSelect', () => { @@ -873,25 +913,40 @@ describe('useConfigWithAdditionalSelect', () => { }; const mockMetadata = ({ + engine = 'MergeTree', engineFull, columns = [], + mergeTreeSettings = new Map(), + isMergeTreeSettingsLoading = false, }: { + engine?: string; engineFull: string; columns?: { name: string; type: string }[]; + mergeTreeSettings?: Map; + isMergeTreeSettingsLoading?: boolean; }) => { mockUseTableMetadata.mockReturnValue({ data: { primary_key: 'toStartOfFiveMinutes(Timestamp), ServiceName, Timestamp', partition_key: 'toDate(Timestamp)', + engine, engine_full: engineFull, }, }); mockUseColumns.mockReturnValue({ data: columns }); + // react-query never reports a disabled query as loading + mockUseMergeTreeSettings.mockImplementation( + (_connectionId: string, { enabled }: { enabled: boolean }) => ({ + data: isMergeTreeSettingsLoading ? undefined : mergeTreeSettings, + isLoading: enabled && isMergeTreeSettingsLoading, + }), + ); }; afterEach(() => { mockUseTableMetadata.mockReset(); mockUseColumns.mockReset(); + mockUseMergeTreeSettings.mockReset(); }); it('restricts the row lookup to PK + _block_offset when only the offset column is persisted', () => { @@ -984,4 +1039,95 @@ describe('useConfigWithAdditionalSelect', () => { ); expect(result.current?.rowKeyColumns).toBeUndefined(); }); + + it('uses the server MergeTree settings when the table does not set them', () => { + mockMetadata({ + engine: 'SharedMergeTree', + engineFull: + 'SharedMergeTree ORDER BY Timestamp SETTINGS index_granularity = 8192', + mergeTreeSettings: new Map([ + ['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('ignores the server MergeTree settings for non-MergeTree engines', () => { + mockMetadata({ + engine: 'Merge', + engineFull: "Merge('default', '^otel_logs')", + mergeTreeSettings: new Map([['enable_block_number_column', '1']]), + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(mockUseMergeTreeSettings).toHaveBeenCalledWith('conn', { + enabled: false, + }); + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp)', + ); + expect(result.current?.rowKeyColumns).toBeUndefined(); + }); + + it('skips the server MergeTree settings when the table sets both', () => { + mockMetadata({ + engineFull: + 'MergeTree ORDER BY Timestamp SETTINGS enable_block_number_column = 0, enable_block_offset_column = 0', + mergeTreeSettings: new Map([ + ['enable_block_number_column', '1'], + ['enable_block_offset_column', '1'], + ]), + isMergeTreeSettingsLoading: true, + }); + + const { result } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(mockUseMergeTreeSettings).toHaveBeenCalledWith('conn', { + enabled: false, + }); + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp)', + ); + }); + + it('waits for the server MergeTree settings before returning a config', () => { + mockMetadata({ + engineFull: 'MergeTree ORDER BY Timestamp', + isMergeTreeSettingsLoading: true, + }); + + const { result, rerender } = renderHook(() => + useConfigWithAdditionalSelect(config, 'source-id'), + ); + + expect(mockUseMergeTreeSettings).toHaveBeenCalledWith('conn', { + enabled: true, + }); + expect(result.current).toBeUndefined(); + + mockMetadata({ + engineFull: 'MergeTree ORDER BY Timestamp', + mergeTreeSettings: new Map([['enable_block_offset_column', '1']]), + }); + rerender(); + + expect(result.current?.select).toBe( + 'Timestamp,ServiceName,Body,toDate(Timestamp),toStartOfFiveMinutes(Timestamp),_block_offset', + ); + }); }); diff --git a/packages/app/src/hooks/__tests__/useMetadata.test.tsx b/packages/app/src/hooks/__tests__/useMetadata.test.tsx index 1078a8320f..58fd549a08 100644 --- a/packages/app/src/hooks/__tests__/useMetadata.test.tsx +++ b/packages/app/src/hooks/__tests__/useMetadata.test.tsx @@ -18,6 +18,7 @@ import { deduplicate2dArray, intersect2dArray, useGetKeyValues, + useMergeTreeSettings, useMultipleAllFields, useMultipleGetKeyValues, } from '@/hooks/useMetadata'; @@ -948,6 +949,62 @@ describe('useMultipleAllFields', () => { }); }); +describe('useMergeTreeSettings', () => { + let wrapper: React.ComponentType<{ children: any }>; + let mockMetadata: Metadata; + const settings = new Map([['enable_block_number_column', '1']]); + + beforeEach(() => { + jest.clearAllMocks(); + mockMetadata = createMockMetadata(); + jest.spyOn(metadataModule, 'getMetadata').mockReturnValue(mockMetadata); + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + wrapper = ({ children }) => ( + {children} + ); + }); + + it('keeps a successful result fresh across mounts', async () => { + const spy = jest + .spyOn(mockMetadata, 'getMergeTreeSettings') + .mockResolvedValue(settings); + + const first = renderHook(() => useMergeTreeSettings('conn1'), { wrapper }); + await waitFor(() => expect(first.result.current.data).toEqual(settings)); + first.unmount(); + + const second = renderHook(() => useMergeTreeSettings('conn1'), { wrapper }); + expect(second.result.current.data).toEqual(settings); + expect(spy).toHaveBeenCalledTimes(1); + }); + + it('returns null on failure and refetches on the next mount', async () => { + const spy = jest + .spyOn(mockMetadata, 'getMergeTreeSettings') + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(settings); + + const first = renderHook(() => useMergeTreeSettings('conn1'), { wrapper }); + await waitFor(() => expect(first.result.current.data).toBeNull()); + first.unmount(); + + const second = renderHook(() => useMergeTreeSettings('conn1'), { wrapper }); + await waitFor(() => expect(second.result.current.data).toEqual(settings)); + expect(spy).toHaveBeenCalledTimes(2); + }); + + it('does not query without a connection', () => { + const spy = jest.spyOn(mockMetadata, 'getMergeTreeSettings'); + + const { result } = renderHook(() => useMergeTreeSettings(''), { wrapper }); + + expect(result.current.isLoading).toBe(false); + expect(spy).not.toHaveBeenCalled(); + }); +}); + describe('deduplicate2dArray', () => { // Test basic deduplication it('should remove duplicate objects across 2D array', () => { diff --git a/packages/app/src/hooks/useMetadata.tsx b/packages/app/src/hooks/useMetadata.tsx index 7ca8d72364..aca01d7c87 100644 --- a/packages/app/src/hooks/useMetadata.tsx +++ b/packages/app/src/hooks/useMetadata.tsx @@ -419,6 +419,27 @@ export function useTableMetadata( }); } +/** Fetches server MergeTree settings; null on failure. */ +export function useMergeTreeSettings( + connectionId: string, + options?: Omit< + UseQueryOptions | null, Error>, + 'queryKey' | 'queryFn' + >, +) { + const metadata = useMetadataWithSettings(); + return useQuery | null>({ + queryKey: ['useMetadata.useMergeTreeSettings', { connectionId }], + // react-query rejects undefined + queryFn: async () => + (await metadata.getMergeTreeSettings({ connectionId })) ?? null, + // Keep failures stale to refetch on next mount. + staleTime: query => (query.state.data ? 1000 * 60 * 5 : 0), + enabled: !!connectionId, + ...options, + }); +} + /** How many keys a filter values query reads, per the team's setting. */ export function useFilterKeysFetchLimit() { const { data: me } = api.useMe(); diff --git a/packages/common-utils/src/__tests__/metadata.test.ts b/packages/common-utils/src/__tests__/metadata.test.ts index dab2349b0b..395a85735e 100644 --- a/packages/common-utils/src/__tests__/metadata.test.ts +++ b/packages/common-utils/src/__tests__/metadata.test.ts @@ -546,6 +546,61 @@ describe('Metadata', () => { }); }); + describe('getMergeTreeSettings', () => { + it('returns the server MergeTree settings by name', async () => { + mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn()); + (mockClickhouseClient.query as jest.Mock).mockResolvedValue({ + json: jest.fn().mockResolvedValue({ + data: [ + { name: 'enable_block_number_column', value: '1' }, + { name: 'enable_block_offset_column', value: '0' }, + ], + }), + }); + + const result = await metadata.getMergeTreeSettings({ + connectionId: 'test_connection', + }); + + expect(result).toEqual( + new Map([ + ['enable_block_number_column', '1'], + ['enable_block_offset_column', '0'], + ]), + ); + expect(mockClickhouseClient.query).toHaveBeenCalledWith( + expect.objectContaining({ + query: 'SELECT name, value FROM system.merge_tree_settings', + }), + ); + }); + + it('re-queries after a failure instead of caching it', async () => { + const realMetadata = new Metadata( + mockClickhouseClient, + new MetadataCache(), + ); + (mockClickhouseClient.query as jest.Mock) + .mockRejectedValueOnce(new Error('Not enough privileges')) + .mockResolvedValueOnce({ + json: jest.fn().mockResolvedValue({ + data: [{ name: 'enable_block_offset_column', value: '1' }], + }), + }); + + const first = await realMetadata.getMergeTreeSettings({ + connectionId: 'test_connection', + }); + const second = await realMetadata.getMergeTreeSettings({ + connectionId: 'test_connection', + }); + + expect(first).toBeUndefined(); + expect(second).toEqual(new Map([['enable_block_offset_column', '1']])); + expect(mockClickhouseClient.query).toHaveBeenCalledTimes(2); + }); + }); + describe('getSkipIndices', () => { beforeEach(() => { mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn()); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 7471296951..ea5a1948d3 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -2204,6 +2204,37 @@ export class Metadata { ); } + /** + * Reads and caches system.merge_tree_settings per connection. Returns + * undefined on failure so the next call retries. + */ + async getMergeTreeSettings({ + connectionId, + }: { + connectionId: string; + }): Promise | undefined> { + return this.cache.getOrFetch( + `${connectionId}.mergeTreeSettings`, + async () => { + try { + const json = await this.clickhouseClient + .query<'JSON'>({ + connectionId, + query: 'SELECT name, value FROM system.merge_tree_settings', + query_params: undefined, + clickhouse_settings: this.getClickHouseSettings(), + }) + .then(res => res.json<{ name: string; value: string }>()); + + return new Map(json.data.map(row => [row.name, row.value])); + } catch (e) { + console.warn('Error fetching MergeTree settings:', e); + return undefined; + } + }, + ); + } + /** * Queries system.data_skipping_indices to retrieve skip index metadata for a table. * Results are cached using MetadataCache.