Skip to content

Commit 78143c8

Browse files
authored
feat(app): prefer shared MergeTree settings (#3570)
## Summary On ClickHouse Cloud, read MergeTree settings from `system.shared_merge_tree_settings`, falling back to `system.merge_tree_settings` for each setting, when detecting persisted `_block_number` and `_block_offset` columns. - `Metadata.getMergeTreeSettings` also reads `system.shared_merge_tree_settings` on Cloud (detected with `isClickHouseCloud`). Where both tables set a key, the shared value wins - Outside Cloud it still reads only `system.merge_tree_settings`, since OSS servers don't have the shared table - If the shared read fails on Cloud, it returns `undefined` (not cached), so the next call retries - Unit tests for the non-Cloud path, the Cloud per-key preference, and the Cloud failure case The code is identical to the EE change, so the forward-port should merge without conflicts. ### How to reproduce **Starting point:** a ClickHouse Cloud service where block columns are enabled in the shared settings but not in `system.merge_tree_settings`. Check with: ```sql SELECT 'merge_tree' AS t, name, value FROM system.merge_tree_settings WHERE name LIKE 'enable_block_%' UNION ALL SELECT 'shared', name, value FROM system.shared_merge_tree_settings WHERE name LIKE 'enable_block_%' ``` Add a log table on that service as a source, with no block-column settings in its `SETTINGS` clause, and ingest some rows. **On `main`:** 1. Open the source in search and click a row. 2. In the ClickHouse query log, the row lookup doesn't use `_block_number` / `_block_offset` and falls back to matching the full row. **On this branch:** 1. Repeat the steps above. 2. The row lookup now selects and filters on `_block_number` and `_block_offset`. ### How to test on Vercel preview N/A — non-UI change. ### References - Linear Issue: O11Y-29693 - Related PRs: backport of DeploySentinel/hyperdx-ee#3925, follows #3530
1 parent 3a412e4 commit 78143c8

3 files changed

Lines changed: 109 additions & 20 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@hyperdx/app': patch
3+
'@hyperdx/common-utils': patch
4+
---
5+
6+
feat: on ClickHouse Cloud, read `system.shared_merge_tree_settings` when detecting `_block_number` and `_block_offset` columns, falling back to `system.merge_tree_settings` per key

‎packages/common-utils/src/__tests__/metadata.test.ts‎

Lines changed: 75 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -547,15 +547,35 @@ describe('Metadata', () => {
547547
});
548548

549549
describe('getMergeTreeSettings', () => {
550-
it('returns the server MergeTree settings by name', async () => {
551-
mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn());
552-
(mockClickhouseClient.query as jest.Mock).mockResolvedValue({
553-
json: jest.fn().mockResolvedValue({
554-
data: [
555-
{ name: 'enable_block_number_column', value: '1' },
556-
{ name: 'enable_block_offset_column', value: '0' },
557-
],
550+
type SettingRow = { name: string; value: string };
551+
const mockSettingsTables = ({
552+
mergeTree,
553+
sharedMergeTree = [],
554+
}: {
555+
mergeTree: SettingRow[];
556+
sharedMergeTree?: SettingRow[];
557+
}) =>
558+
(mockClickhouseClient.query as jest.Mock).mockImplementation(
559+
async ({ query }: { query: string }) => ({
560+
json: async () => ({
561+
data: query.includes('shared_merge_tree_settings')
562+
? sharedMergeTree
563+
: mergeTree,
564+
}),
558565
}),
566+
);
567+
568+
beforeEach(() => {
569+
mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn());
570+
});
571+
572+
it('reads only merge_tree_settings outside Cloud', async () => {
573+
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(false);
574+
mockSettingsTables({
575+
mergeTree: [
576+
{ name: 'enable_block_number_column', value: '1' },
577+
{ name: 'enable_block_offset_column', value: '0' },
578+
],
559579
});
560580

561581
const result = await metadata.getMergeTreeSettings({
@@ -568,18 +588,65 @@ describe('Metadata', () => {
568588
['enable_block_offset_column', '0'],
569589
]),
570590
);
591+
expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1);
571592
expect(mockClickhouseClient.query).toHaveBeenCalledWith(
572593
expect.objectContaining({
573594
query: 'SELECT name, value FROM system.merge_tree_settings',
574595
}),
575596
);
576597
});
577598

599+
it('prefers shared_merge_tree_settings per key on Cloud', async () => {
600+
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(true);
601+
mockSettingsTables({
602+
mergeTree: [
603+
{ name: 'enable_block_number_column', value: '0' },
604+
{ name: 'enable_block_offset_column', value: '1' },
605+
],
606+
sharedMergeTree: [{ name: 'enable_block_number_column', value: '1' }],
607+
});
608+
609+
const result = await metadata.getMergeTreeSettings({
610+
connectionId: 'test_connection',
611+
});
612+
613+
expect(result).toEqual(
614+
new Map([
615+
['enable_block_number_column', '1'],
616+
['enable_block_offset_column', '1'],
617+
]),
618+
);
619+
expect(mockClickhouseClient.query).toHaveBeenCalledWith(
620+
expect.objectContaining({
621+
query: 'SELECT name, value FROM system.shared_merge_tree_settings',
622+
}),
623+
);
624+
});
625+
626+
it('returns undefined when the shared read fails on Cloud', async () => {
627+
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(true);
628+
(mockClickhouseClient.query as jest.Mock).mockImplementation(
629+
async ({ query }: { query: string }) => {
630+
if (query.includes('shared_merge_tree_settings')) {
631+
throw new Error('timeout');
632+
}
633+
return { json: async () => ({ data: [] }) };
634+
},
635+
);
636+
637+
const result = await metadata.getMergeTreeSettings({
638+
connectionId: 'test_connection',
639+
});
640+
641+
expect(result).toBeUndefined();
642+
});
643+
578644
it('re-queries after a failure instead of caching it', async () => {
579645
const realMetadata = new Metadata(
580646
mockClickhouseClient,
581647
new MetadataCache(),
582648
);
649+
jest.spyOn(realMetadata, 'isClickHouseCloud').mockResolvedValue(false);
583650
(mockClickhouseClient.query as jest.Mock)
584651
.mockRejectedValueOnce(new Error('Not enough privileges'))
585652
.mockResolvedValueOnce({

‎packages/common-utils/src/core/metadata.ts‎

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2242,28 +2242,44 @@ export class Metadata {
22422242
}
22432243

22442244
/**
2245-
* Reads and caches system.merge_tree_settings per connection. Returns
2246-
* undefined on failure so the next call retries.
2245+
* Reads and caches MergeTree settings per connection; on Cloud,
2246+
* system.shared_merge_tree_settings overrides system.merge_tree_settings per
2247+
* key. Returns undefined on failure so the next call retries.
22472248
*/
22482249
async getMergeTreeSettings({
22492250
connectionId,
22502251
}: {
22512252
connectionId: string;
22522253
}): Promise<Map<string, string> | undefined> {
2254+
const readSettings = (query: string) =>
2255+
this.clickhouseClient
2256+
.query<'JSON'>({
2257+
connectionId,
2258+
query,
2259+
query_params: undefined,
2260+
clickhouse_settings: this.getClickHouseSettings(),
2261+
})
2262+
.then(res => res.json<{ name: string; value: string }>())
2263+
.then(json => json.data);
2264+
22532265
return this.cache.getOrFetch(
22542266
`${connectionId}.mergeTreeSettings`,
22552267
async () => {
22562268
try {
2257-
const json = await this.clickhouseClient
2258-
.query<'JSON'>({
2259-
connectionId,
2260-
query: 'SELECT name, value FROM system.merge_tree_settings',
2261-
query_params: undefined,
2262-
clickhouse_settings: this.getClickHouseSettings(),
2263-
})
2264-
.then(res => res.json<{ name: string; value: string }>());
2265-
2266-
return new Map(json.data.map(row => [row.name, row.value]));
2269+
// OSS has no shared_merge_tree_settings; querying it fails.
2270+
const isCloud = await this.isClickHouseCloud({ connectionId });
2271+
const [mergeTreeRows, sharedMergeTreeRows] = await Promise.all([
2272+
readSettings('SELECT name, value FROM system.merge_tree_settings'),
2273+
isCloud
2274+
? readSettings(
2275+
'SELECT name, value FROM system.shared_merge_tree_settings',
2276+
)
2277+
: [],
2278+
]);
2279+
2280+
// Later rows win.
2281+
const rows = [...mergeTreeRows, ...sharedMergeTreeRows];
2282+
return new Map(rows.map(row => [row.name, row.value]));
22672283
} catch (e) {
22682284
console.warn('Error fetching MergeTree settings:', e);
22692285
return undefined;

0 commit comments

Comments
 (0)