From 0ccc35c5f191bf6575e3aaa64eda19b1543eaee6 Mon Sep 17 00:00:00 2001 From: Alvaro Date: Mon, 20 Jul 2026 13:57:07 +0200 Subject: [PATCH 1/2] fix(drift): compare SQL expressions in ClickHouse's canonical form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ClickHouse reformats expressions when it stores them — spacing argument separators and operators, adding precedence parens, rewriting `INTERVAL 5 YEAR` to `toIntervalYear(5)`. So a skip index, PARTITION BY, ORDER BY, or TTL written `cityHash64(a,b)` is stored `cityHash64(a, b)` and chkit drift reported permanent drift no migration could fix (#195). No local string normalizer can reproduce that rewriting. drift now normalizes expressions through ClickHouse's own formatter. The drift command collects every fragment across the compared tables, formats them in one batched `formatQuerySingleLineOrNull` round-trip (which yields NULL per unparseable fragment instead of failing the batch), and injects a map-backed canonicalizer into compareTableShape. Each field falls back to the existing string normalization when the fragment isn't in the map — an older server, a parse failure, or offline — so behavior is never worse than before. compareTableShape stays pure and synchronous; the ClickHouse call lives in the async payload builder. Adds @chkit/clickhouse `canonicalizeSqlFragments`. Closes #195 Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu --- .changeset/drift-ch-canonical-expressions.md | 8 ++ apps/docs/src/content/docs/cli/drift.md | 4 + packages/cli/src/commands/drift/compare.ts | 138 ++++++++++++++++--- packages/cli/src/commands/drift/payload.ts | 57 +++++++- packages/cli/src/test/drift.test.ts | 123 ++++++++++++++++- packages/clickhouse/src/canonicalize.test.ts | 75 ++++++++++ packages/clickhouse/src/canonicalize.ts | 54 ++++++++ packages/clickhouse/src/index.ts | 1 + 8 files changed, 432 insertions(+), 28 deletions(-) create mode 100644 .changeset/drift-ch-canonical-expressions.md create mode 100644 packages/clickhouse/src/canonicalize.test.ts create mode 100644 packages/clickhouse/src/canonicalize.ts diff --git a/.changeset/drift-ch-canonical-expressions.md b/.changeset/drift-ch-canonical-expressions.md new file mode 100644 index 00000000..8bc240f5 --- /dev/null +++ b/.changeset/drift-ch-canonical-expressions.md @@ -0,0 +1,8 @@ +--- +"@chkit/clickhouse": patch +"chkit": patch +--- + +`chkit drift` no longer reports false drift when a schema expression is spelled differently from how ClickHouse stores it. ClickHouse reformats expressions on the way in — spacing argument separators and operators, adding precedence parentheses, and rewriting `INTERVAL 5 YEAR` to `toIntervalYear(5)` — so a skip index, `PARTITION BY`, `ORDER BY`, or `TTL` written as `cityHash64(a,b)` was stored as `cityHash64(a, b)` and reported as permanent drift that no migration could fix (#195). + +`drift` now normalizes expressions through ClickHouse's own formatter (`formatQuerySingleLine`) before comparing, in a single batched round-trip, so equivalent expressions match regardless of spelling. When the connected server can't format a fragment — an older server without the function, or an expression it can't parse — that fragment falls back to plain text comparison, so behavior is never worse than before. `@chkit/clickhouse` gains `canonicalizeSqlFragments` for this. diff --git a/apps/docs/src/content/docs/cli/drift.md b/apps/docs/src/content/docs/cli/drift.md index e44a0095..1e68c2e9 100644 --- a/apps/docs/src/content/docs/cli/drift.md +++ b/apps/docs/src/content/docs/cli/drift.md @@ -33,6 +33,10 @@ Global flags documented on [CLI Overview](/cli/overview/#global-flags). When comparing engines, `SharedMergeTree` is normalized to `MergeTree`. This prevents false positives on managed environments (e.g. [ObsessionDB](https://obsessiondb.com)) where the server transparently substitutes `SharedMergeTree` for `MergeTree`. +### Expression normalization + +ClickHouse rewrites SQL expressions when it stores them — it spaces argument separators and operators, adds precedence parentheses, and rewrites `INTERVAL 5 YEAR` to `toIntervalYear(5)`. So a skip index, `PARTITION BY`, `ORDER BY`, or `TTL` expression written as `cityHash64(a,b)` is stored as `cityHash64(a, b)`. To avoid reporting these as drift, `drift` normalizes expressions through ClickHouse's own formatter before comparing, so equivalent expressions match regardless of spelling. When the connected server can't format an expression, comparison falls back to plain text matching. + ### Drift reason codes **Object-level drift:** diff --git a/packages/cli/src/commands/drift/compare.ts b/packages/cli/src/commands/drift/compare.ts index 5f3b5fad..d6ef3d92 100644 --- a/packages/cli/src/commands/drift/compare.ts +++ b/packages/cli/src/commands/drift/compare.ts @@ -3,6 +3,7 @@ import { isIndexProjection, normalizeProjectionIndex, normalizeSQLFragment, + splitTopLevelComma, type ColumnDefinition, type ProjectionDefinition, type SkipIndexDefinition, @@ -10,6 +11,24 @@ import { } from '@chkit/core' import { diffByName, diffNamedShapeMaps, diffSettings } from './diff.js' +/** + * Canonicalizes SQL fragments to the exact form ClickHouse stores, so a schema + * fragment compares equal to what a live table reports even when the two are + * spelled differently (`cityHash64(a,b)` vs `cityHash64(a, b)`, `n*2+1` vs + * `(n * 2) + 1`, `INTERVAL 5 YEAR` vs `toIntervalYear(5)`). Only ClickHouse's own + * formatter can produce this, so it is injected by the drift command; when it is + * absent (offline, or a fragment ClickHouse couldn't parse) each field falls + * back to plain string normalization (#195). + */ +export interface SqlCanonicalizer { + expression(fragment: string): string | null + query(fragment: string): string | null +} + +function canonicalizeExpression(base: string, canonicalizer?: SqlCanonicalizer): string { + return canonicalizer?.expression(base) ?? base +} + type TableDriftReasonCode = | 'missing_column' | 'extra_column' @@ -220,37 +239,104 @@ function renderIndexTypeFingerprint(index: SkipIndexDefinition): string { } } -function normalizeIndexShape(index: SkipIndexDefinition): string { +function normalizeIndexShape(index: SkipIndexDefinition, canonicalizer?: SqlCanonicalizer): string { return [ - `expr=${normalizeSQLFragment(index.expression)}`, + `expr=${canonicalizeExpression(normalizeSQLFragment(index.expression), canonicalizer)}`, `type=${renderIndexTypeFingerprint(index)}`, `granularity=${index.granularity}`, ].join('|') } -function normalizeProjectionShape(projection: ProjectionDefinition): string { +function normalizeProjectionShape( + projection: ProjectionDefinition, + canonicalizer?: SqlCanonicalizer +): string { if (isIndexProjection(projection)) { return [ `index=${normalizeProjectionIndex(projection.index)}`, `type=${projection.type.trim()}`, ].join('|') } - return `query=${normalizeSQLFragment(projection.query)}` + const base = normalizeSQLFragment(projection.query) + return `query=${canonicalizer?.query(base) ?? base}` } -function normalizeClause(value: string | undefined): string { +/** Strip backticks and one layer of wrapping parens; the shared prep both the + * comparison and the fragment collector build on. */ +function normalizeClauseBase(value: string | undefined): string { if (!value) return '' const normalized = normalizeSQLFragment(value).replace(/`/g, '') const wrapped = normalized.match(/^\((.*)\)$/) return wrapped?.[1] ? normalizeSQLFragment(wrapped[1]) : normalized } +function normalizeClause(value: string | undefined, canonicalizer?: SqlCanonicalizer): string { + const base = normalizeClauseBase(value) + if (!canonicalizer || base === '') return base + // Canonicalize each element so a function expression in a key/partition clause + // (`cityHash64(a,b)`) matches ClickHouse's stored spelling. + return splitTopLevelComma(base) + .map((element) => canonicalizeExpression(element.trim(), canonicalizer)) + .join(', ') +} + function normalizeEngine(value: string | undefined): string { if (!value) return '' return coreNormalizeEngine(normalizeSQLFragment(value)).toLowerCase() } -export function compareTableShape(expected: TableDefinition, actual: ActualTableShape): TableDriftDetail | null { +/** + * Every SQL fragment `compareTableShape` will look up in a `SqlCanonicalizer`, + * at the exact granularity it looks them up (whole expressions for index/ttl, + * per-element for key/partition clauses, whole query for SELECT projections). + * The drift command collects these across all compared tables, formats them in + * one round-trip, and hands back a map-backed canonicalizer. + */ +export function collectTableSqlFragments( + expected: TableDefinition, + actual: ActualTableShape +): { expressions: string[]; queries: string[] } { + const expressions: string[] = [] + const queries: string[] = [] + + const addExpression = (raw: string | undefined) => { + if (raw) expressions.push(normalizeSQLFragment(raw)) + } + const addClause = (value: string | undefined) => { + const base = normalizeClauseBase(value) + if (base) expressions.push(...splitTopLevelComma(base).map((element) => element.trim())) + } + + for (const index of expected.indexes ?? []) addExpression(index.expression) + for (const index of actual.indexes) addExpression(index.expression) + + addExpression(expected.ttl) + addExpression(actual.ttl) + + addClause((expected.primaryKey.length > 0 ? expected.primaryKey : expected.orderBy).join(', ')) + addClause(actual.primaryKey ?? actual.orderBy) + addClause(expected.orderBy.join(', ')) + addClause(actual.orderBy) + addClause((expected.uniqueKey ?? []).join(', ')) + addClause(actual.uniqueKey) + addClause(expected.partitionBy) + addClause(actual.partitionBy) + + for (const projection of expected.projections ?? []) { + if (!isIndexProjection(projection)) queries.push(normalizeSQLFragment(projection.query)) + } + for (const projection of actual.projections) { + if (!isIndexProjection(projection)) queries.push(normalizeSQLFragment(projection.query)) + } + + return { expressions, queries } +} + +export function compareTableShape( + expected: TableDefinition, + actual: ActualTableShape, + canonicalizer?: SqlCanonicalizer +): TableDriftDetail | null { const columnDiff = diffByName( expected.columns, actual.columns, @@ -264,13 +350,21 @@ export function compareTableShape(expected: TableDefinition, actual: ActualTable const settingDiffs = diffSettings(expected.settings ?? {}, actual.settings) const expectedIndexes = new Map( - (expected.indexes ?? []).map((idx) => [idx.name, normalizeIndexShape(idx)]) + (expected.indexes ?? []).map((idx) => [idx.name, normalizeIndexShape(idx, canonicalizer)]) + ) + const actualIndexes = new Map( + actual.indexes.map((idx) => [idx.name, normalizeIndexShape(idx, canonicalizer)]) ) - const actualIndexes = new Map(actual.indexes.map((idx) => [idx.name, normalizeIndexShape(idx)])) const indexDiffs = diffNamedShapeMaps(expectedIndexes, actualIndexes) - const expectedTTL = expected.ttl ? normalizeSQLFragment(expected.ttl) : '' - const actualTTL = actual.ttl ? normalizeSQLFragment(actual.ttl) : '' + const expectedTTL = canonicalizeExpression( + expected.ttl ? normalizeSQLFragment(expected.ttl) : '', + canonicalizer + ) + const actualTTL = canonicalizeExpression( + actual.ttl ? normalizeSQLFragment(actual.ttl) : '', + canonicalizer + ) const ttlMismatch = expectedTTL !== actualTTL const engineMismatch = normalizeEngine(expected.engine) !== normalizeEngine(actual.engine) @@ -279,28 +373,32 @@ export function compareTableShape(expected: TableDefinition, actual: ActualTable // Mirror that on both sides (as canonical.ts does for the schema), else every // such table drifts forever (#194). const expectedPrimaryKey = normalizeClause( - (expected.primaryKey.length > 0 ? expected.primaryKey : expected.orderBy).join(', ') + (expected.primaryKey.length > 0 ? expected.primaryKey : expected.orderBy).join(', '), + canonicalizer ) - const actualPrimaryKey = normalizeClause(actual.primaryKey ?? actual.orderBy) + const actualPrimaryKey = normalizeClause(actual.primaryKey ?? actual.orderBy, canonicalizer) const primaryKeyMismatch = expectedPrimaryKey !== actualPrimaryKey - const expectedOrderBy = normalizeClause(expected.orderBy.join(', ')) - const actualOrderBy = normalizeClause(actual.orderBy) + const expectedOrderBy = normalizeClause(expected.orderBy.join(', '), canonicalizer) + const actualOrderBy = normalizeClause(actual.orderBy, canonicalizer) const orderByMismatch = expectedOrderBy !== actualOrderBy - const expectedUniqueKey = normalizeClause((expected.uniqueKey ?? []).join(', ')) - const actualUniqueKey = normalizeClause(actual.uniqueKey) + const expectedUniqueKey = normalizeClause((expected.uniqueKey ?? []).join(', '), canonicalizer) + const actualUniqueKey = normalizeClause(actual.uniqueKey, canonicalizer) const uniqueKeyMismatch = expectedUniqueKey !== actualUniqueKey - const expectedPartitionBy = normalizeClause(expected.partitionBy) - const actualPartitionBy = normalizeClause(actual.partitionBy) + const expectedPartitionBy = normalizeClause(expected.partitionBy, canonicalizer) + const actualPartitionBy = normalizeClause(actual.partitionBy, canonicalizer) const partitionByMismatch = expectedPartitionBy !== actualPartitionBy const expectedProjections = new Map( (expected.projections ?? []).map((projection) => [ projection.name, - normalizeProjectionShape(projection), + normalizeProjectionShape(projection, canonicalizer), ]) ) const actualProjections = new Map( - actual.projections.map((projection) => [projection.name, normalizeProjectionShape(projection)]) + actual.projections.map((projection) => [ + projection.name, + normalizeProjectionShape(projection, canonicalizer), + ]) ) const projectionDiffs = diffNamedShapeMaps(expectedProjections, actualProjections) diff --git a/packages/cli/src/commands/drift/payload.ts b/packages/cli/src/commands/drift/payload.ts index 918c3a2a..06e73faf 100644 --- a/packages/cli/src/commands/drift/payload.ts +++ b/packages/cli/src/commands/drift/payload.ts @@ -1,17 +1,59 @@ import { join } from 'node:path' -import { isUnknownDatabaseError, type ClickHouseExecutor, type SchemaObjectRef } from '@chkit/clickhouse' +import { + canonicalizeSqlFragments, + isUnknownDatabaseError, + type ClickHouseExecutor, + type IntrospectedTable, + type SchemaObjectRef, +} from '@chkit/clickhouse' import type { ChxConfig, Snapshot, TableDefinition } from '@chkit/core' import { debug } from '../../runtime/debug.js' import type { TableScope } from '../../runtime/table-scope.js' import { + collectTableSqlFragments, compareSchemaObjects, compareTableShape, type ObjectDriftDetail, + type SqlCanonicalizer, type TableDriftDetail, } from './compare.js' +/** + * Build a canonicalizer for every SQL fragment across the compared tables in a + * single ClickHouse round-trip. Returns undefined when there is nothing to + * canonicalize or ClickHouse can't format (old server, offline) — the comparer + * then falls back to plain string normalization. + */ +async function buildSqlCanonicalizer( + db: ClickHouseExecutor, + pairs: Array<{ expected: TableDefinition; actual: IntrospectedTable }> +): Promise { + const expressions: string[] = [] + const queries: string[] = [] + for (const { expected, actual } of pairs) { + const fragments = collectTableSqlFragments(expected, actual) + expressions.push(...fragments.expressions) + queries.push(...fragments.queries) + } + if (expressions.length === 0 && queries.length === 0) return undefined + + try { + const [expressionMap, queryMap] = await Promise.all([ + canonicalizeSqlFragments(db, expressions, { wrap: true }), + canonicalizeSqlFragments(db, queries, { wrap: false }), + ]) + return { + expression: (fragment) => expressionMap.get(fragment) ?? null, + query: (fragment) => queryMap.get(fragment) ?? null, + } + } catch (error) { + debug('drift', `sql canonicalization unavailable, using string comparison: ${String(error)}`) + return undefined + } +} + export interface DriftPayload { scope?: TableScope snapshotFile: string @@ -119,12 +161,13 @@ export async function buildDriftPayload( expectedTables.map((table) => [`${table.database}.${table.name}`, table]) ) const actualTables = await db.listTableDetails([...expectedDatabases]) - const tableDrift = actualTables - .map((actual) => { - const expected = expectedTableMap.get(`${actual.database}.${actual.name}`) - if (!expected) return null - return compareTableShape(expected, actual) - }) + const pairs = actualTables.flatMap((actual) => { + const expected = expectedTableMap.get(`${actual.database}.${actual.name}`) + return expected ? [{ expected, actual }] : [] + }) + const canonicalizer = await buildSqlCanonicalizer(db, pairs) + const tableDrift = pairs + .map(({ expected, actual }) => compareTableShape(expected, actual, canonicalizer)) .filter((item): item is NonNullable => item !== null) .sort((a, b) => a.table.localeCompare(b.table)) diff --git a/packages/cli/src/test/drift.test.ts b/packages/cli/src/test/drift.test.ts index 971c581f..92aa40f4 100644 --- a/packages/cli/src/test/drift.test.ts +++ b/packages/cli/src/test/drift.test.ts @@ -2,7 +2,13 @@ import { describe, expect, test } from 'bun:test' import { table } from '@chkit/core' -import { compareSchemaObjects, compareTableShape, summarizeDriftReasons } from '../commands/drift/compare.js' +import { + collectTableSqlFragments, + compareSchemaObjects, + compareTableShape, + type SqlCanonicalizer, + summarizeDriftReasons, +} from '../commands/drift/compare.js' describe('@chkit/cli drift comparer', () => { test('emits missing_object reason code when expected object is absent', () => { @@ -432,3 +438,118 @@ describe('@chkit/cli drift comparer', () => { expect(result.partitionByMismatch).toBe(true) }) }) + +describe('@chkit/cli drift SQL canonicalization (#195)', () => { + // Stand-in for ClickHouse's formatter: collapse whitespace and space after + // commas, so `cityHash64(a,b)` and `cityHash64(a, b)` share a canonical form. + const fakeClickHouseFormat = (fragment: string): string => + fragment.replace(/\s+/g, ' ').replace(/\s*,\s*/g, ', ').trim() + const canonicalizer: SqlCanonicalizer = { + expression: fakeClickHouseFormat, + query: fakeClickHouseFormat, + } + + const withSkipIndex = (expression: string) => ({ + database: 'app', + name: 'events', + engine: 'MergeTree()', + columns: [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + ], + primaryKey: ['a'] as string[], + orderBy: ['a'] as string[], + indexes: [{ name: 'i', expression, type: 'minmax' as const, granularity: 1 }], + }) + + test('a skip-index expression that differs only in comma spacing reads clean', () => { + const expected = table(withSkipIndex('cityHash64(a,b)')) + const actual = { + engine: 'MergeTree()', + primaryKey: '(a)', + orderBy: '(a)', + columns: [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + ], + settings: {}, + // What ClickHouse actually stores for `cityHash64(a,b)`. + indexes: [{ name: 'i', expression: 'cityHash64(a, b)', type: 'minmax' as const, granularity: 1 }], + projections: [], + } + + // Without the canonicalizer the spacing reads as drift (today's behavior)... + expect(compareTableShape(expected, actual)?.reasonCodes).toContain('index_mismatch') + // ...with it, the two are recognized as equal. + expect(compareTableShape(expected, actual, canonicalizer)).toBeNull() + }) + + test('a genuinely different index expression still drifts under canonicalization', () => { + const expected = table(withSkipIndex('cityHash64(a,b)')) + const actual = { + engine: 'MergeTree()', + primaryKey: '(a)', + orderBy: '(a)', + columns: [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + ], + settings: {}, + indexes: [{ name: 'i', expression: 'sipHash64(a, b)', type: 'minmax' as const, granularity: 1 }], + projections: [], + } + + expect(compareTableShape(expected, actual, canonicalizer)?.reasonCodes).toContain('index_mismatch') + }) + + test('collectTableSqlFragments gathers index, ttl, clause elements, and projection queries', () => { + const expected = table({ + database: 'app', + name: 'events', + engine: 'MergeTree()', + columns: [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + { name: 'ts', type: 'DateTime' }, + ], + primaryKey: ['a'], + orderBy: ['a', 'b'], + partitionBy: 'toYYYYMM(ts)', + ttl: 'ts + toIntervalDay(30)', + indexes: [{ name: 'i', expression: 'cityHash64(a,b)', type: 'minmax', granularity: 1 }], + projections: [ + { name: 'p_sel', query: 'SELECT a, count() GROUP BY a' }, + { name: 'p_idx', index: 'b', type: 'basic' }, + ], + }) + const actual = { + engine: 'MergeTree()', + primaryKey: undefined, + orderBy: '(a, b)', + columns: [ + { name: 'a', type: 'String' }, + { name: 'b', type: 'String' }, + { name: 'ts', type: 'DateTime' }, + ], + settings: {}, + indexes: [{ name: 'i', expression: 'cityHash64(a, b)', type: 'minmax' as const, granularity: 1 }], + partitionBy: 'toYYYYMM(ts)', + ttl: 'ts + toIntervalDay(30)', + projections: [ + { name: 'p_sel', query: 'SELECT a, count() GROUP BY a' }, + { name: 'p_idx', index: 'b', type: 'basic' as const }, + ], + } + + const { expressions, queries } = collectTableSqlFragments(expected, actual) + // Index expressions and clause elements are collected as expressions... + expect(expressions).toContain('cityHash64(a,b)') + expect(expressions).toContain('cityHash64(a, b)') + expect(expressions).toContain('toYYYYMM(ts)') + expect(expressions).toContain('a') + expect(expressions).toContain('b') + // ...SELECT projections as queries, and the index-only projection is not. + expect(queries).toContain('SELECT a, count() GROUP BY a') + expect(queries).not.toContain('b') + }) +}) diff --git a/packages/clickhouse/src/canonicalize.test.ts b/packages/clickhouse/src/canonicalize.test.ts new file mode 100644 index 00000000..1bf2f878 --- /dev/null +++ b/packages/clickhouse/src/canonicalize.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, test } from 'bun:test' + +import { canonicalizeSqlFragments } from './canonicalize.js' +import type { ClickHouseExecutor } from './index.js' + +// Minimal executor stand-in: records the SQL it was asked to run and replays a +// canned arrayMap result. Only `query` is exercised by canonicalizeSqlFragments. +function fakeExecutor(reply: Array, capture: { sql?: string }): ClickHouseExecutor { + return { + async query(sql: string): Promise { + capture.sql = sql + return [{ formatted: reply } as unknown as T] + }, + } as unknown as ClickHouseExecutor +} + +describe('canonicalizeSqlFragments', () => { + test('strips the SELECT wrapper and maps each fragment to its canonical form', async () => { + const capture: { sql?: string } = {} + const executor = fakeExecutor(['SELECT cityHash64(a, b)', 'SELECT (n * 2) + 1'], capture) + + const map = await canonicalizeSqlFragments(executor, ['cityHash64(a,b)', 'n*2+1'], { wrap: true }) + + expect(map.get('cityHash64(a,b)')).toBe('cityHash64(a, b)') + expect(map.get('n*2+1')).toBe('(n * 2) + 1') + expect(capture.sql).toContain("formatQuerySingleLineOrNull('SELECT ' || fragment)") + }) + + test('does not wrap when formatting full queries', async () => { + const capture: { sql?: string } = {} + const executor = fakeExecutor(['SELECT a, count() GROUP BY a'], capture) + + const map = await canonicalizeSqlFragments(executor, ['SELECT a,count() GROUP BY a'], { + wrap: false, + }) + + expect(map.get('SELECT a,count() GROUP BY a')).toBe('SELECT a, count() GROUP BY a') + expect(capture.sql).toContain('formatQuerySingleLineOrNull(fragment)') + }) + + test('omits fragments ClickHouse could not format (null)', async () => { + const executor = fakeExecutor(['SELECT cityHash64(a, b)', null], {}) + + const map = await canonicalizeSqlFragments(executor, ['cityHash64(a,b)', '((bad'], { wrap: true }) + + expect(map.get('cityHash64(a,b)')).toBe('cityHash64(a, b)') + expect(map.has('((bad')).toBe(false) + }) + + test('escapes quotes and backslashes, and dedupes blank/empty input', async () => { + const capture: { sql?: string } = {} + const executor = fakeExecutor(["SELECT concat(a, 'x,y')"], capture) + + const map = await canonicalizeSqlFragments(executor, ["concat(a,'x,y')", '', ' '], { wrap: true }) + + // Single-quote doubled, blanks dropped — a valid single-element array literal. + expect(capture.sql).toContain("['concat(a,''x,y'')']") + expect(map.get("concat(a,'x,y')")).toBe("concat(a, 'x,y')") + }) + + test('returns an empty map without querying when there is nothing to format', async () => { + let called = false + const executor = { + async query() { + called = true + return [] + }, + } as unknown as ClickHouseExecutor + + const map = await canonicalizeSqlFragments(executor, ['', ' '], { wrap: true }) + + expect(map.size).toBe(0) + expect(called).toBe(false) + }) +}) diff --git a/packages/clickhouse/src/canonicalize.ts b/packages/clickhouse/src/canonicalize.ts new file mode 100644 index 00000000..253a353a --- /dev/null +++ b/packages/clickhouse/src/canonicalize.ts @@ -0,0 +1,54 @@ +import type { ClickHouseExecutor } from './index.js' + +function quoteLiteral(value: string): string { + return `'${value.replace(/\\/g, '\\\\').replace(/'/g, "''")}'` +} + +function stripSelectPrefix(formatted: string): string | null { + const match = formatted.match(/^SELECT\s+([\s\S]+)$/i) + return match?.[1]?.trim() ?? null +} + +/** + * Canonicalize SQL fragments through ClickHouse's own formatter, so a schema + * fragment can be compared against what ClickHouse actually stores. The server + * rewrites expressions on the way in — spacing, operator precedence parens, + * `INTERVAL n UNIT` → `toIntervalUnit(n)` — which no local string normalizer can + * reproduce (#195). + * + * `wrap` distinguishes a scalar expression (wrapped as `SELECT `, prefix + * stripped back off) from a full `SELECT` query (formatted as-is). Formatting is + * batched into a single query using `formatQuerySingleLineOrNull`, which yields + * NULL for a fragment it can't parse rather than failing the whole batch — those + * are simply left out of the map so the caller falls back to string comparison. + * + * Returns a map from the input fragment to its ClickHouse-canonical form. A + * fragment missing from the map (unparseable, or an empty input) has no entry. + */ +export async function canonicalizeSqlFragments( + executor: ClickHouseExecutor, + fragments: readonly string[], + options: { wrap: boolean } +): Promise> { + const result = new Map() + const unique = [...new Set(fragments.map((fragment) => fragment.trim()).filter((f) => f.length > 0))] + if (unique.length === 0) return result + + const arrayLiteral = unique.map(quoteLiteral).join(', ') + const formatCall = options.wrap + ? `formatQuerySingleLineOrNull('SELECT ' || fragment)` + : `formatQuerySingleLineOrNull(fragment)` + const rows = await executor.query<{ formatted: Array }>( + `SELECT arrayMap(fragment -> ${formatCall}, [${arrayLiteral}]) AS formatted` + ) + + const formatted = rows[0]?.formatted ?? [] + for (let i = 0; i < unique.length; i += 1) { + const raw = unique[i] + const value = formatted[i] + if (raw === undefined || value == null) continue + const canonical = options.wrap ? stripSelectPrefix(value) : value.trim() + if (canonical) result.set(raw, canonical) + } + return result +} diff --git a/packages/clickhouse/src/index.ts b/packages/clickhouse/src/index.ts index d1504e46..0e2873bb 100644 --- a/packages/clickhouse/src/index.ts +++ b/packages/clickhouse/src/index.ts @@ -159,6 +159,7 @@ export { parseTTLFromCreateTableQuery, parseUniqueKeyFromCreateTableQuery, } from './create-table-parser.js' +export { canonicalizeSqlFragments } from './canonicalize.js' export function inferSchemaKindFromEngine( engine: string, From 016a776e2259ee28cd40627b7b57d07e3952ea82 Mon Sep 17 00:00:00 2001 From: Alvaro Date: Mon, 20 Jul 2026 17:59:06 +0200 Subject: [PATCH 2/2] fix(drift): chunk canonicalization query to stay under max_query_size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up. canonicalizeSqlFragments inlined every fragment across all compared tables into one query. On a large schema the string-literal array could exceed ClickHouse's default max_query_size (256 KiB), throw, and — since buildSqlCanonicalizer swallows the error and falls back — silently revert the whole drift run to string comparison, no-opping the feature exactly where it matters. Fragments are now split into batches bounded well under the limit, each run as its own query and merged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu --- packages/clickhouse/src/canonicalize.test.ts | 27 +++++++++++ packages/clickhouse/src/canonicalize.ts | 47 +++++++++++++++----- 2 files changed, 63 insertions(+), 11 deletions(-) diff --git a/packages/clickhouse/src/canonicalize.test.ts b/packages/clickhouse/src/canonicalize.test.ts index 1bf2f878..c34753b4 100644 --- a/packages/clickhouse/src/canonicalize.test.ts +++ b/packages/clickhouse/src/canonicalize.test.ts @@ -72,4 +72,31 @@ describe('canonicalizeSqlFragments', () => { expect(map.size).toBe(0) expect(called).toBe(false) }) + + // Guards against overflowing ClickHouse's max_query_size (256 KiB) on a large + // schema, which would throw and silently drop the whole run to string + // comparison — the feature no-opping exactly where it's needed. + test('splits large input into multiple bounded queries and merges the results', async () => { + const sqls: string[] = [] + // Echo each literal in the received array back as `SELECT `, so + // every fragment maps to itself regardless of how batching splits them. + const executor = { + async query(sql: string): Promise { + sqls.push(sql) + const literalCount = (sql.match(/'/g)?.length ?? 0) / 2 + return [{ formatted: Array.from({ length: literalCount }, () => 'SELECT ok') } as unknown as T] + }, + } as unknown as ClickHouseExecutor + + // ~40 bytes of literal each × 8000 ≈ 320 KB → forces several batches. + const fragments = Array.from({ length: 8000 }, (_, i) => `expr_${i}_${'x'.repeat(30)}`) + const map = await canonicalizeSqlFragments(executor, fragments, { wrap: true }) + + expect(sqls.length).toBeGreaterThan(1) + for (const sql of sqls) expect(sql.length).toBeLessThan(200_000) + // Every distinct fragment was formatted across the batches. + expect(map.size).toBe(fragments.length) + expect(map.get(fragments[0]!)).toBe('ok') + expect(map.get(fragments.at(-1)!)).toBe('ok') + }) }) diff --git a/packages/clickhouse/src/canonicalize.ts b/packages/clickhouse/src/canonicalize.ts index 253a353a..eeb462fe 100644 --- a/packages/clickhouse/src/canonicalize.ts +++ b/packages/clickhouse/src/canonicalize.ts @@ -9,6 +9,29 @@ function stripSelectPrefix(formatted: string): string | null { return match?.[1]?.trim() ?? null } +// Keep each batched query well under ClickHouse's default max_query_size +// (262144 bytes) so a large schema doesn't overflow a single query — which would +// throw and silently drop the whole run back to string comparison. +const MAX_BATCH_LITERAL_BYTES = 128_000 + +function batchFragments(fragments: readonly string[]): string[][] { + const batches: string[][] = [] + let current: string[] = [] + let size = 0 + for (const fragment of fragments) { + const cost = quoteLiteral(fragment).length + 2 // literal + ", " separator + if (current.length > 0 && size + cost > MAX_BATCH_LITERAL_BYTES) { + batches.push(current) + current = [] + size = 0 + } + current.push(fragment) + size += cost + } + if (current.length > 0) batches.push(current) + return batches +} + /** * Canonicalize SQL fragments through ClickHouse's own formatter, so a schema * fragment can be compared against what ClickHouse actually stores. The server @@ -34,21 +57,23 @@ export async function canonicalizeSqlFragments( const unique = [...new Set(fragments.map((fragment) => fragment.trim()).filter((f) => f.length > 0))] if (unique.length === 0) return result - const arrayLiteral = unique.map(quoteLiteral).join(', ') const formatCall = options.wrap ? `formatQuerySingleLineOrNull('SELECT ' || fragment)` : `formatQuerySingleLineOrNull(fragment)` - const rows = await executor.query<{ formatted: Array }>( - `SELECT arrayMap(fragment -> ${formatCall}, [${arrayLiteral}]) AS formatted` - ) - const formatted = rows[0]?.formatted ?? [] - for (let i = 0; i < unique.length; i += 1) { - const raw = unique[i] - const value = formatted[i] - if (raw === undefined || value == null) continue - const canonical = options.wrap ? stripSelectPrefix(value) : value.trim() - if (canonical) result.set(raw, canonical) + for (const batch of batchFragments(unique)) { + const arrayLiteral = batch.map(quoteLiteral).join(', ') + const rows = await executor.query<{ formatted: Array }>( + `SELECT arrayMap(fragment -> ${formatCall}, [${arrayLiteral}]) AS formatted` + ) + const formatted = rows[0]?.formatted ?? [] + for (let i = 0; i < batch.length; i += 1) { + const raw = batch[i] + const value = formatted[i] + if (raw === undefined || value == null) continue + const canonical = options.wrap ? stripSelectPrefix(value) : value.trim() + if (canonical) result.set(raw, canonical) + } } return result }