From 09a0c79866fc9b43bef2afff3f62acdd51786461 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 18 Feb 2026 16:57:47 -0500 Subject: [PATCH 1/5] feat: add optimization settings to all queries if available --- .../common-utils/src/clickhouse/browser.ts | 12 +++-- packages/common-utils/src/clickhouse/index.ts | 52 +++++++++++++++---- packages/common-utils/src/clickhouse/node.ts | 17 ++++-- packages/common-utils/src/core/metadata.ts | 37 +++++++++++++ 4 files changed, 100 insertions(+), 18 deletions(-) diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index b56a2cb0da..1d46332853 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -5,6 +5,8 @@ import type { } from '@clickhouse/client-common'; import { createClient } from '@clickhouse/client-web'; +import { GET_SETTINGS_QUERY } from '@/core/metadata'; + import { BaseClickhouseClient, ClickhouseClientOptions, @@ -114,9 +116,13 @@ export class ClickhouseClient extends BaseClickhouseClient { this.logDebugQuery(query, query_params); - const clickhouseSettings = this.processClickhouseSettings( - externalClickhouseSettings, - ); + let clickhouseSettings: ClickHouseSettings | undefined; + if (query !== GET_SETTINGS_QUERY) { + clickhouseSettings = await this.processClickhouseSettings({ + connectionId, + externalClickhouseSettings, + }); + } const httpHeaders: { [header: string]: string } = { ...(connectionId && connectionId !== 'local' diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index 8e00e74fa4..a3feecc119 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -11,7 +11,7 @@ import type { ClickHouseClient as WebClickHouseClient } from '@clickhouse/client import * as SQLParser from 'node-sql-parser'; import objectHash from 'object-hash'; -import { Metadata } from '@/core/metadata'; +import { getMetadata, Metadata } from '@/core/metadata'; import { renderChartConfig, setChartSelectsAlias, @@ -496,11 +496,15 @@ export abstract class BaseClickhouseClient { console.debug('--------------------------------------------------------'); } - protected processClickhouseSettings( - external_clickhouse_settings?: ClickHouseSettings, - ): ClickHouseSettings { + protected async processClickhouseSettings({ + connectionId, + externalClickhouseSettings, + }: { + connectionId?: string; + externalClickhouseSettings?: ClickHouseSettings; + }): Promise { const clickhouse_settings = structuredClone( - external_clickhouse_settings || {}, + externalClickhouseSettings || {}, ); if (clickhouse_settings?.max_rows_to_read && this.maxRowReadOnly) { delete clickhouse_settings['max_rows_to_read']; @@ -512,13 +516,39 @@ export abstract class BaseClickhouseClient { clickhouse_settings.max_execution_time = this.queryTimeout; } - return { - allow_experimental_analyzer: 1, - date_time_output_format: 'iso', - wait_end_of_query: 0, - cancel_http_readonly_queries_on_client_close: 1, - ...clickhouse_settings, + clickhouse_settings.allow_experimental_analyzer = 1; + clickhouse_settings.date_time_output_format = 'iso'; + clickhouse_settings.wait_end_of_query = 0; + clickhouse_settings.cancel_http_readonly_queries_on_client_close = 1; + const metadata = getMetadata(this); + const serverSettings = await metadata.getSettings({ connectionId }); + + const applySettingIfAvailable = (name: string, value: string) => { + if (!serverSettings || !serverSettings.has(name)) return; + // eslint-disable-next-line security/detect-object-injection + clickhouse_settings[name] = value; }; + + // Enables lazy materialization up to the given LIMIT + applySettingIfAvailable('query_plan_optimize_lazy_materialization', '1'); + applySettingIfAvailable( + 'query_plan_max_limit_for_lazy_materialization', + '100000', + ); + // Enables skip indexes to be used for top k style queries up to the given LIMIT + applySettingIfAvailable('use_skip_indexes_for_top_k', '1'); + applySettingIfAvailable( + 'query_plan_max_limit_for_top_k_optimization', + '100000', + ); + applySettingIfAvailable('use_top_k_dynamic_filtering', '1'); + // Enables skip indexes to be used on data read + applySettingIfAvailable('use_skip_indexes_on_data_read', '1'); + // Evaluate WHERE filters with mixed AND and OR conditions using skip indexes. + // If value is 0, then skip indicies only used on AND queries + applySettingIfAvailable('use_skip_indexes_for_disjunctions', '1'); + + return clickhouse_settings; } async query( diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index 116789e1e5..e2c27e873c 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -1,5 +1,11 @@ import { createClient } from '@clickhouse/client'; -import type { BaseResultSet, DataFormat } from '@clickhouse/client-common'; +import type { + BaseResultSet, + ClickHouseSettings, + DataFormat, +} from '@clickhouse/client-common'; + +import { GET_SETTINGS_QUERY } from '@/core/metadata'; import { BaseClickhouseClient, @@ -34,9 +40,12 @@ export class ClickhouseClient extends BaseClickhouseClient { }: QueryInputs): Promise> { this.logDebugQuery(query, query_params); - const clickhouseSettings = this.processClickhouseSettings( - externalClickhouseSettings, - ); + let clickhouseSettings: ClickHouseSettings | undefined; + if (query !== GET_SETTINGS_QUERY) { + clickhouseSettings = await this.processClickhouseSettings({ + externalClickhouseSettings, + }); + } // TODO: Custom error handling // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- client library type mismatch diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index fd61080593..539fc4dbd3 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -660,6 +660,41 @@ export class Metadata { }); } + async getSettings({ connectionId }: { connectionId?: string }) { + return this.cache.getOrFetch( + `${connectionId}.availableSettings`, + async () => { + try { + // we must use client here to avoid an infinite + const json = await this.clickhouseClient + .query<'JSON'>({ + connectionId, + query: GET_SETTINGS_QUERY, + 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) { + // Don't retry permissions errors, just silently return undefined + if ( + e instanceof Error && + e.message.includes('Not enough privileges') + ) { + console.warn( + 'Not enough privileges to fetch settings, may result in unoptimized queries:', + e, + ); + return undefined; + } + + throw e; + } + }, + ); + } + /** * Queries system.data_skipping_indices to retrieve skip index metadata for a table. * Results are cached using MetadataCache. @@ -1195,6 +1230,8 @@ export function tcFromSource(source?: TSource): TableConnection { }; } +export const GET_SETTINGS_QUERY = 'SELECT name, value FROM system.settings'; + const __LOCAL_CACHE__ = new MetadataCache(); // TODO: better to init the Metadata object on the client side From 231cf2c3db4a8af15f05c267a7d89fcd6ac0f223 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 18 Feb 2026 17:22:21 -0500 Subject: [PATCH 2/5] add changeset --- .changeset/rotten-squids-destroy.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/rotten-squids-destroy.md diff --git a/.changeset/rotten-squids-destroy.md b/.changeset/rotten-squids-destroy.md new file mode 100644 index 0000000000..553b83e469 --- /dev/null +++ b/.changeset/rotten-squids-destroy.md @@ -0,0 +1,7 @@ +--- +"@hyperdx/common-utils": minor +"@hyperdx/api": minor +"@hyperdx/app": minor +--- + +feat: use optimization settings if available for use in CH From 25b467089e5df6215157a686298bd4ebdf4ca8af Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Wed, 18 Feb 2026 17:30:21 -0500 Subject: [PATCH 3/5] pr improvements --- .../common-utils/src/clickhouse/browser.ts | 1 + packages/common-utils/src/clickhouse/index.ts | 18 ++++++++++++------ packages/common-utils/src/clickhouse/node.ts | 1 + packages/common-utils/src/core/metadata.ts | 3 +-- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 1d46332853..7685fa18a8 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -117,6 +117,7 @@ export class ClickhouseClient extends BaseClickhouseClient { this.logDebugQuery(query, query_params); let clickhouseSettings: ClickHouseSettings | undefined; + // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse if (query !== GET_SETTINGS_QUERY) { clickhouseSettings = await this.processClickhouseSettings({ connectionId, diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index a3feecc119..56b63d279f 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -516,17 +516,20 @@ export abstract class BaseClickhouseClient { clickhouse_settings.max_execution_time = this.queryTimeout; } - clickhouse_settings.allow_experimental_analyzer = 1; - clickhouse_settings.date_time_output_format = 'iso'; - clickhouse_settings.wait_end_of_query = 0; - clickhouse_settings.cancel_http_readonly_queries_on_client_close = 1; + const defaultSettings: ClickHouseSettings = { + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + }; + const metadata = getMetadata(this); const serverSettings = await metadata.getSettings({ connectionId }); const applySettingIfAvailable = (name: string, value: string) => { if (!serverSettings || !serverSettings.has(name)) return; // eslint-disable-next-line security/detect-object-injection - clickhouse_settings[name] = value; + defaultSettings[name] = value; }; // Enables lazy materialization up to the given LIMIT @@ -548,7 +551,10 @@ export abstract class BaseClickhouseClient { // If value is 0, then skip indicies only used on AND queries applySettingIfAvailable('use_skip_indexes_for_disjunctions', '1'); - return clickhouse_settings; + return { + ...defaultSettings, + ...clickhouse_settings, + }; } async query( diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index e2c27e873c..e538913630 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -41,6 +41,7 @@ export class ClickhouseClient extends BaseClickhouseClient { this.logDebugQuery(query, query_params); let clickhouseSettings: ClickHouseSettings | undefined; + // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse if (query !== GET_SETTINGS_QUERY) { clickhouseSettings = await this.processClickhouseSettings({ externalClickhouseSettings, diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 539fc4dbd3..841e1651b8 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -665,7 +665,6 @@ export class Metadata { `${connectionId}.availableSettings`, async () => { try { - // we must use client here to avoid an infinite const json = await this.clickhouseClient .query<'JSON'>({ connectionId, @@ -686,7 +685,7 @@ export class Metadata { 'Not enough privileges to fetch settings, may result in unoptimized queries:', e, ); - return undefined; + return new Map(); } throw e; From 0e052f61156c4a6f3a73a8be7708accbbe12a046 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Thu, 19 Feb 2026 09:54:21 -0500 Subject: [PATCH 4/5] fix: boolean flag for recursion guard --- packages/common-utils/src/clickhouse/browser.ts | 5 ++--- packages/common-utils/src/clickhouse/index.ts | 1 + packages/common-utils/src/clickhouse/node.ts | 5 ++--- packages/common-utils/src/core/metadata.ts | 13 ++++--------- 4 files changed, 9 insertions(+), 15 deletions(-) diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 7685fa18a8..5a6233545d 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -5,8 +5,6 @@ import type { } from '@clickhouse/client-common'; import { createClient } from '@clickhouse/client-web'; -import { GET_SETTINGS_QUERY } from '@/core/metadata'; - import { BaseClickhouseClient, ClickhouseClientOptions, @@ -107,6 +105,7 @@ export class ClickhouseClient extends BaseClickhouseClient { clickhouse_settings: externalClickhouseSettings, connectionId, queryId, + shouldSkipApplySettings, }: QueryInputs): Promise> { // FIXME: we couldn't initialize the client in the constructor // since the window is not avalible @@ -118,7 +117,7 @@ export class ClickhouseClient extends BaseClickhouseClient { let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse - if (query !== GET_SETTINGS_QUERY) { + if (shouldSkipApplySettings) { clickhouseSettings = await this.processClickhouseSettings({ connectionId, externalClickhouseSettings, diff --git a/packages/common-utils/src/clickhouse/index.ts b/packages/common-utils/src/clickhouse/index.ts index 56b63d279f..bfb14463ca 100644 --- a/packages/common-utils/src/clickhouse/index.ts +++ b/packages/common-utils/src/clickhouse/index.ts @@ -422,6 +422,7 @@ export interface QueryInputs { clickhouse_settings?: ClickHouseSettings; connectionId?: string; queryId?: string; + shouldSkipApplySettings?: boolean; } export type ClickhouseClientOptions = { diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index e538913630..3300782de5 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -5,8 +5,6 @@ import type { DataFormat, } from '@clickhouse/client-common'; -import { GET_SETTINGS_QUERY } from '@/core/metadata'; - import { BaseClickhouseClient, ClickhouseClientOptions, @@ -37,12 +35,13 @@ export class ClickhouseClient extends BaseClickhouseClient { abort_signal, clickhouse_settings: externalClickhouseSettings, queryId, + shouldSkipApplySettings, }: QueryInputs): Promise> { this.logDebugQuery(query, query_params); let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse - if (query !== GET_SETTINGS_QUERY) { + if (shouldSkipApplySettings) { clickhouseSettings = await this.processClickhouseSettings({ externalClickhouseSettings, }); diff --git a/packages/common-utils/src/core/metadata.ts b/packages/common-utils/src/core/metadata.ts index 841e1651b8..6a23936142 100644 --- a/packages/common-utils/src/core/metadata.ts +++ b/packages/common-utils/src/core/metadata.ts @@ -13,12 +13,7 @@ import { tableExpr, } from '@/clickhouse'; import { renderChartConfig } from '@/core/renderChartConfig'; -import type { - ChartConfig, - ChartConfigWithDateRange, - QuerySettings, - TSource, -} from '@/types'; +import type { ChartConfig, ChartConfigWithDateRange, TSource } from '@/types'; import { optimizeGetKeyValuesCalls } from './materializedViews'; import { objectHash } from './utils'; @@ -664,13 +659,15 @@ export class Metadata { return this.cache.getOrFetch( `${connectionId}.availableSettings`, async () => { + const query = 'SELECT name, value FROM system.settings'; try { const json = await this.clickhouseClient .query<'JSON'>({ connectionId, - query: GET_SETTINGS_QUERY, + query, query_params: undefined, clickhouse_settings: this.getClickHouseSettings(), + shouldSkipApplySettings: true, }) .then(res => res.json<{ name: string; value: string }>()); @@ -1229,8 +1226,6 @@ export function tcFromSource(source?: TSource): TableConnection { }; } -export const GET_SETTINGS_QUERY = 'SELECT name, value FROM system.settings'; - const __LOCAL_CACHE__ = new MetadataCache(); // TODO: better to init the Metadata object on the client side From 14f9233244a21ff0c2b8a8900e648a94936c0d50 Mon Sep 17 00:00:00 2001 From: Aaron Knudtson <87577305+knudtty@users.noreply.github.com> Date: Thu, 19 Feb 2026 14:27:41 -0500 Subject: [PATCH 5/5] add tests and fix boolean --- .../src/__tests__/clickhouse.test.ts | 247 ++++++++++++++++++ .../common-utils/src/clickhouse/browser.ts | 2 +- packages/common-utils/src/clickhouse/node.ts | 2 +- 3 files changed, 249 insertions(+), 2 deletions(-) diff --git a/packages/common-utils/src/__tests__/clickhouse.test.ts b/packages/common-utils/src/__tests__/clickhouse.test.ts index 6136ffaec5..d8beade8f1 100644 --- a/packages/common-utils/src/__tests__/clickhouse.test.ts +++ b/packages/common-utils/src/__tests__/clickhouse.test.ts @@ -8,6 +8,8 @@ import { convertCHDataTypeToJSType, JSDataType, } from '@/clickhouse'; +import { ClickhouseClient } from '@/clickhouse/node'; +import { Metadata, MetadataCache } from '@/core/metadata'; describe('convertCHDataTypeToJSType - unit - type', () => { it('Date type', () => { @@ -354,3 +356,248 @@ describe('computeResultSetRatio', () => { ); }); }); + +describe('processClickhouseSettings - optimization settings', () => { + let client: ClickhouseClient; + let mockQueryMethod: jest.Mock; + let metadataCache: MetadataCache; + + const createClient = () => { + const newClient = new ClickhouseClient({ + host: 'http://localhost:8123', + username: 'default', + password: '', + }); + + // Mock the underlying ClickHouse client's query method + const newMockQueryMethod = jest.fn(); + (newClient as any).client = { + query: newMockQueryMethod, + }; + + // Create a fresh metadata cache for each test + const newCache = new MetadataCache(); + + // Mock getMetadata to return a metadata instance with our fresh cache + jest + // eslint-disable-next-line + .spyOn(require('@/core/metadata'), 'getMetadata') + .mockImplementation(() => { + return new Metadata(newClient, newCache); + }); + + return { + client: newClient, + mockQueryMethod: newMockQueryMethod, + cache: newCache, + }; + }; + + beforeEach(() => { + const setup = createClient(); + client = setup.client; + mockQueryMethod = setup.mockQueryMethod; + metadataCache = setup.cache; + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + const setupMockQuery = ( + settingsData?: Array<{ name: string; value: string }>, + ) => { + mockQueryMethod.mockImplementation( + async ({ query, clickhouse_settings }: any) => { + // Return mocked settings for getSettings query + if (query === 'SELECT name, value FROM system.settings') { + return { + json: async () => ({ + data: settingsData || [], + }), + }; + } + // Return the settings so we can inspect them + return { + json: async () => ({ data: [], clickhouse_settings }), + }; + }, + ); + }; + + it('should apply default settings without server settings', async () => { + setupMockQuery([]); + + await client.query({ + query: 'SELECT 1', + format: 'JSON', + }); + + // Find the actual query call (not the settings query) + const actualQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => call[0].query === 'SELECT 1', + ); + + expect(actualQueryCall).toBeDefined(); + expect(actualQueryCall[0].clickhouse_settings).toEqual({ + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + }); + }); + + it('should apply all optimization settings when available on server', async () => { + setupMockQuery([ + { name: 'query_plan_optimize_lazy_materialization', value: '1' }, + { + name: 'query_plan_max_limit_for_lazy_materialization', + value: '100000', + }, + { name: 'use_skip_indexes_for_top_k', value: '1' }, + { name: 'query_plan_max_limit_for_top_k_optimization', value: '100000' }, + { name: 'use_top_k_dynamic_filtering', value: '1' }, + { name: 'use_skip_indexes_on_data_read', value: '1' }, + { name: 'use_skip_indexes_for_disjunctions', value: '1' }, + ]); + + await client.query({ + query: 'SELECT 1', + format: 'JSON', + }); + + const actualQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => call[0].query === 'SELECT 1', + ); + + expect(actualQueryCall).toBeDefined(); + expect(actualQueryCall[0].clickhouse_settings).toEqual({ + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + query_plan_optimize_lazy_materialization: '1', + query_plan_max_limit_for_lazy_materialization: '100000', + use_skip_indexes_for_top_k: '1', + query_plan_max_limit_for_top_k_optimization: '100000', + use_top_k_dynamic_filtering: '1', + use_skip_indexes_on_data_read: '1', + use_skip_indexes_for_disjunctions: '1', + }); + }); + + it('should only apply available optimization settings', async () => { + setupMockQuery([ + { name: 'use_skip_indexes_for_top_k', value: '1' }, + { name: 'use_skip_indexes_on_data_read', value: '1' }, + ]); + + await client.query({ + query: 'SELECT 1', + format: 'JSON', + }); + + const actualQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => call[0].query === 'SELECT 1', + ); + + expect(actualQueryCall).toBeDefined(); + const settings = actualQueryCall[0].clickhouse_settings; + expect(settings).toEqual({ + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + use_skip_indexes_for_top_k: '1', + use_skip_indexes_on_data_read: '1', + }); + expect(settings.query_plan_optimize_lazy_materialization).toBeUndefined(); + expect(settings.use_top_k_dynamic_filtering).toBeUndefined(); + }); + + it('should merge external clickhouse settings with optimization settings', async () => { + setupMockQuery([{ name: 'use_skip_indexes_for_top_k', value: '1' }]); + + await client.query({ + query: 'SELECT 1', + format: 'JSON', + clickhouse_settings: { + max_rows_to_read: '1000000', + }, + }); + + const actualQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => call[0].query === 'SELECT 1', + ); + + expect(actualQueryCall).toBeDefined(); + expect(actualQueryCall[0].clickhouse_settings).toEqual({ + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + use_skip_indexes_for_top_k: '1', + max_rows_to_read: '1000000', + }); + }); + + it('should not apply settings when shouldSkipApplySettings is true', async () => { + setupMockQuery([{ name: 'use_skip_indexes_for_top_k', value: '1' }]); + + await client.query({ + query: 'SELECT name, value FROM system.settings', + format: 'JSON', + shouldSkipApplySettings: true, + }); + + const settingsQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => + call[0].query === 'SELECT name, value FROM system.settings', + ); + + expect(settingsQueryCall).toBeDefined(); + expect(settingsQueryCall[0].clickhouse_settings).toBeUndefined(); + }); + + it('should handle metadata getSettings returning undefined (permissions error)', async () => { + mockQueryMethod.mockImplementation(async ({ query }: any) => { + if (query === 'SELECT name, value FROM system.settings') { + throw new Error('Not enough privileges'); + } + return { json: async () => ({ data: [] }) }; + }); + + // Should not throw, but silently continue without optimization settings + await client.query({ + query: 'SELECT 1', + format: 'JSON', + }); + + const actualQueryCall = mockQueryMethod.mock.calls.find( + (call: any) => call[0].query === 'SELECT 1', + ); + + expect(actualQueryCall).toBeDefined(); + expect(actualQueryCall[0].clickhouse_settings).toEqual({ + allow_experimental_analyzer: 1, + date_time_output_format: 'iso', + wait_end_of_query: 0, + cancel_http_readonly_queries_on_client_close: 1, + }); + }); + + it('should cache settings result across multiple queries', async () => { + setupMockQuery([{ name: 'use_skip_indexes_for_top_k', value: '1' }]); + + // Run two queries + await client.query({ query: 'SELECT 1', format: 'JSON' }); + await client.query({ query: 'SELECT 2', format: 'JSON' }); + + // Should only fetch settings once + const settingsCalls = mockQueryMethod.mock.calls.filter( + (call: any) => + call[0].query === 'SELECT name, value FROM system.settings', + ); + expect(settingsCalls.length).toEqual(1); + }); +}); diff --git a/packages/common-utils/src/clickhouse/browser.ts b/packages/common-utils/src/clickhouse/browser.ts index 5a6233545d..a9000c67e0 100644 --- a/packages/common-utils/src/clickhouse/browser.ts +++ b/packages/common-utils/src/clickhouse/browser.ts @@ -117,7 +117,7 @@ export class ClickhouseClient extends BaseClickhouseClient { let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse - if (shouldSkipApplySettings) { + if (!shouldSkipApplySettings) { clickhouseSettings = await this.processClickhouseSettings({ connectionId, externalClickhouseSettings, diff --git a/packages/common-utils/src/clickhouse/node.ts b/packages/common-utils/src/clickhouse/node.ts index 3300782de5..834496ac27 100644 --- a/packages/common-utils/src/clickhouse/node.ts +++ b/packages/common-utils/src/clickhouse/node.ts @@ -41,7 +41,7 @@ export class ClickhouseClient extends BaseClickhouseClient { let clickhouseSettings: ClickHouseSettings | undefined; // If this is the settings query, we must not process the clickhouse settings, or else we will infinitely recurse - if (shouldSkipApplySettings) { + if (!shouldSkipApplySettings) { clickhouseSettings = await this.processClickhouseSettings({ externalClickhouseSettings, });