Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/shared-merge-tree-settings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@hyperdx/app': patch
'@hyperdx/common-utils': patch
---

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
83 changes: 75 additions & 8 deletions packages/common-utils/src/__tests__/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -547,15 +547,35 @@ 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' },
],
type SettingRow = { name: string; value: string };
const mockSettingsTables = ({
mergeTree,
sharedMergeTree = [],
}: {
mergeTree: SettingRow[];
sharedMergeTree?: SettingRow[];
}) =>
(mockClickhouseClient.query as jest.Mock).mockImplementation(
async ({ query }: { query: string }) => ({
json: async () => ({
data: query.includes('shared_merge_tree_settings')
? sharedMergeTree
: mergeTree,
}),
}),
);

beforeEach(() => {
mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn());
});

it('reads only merge_tree_settings outside Cloud', async () => {
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(false);
mockSettingsTables({
mergeTree: [
{ name: 'enable_block_number_column', value: '1' },
{ name: 'enable_block_offset_column', value: '0' },
],
});

const result = await metadata.getMergeTreeSettings({
Expand All @@ -568,18 +588,65 @@ describe('Metadata', () => {
['enable_block_offset_column', '0'],
]),
);
expect(mockClickhouseClient.query).toHaveBeenCalledTimes(1);
expect(mockClickhouseClient.query).toHaveBeenCalledWith(
expect.objectContaining({
query: 'SELECT name, value FROM system.merge_tree_settings',
}),
);
});

it('prefers shared_merge_tree_settings per key on Cloud', async () => {
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(true);
mockSettingsTables({
mergeTree: [
{ name: 'enable_block_number_column', value: '0' },
{ name: 'enable_block_offset_column', value: '1' },
],
sharedMergeTree: [{ name: 'enable_block_number_column', value: '1' }],
});

const result = await metadata.getMergeTreeSettings({
connectionId: 'test_connection',
});

expect(result).toEqual(
new Map([
['enable_block_number_column', '1'],
['enable_block_offset_column', '1'],
]),
);
expect(mockClickhouseClient.query).toHaveBeenCalledWith(
expect.objectContaining({
query: 'SELECT name, value FROM system.shared_merge_tree_settings',
}),
);
});

it('returns undefined when the shared read fails on Cloud', async () => {
jest.spyOn(metadata, 'isClickHouseCloud').mockResolvedValue(true);
(mockClickhouseClient.query as jest.Mock).mockImplementation(
async ({ query }: { query: string }) => {
if (query.includes('shared_merge_tree_settings')) {
throw new Error('timeout');
}
return { json: async () => ({ data: [] }) };
},
);

const result = await metadata.getMergeTreeSettings({
connectionId: 'test_connection',
});

expect(result).toBeUndefined();
});

it('re-queries after a failure instead of caching it', async () => {
const realMetadata = new Metadata(
mockClickhouseClient,
new MetadataCache(),
);
jest.spyOn(realMetadata, 'isClickHouseCloud').mockResolvedValue(false);
(mockClickhouseClient.query as jest.Mock)
.mockRejectedValueOnce(new Error('Not enough privileges'))
.mockResolvedValueOnce({
Expand Down
40 changes: 28 additions & 12 deletions packages/common-utils/src/core/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2242,28 +2242,44 @@ export class Metadata {
}

/**
* Reads and caches system.merge_tree_settings per connection. Returns
* undefined on failure so the next call retries.
* Reads and caches MergeTree settings per connection; on Cloud,
* system.shared_merge_tree_settings overrides system.merge_tree_settings per
* key. Returns undefined on failure so the next call retries.
*/
async getMergeTreeSettings({
connectionId,
}: {
connectionId: string;
}): Promise<Map<string, string> | undefined> {
const readSettings = (query: string) =>
this.clickhouseClient
.query<'JSON'>({
connectionId,
query,
query_params: undefined,
clickhouse_settings: this.getClickHouseSettings(),
})
.then(res => res.json<{ name: string; value: string }>())
.then(json => json.data);

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]));
// OSS has no shared_merge_tree_settings; querying it fails.
const isCloud = await this.isClickHouseCloud({ connectionId });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — A failed Cloud check permanently caches settings that ignore the shared table

If the Cloud check fails once (a timeout or network blip), isClickHouseCloud (metadata.ts:2146-2173) doesn't cache the failure but returns false. getMergeTreeSettings then builds a map from merge_tree_settings alone. MetadataCache.set stores it with no TTL (metadata.ts:304-330), so a Cloud connection keeps missing the shared-table values for the life of the session. That is exactly the bug this PR is meant to fix. Fix: if detection fails, don't cache this result either. For example, add a method that returns boolean | undefined, and return undefined from getMergeTreeSettings when the result is undefined.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

const [mergeTreeRows, sharedMergeTreeRows] = await Promise.all([

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — A failed shared-table read throws away the merge_tree_settings result too

Both queries run in one Promise.all inside one try, so any error reading system.shared_merge_tree_settings makes the whole call return undefined. That includes 'Not enough privileges' for a restricted Cloud user, a case getSettings (metadata.ts:2226-2236) already handles. Block-column detection then fails, although on main it worked from merge_tree_settings. And since undefined isn't cached, every call re-runs both queries (useMergeTreeSettings uses staleTime 0 on failure). This contradicts the 'falls back to merge_tree_settings per key' claim in the changeset. Fix: catch the shared read on its own (e.g. .catch(() => [])) so the merge_tree_settings rows are still used.

Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond

readSettings('SELECT name, value FROM system.merge_tree_settings'),
isCloud
? readSettings(
'SELECT name, value FROM system.shared_merge_tree_settings',
)
: [],
]);

// Later rows win.
const rows = [...mergeTreeRows, ...sharedMergeTreeRows];
return new Map(rows.map(row => [row.name, row.value]));
} catch (e) {
console.warn('Error fetching MergeTree settings:', e);
return undefined;
Expand Down
Loading