From bc69dd89db765b9724fa2f461ade866c0c131bb9 Mon Sep 17 00:00:00 2001 From: Alvaro Date: Mon, 20 Jul 2026 10:55:29 +0200 Subject: [PATCH] fix(pull,drift): parse table clauses past the column list; treat derived primary key as clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related bugs on tables whose ORDER BY sits alongside a projection or a derived primary key. Both verified against live ClickHouse 26.3. #190 — chkit pull leaked the engine into orderBy/primaryKey. The clause parsers matched the first ENGINE/ORDER BY/PRIMARY KEY/PARTITION BY/TTL/ SETTINGS keyword anywhere in SHOW CREATE TABLE. A projection whose SELECT body contains ORDER BY (or a column-level TTL) sits in the column list, before the table-level clauses, so the parser matched the inner keyword and swallowed the engine into the clause. Table-level clauses are now parsed only from the portion after the column list. #194 — chkit drift always reported primary_key_mismatch for tables whose PRIMARY KEY is derived from ORDER BY. ClickHouse omits the derived key from SHOW CREATE, but the canonicalized schema carries it, so the two never matched. Drift now applies the same derivation to the live side; a genuine primary-key difference is still reported. The two compound: fixing #190 makes pull emit a valid ORDER BY-only schema, and #194 makes that schema read clean against its source table. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu --- .../pull-clause-and-derived-primary-key.md | 11 ++++ packages/cli/src/commands/drift/compare.ts | 10 ++- packages/cli/src/test/drift.test.ts | 61 +++++++++++++++++++ .../clickhouse/src/create-table-parser.ts | 46 +++++++++++--- packages/clickhouse/src/index.test.ts | 32 ++++++++++ 5 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 .changeset/pull-clause-and-derived-primary-key.md diff --git a/.changeset/pull-clause-and-derived-primary-key.md b/.changeset/pull-clause-and-derived-primary-key.md new file mode 100644 index 0000000..3d4301d --- /dev/null +++ b/.changeset/pull-clause-and-derived-primary-key.md @@ -0,0 +1,11 @@ +--- +"@chkit/clickhouse": patch +"@chkit/plugin-pull": patch +"chkit": patch +--- + +Fix two related pull/drift bugs around tables whose `ORDER BY` is declared alongside a projection or a derived primary key. + +`chkit pull` parsed table-level clauses (`ENGINE`, `ORDER BY`, `PRIMARY KEY`, `PARTITION BY`, `TTL`, `SETTINGS`) by matching the first keyword anywhere in `SHOW CREATE TABLE`. A projection whose `SELECT` body contains `ORDER BY` — or a column-level `TTL` — sits in the column list before those clauses, so the parser matched the inner keyword and swallowed the engine into `orderBy`/`primaryKey`, producing an invalid pulled schema (#190). Table-level clauses are now parsed only from the portion after the column list. + +`chkit drift` always reported `primary_key_mismatch` for any table whose `PRIMARY KEY` is derived from `ORDER BY`. ClickHouse omits the derived key from `SHOW CREATE TABLE`, but the schema carries it, so the two never matched. Drift now applies the same derivation to the live side, so a derived primary key reads clean while a genuine primary-key difference is still reported (#194). diff --git a/packages/cli/src/commands/drift/compare.ts b/packages/cli/src/commands/drift/compare.ts index 20d6000..5f3b5fa 100644 --- a/packages/cli/src/commands/drift/compare.ts +++ b/packages/cli/src/commands/drift/compare.ts @@ -274,8 +274,14 @@ export function compareTableShape(expected: TableDefinition, actual: ActualTable const ttlMismatch = expectedTTL !== actualTTL const engineMismatch = normalizeEngine(expected.engine) !== normalizeEngine(actual.engine) - const expectedPrimaryKey = normalizeClause(expected.primaryKey.join(', ')) - const actualPrimaryKey = normalizeClause(actual.primaryKey) + // ClickHouse derives PRIMARY KEY from ORDER BY when it is omitted, then omits + // it from SHOW CREATE — so a table with only ORDER BY reports no primary key. + // 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(', ') + ) + const actualPrimaryKey = normalizeClause(actual.primaryKey ?? actual.orderBy) const primaryKeyMismatch = expectedPrimaryKey !== actualPrimaryKey const expectedOrderBy = normalizeClause(expected.orderBy.join(', ')) const actualOrderBy = normalizeClause(actual.orderBy) diff --git a/packages/cli/src/test/drift.test.ts b/packages/cli/src/test/drift.test.ts index d218ed1..971c581 100644 --- a/packages/cli/src/test/drift.test.ts +++ b/packages/cli/src/test/drift.test.ts @@ -123,6 +123,67 @@ describe('@chkit/cli drift comparer', () => { expect(result).toBeNull() }) + // #194: ClickHouse derives PRIMARY KEY from ORDER BY when it is omitted, then + // omits it from SHOW CREATE. A pulled schema carries the derived primary key, + // so the live table (no PRIMARY KEY) must not read as drift against it. + test('treats a primary key derived from ORDER BY as clean', () => { + const expected = table({ + database: 'bi', + name: 'price_history', + engine: 'MergeTree()', + columns: [ + { name: 'day', type: 'Date' }, + { name: 'csin', type: 'String' }, + ], + primaryKey: ['day', 'csin'], // what `chkit pull` writes out (derived) + orderBy: ['day', 'csin'], + }) + + const result = compareTableShape(expected, { + engine: 'MergeTree()', + primaryKey: undefined, // live table has no PRIMARY KEY clause + orderBy: '(day, csin)', + columns: [ + { name: 'day', type: 'Date' }, + { name: 'csin', type: 'String' }, + ], + settings: {}, + indexes: [], + projections: [], + }) + + expect(result).toBeNull() + }) + + test('still reports primary_key_mismatch when the keys genuinely differ', () => { + const expected = table({ + database: 'bi', + name: 'price_history', + engine: 'MergeTree()', + columns: [ + { name: 'day', type: 'Date' }, + { name: 'csin', type: 'String' }, + ], + primaryKey: ['day'], + orderBy: ['day', 'csin'], + }) + + const result = compareTableShape(expected, { + engine: 'MergeTree()', + primaryKey: '(csin)', + orderBy: '(day, csin)', + columns: [ + { name: 'day', type: 'Date' }, + { name: 'csin', type: 'String' }, + ], + settings: {}, + indexes: [], + projections: [], + }) + + expect(result?.reasonCodes).toContain('primary_key_mismatch') + }) + // ClickHouse rewrites a single-column `INDEX (id)` to `INDEX id`, so a schema // written with parens must not read as drift against the live table. test('treats an index-only projection as clean regardless of index parens', () => { diff --git a/packages/clickhouse/src/create-table-parser.ts b/packages/clickhouse/src/create-table-parser.ts index 1699678..09fbed9 100644 --- a/packages/clickhouse/src/create-table-parser.ts +++ b/packages/clickhouse/src/create-table-parser.ts @@ -10,17 +10,28 @@ function parseClauseFromCreateTableQuery( stopPattern: RegExp ): string | undefined { if (!createTableQuery) return undefined - const start = createTableQuery.match(clausePattern) + // Table-level clauses (ENGINE, ORDER BY, PRIMARY KEY, ...) only appear after + // the column list. Searching the whole query would match a keyword inside the + // body — e.g. the `ORDER BY` of a projection's SELECT — and swallow the real + // clause plus everything up to the next stop keyword (issue #190). + const options = extractTableOptions(createTableQuery) + const start = options.match(clausePattern) if (!start || start.index === undefined) return undefined - const afterClause = createTableQuery.slice(start.index + start[0].length) + const afterClause = options.slice(start.index + start[0].length) const stop = afterClause.match(stopPattern) const raw = (stop ? afterClause.slice(0, stop.index) : afterClause).trim() if (!raw) return undefined return normalizeSQLFragment(raw) } -function extractCreateTableBody(createTableQuery: string | undefined): string | undefined { - if (!createTableQuery) return undefined +/** + * Positions of the parens that open and close the column list — the close being + * the one right before the table-level `ENGINE =`. Returns undefined when there + * is no balanced column list (e.g. a view, or a query we can't parse). + */ +function findColumnListBounds( + createTableQuery: string +): { open: number; close: number } | undefined { const engineMatch = /\)\s*ENGINE\s*=/i.exec(createTableQuery) if (!engineMatch || engineMatch.index === undefined) return undefined const left = createTableQuery.slice(0, engineMatch.index + 1) @@ -50,19 +61,34 @@ function extractCreateTableBody(createTableQuery: string | undefined): string | } if (char === ')') { depth -= 1 - if (depth === 0) { - const body = left.slice(openIndex + 1, i).trim() - return body.length > 0 ? body : undefined - } + if (depth === 0) return { open: openIndex, close: i } } } return undefined } +function extractCreateTableBody(createTableQuery: string | undefined): string | undefined { + if (!createTableQuery) return undefined + const bounds = findColumnListBounds(createTableQuery) + if (!bounds) return undefined + const body = createTableQuery.slice(bounds.open + 1, bounds.close).trim() + return body.length > 0 ? body : undefined +} + +/** + * Everything after the column list: `ENGINE = ... PARTITION BY ... ORDER BY ...`. + * This is where table-level clauses live. Falls back to the whole query when the + * column list can't be located, preserving behaviour for unparseable inputs. + */ +function extractTableOptions(createTableQuery: string): string { + const bounds = findColumnListBounds(createTableQuery) + return bounds ? createTableQuery.slice(bounds.close + 1) : createTableQuery +} + export function parseSettingsFromCreateTableQuery(createTableQuery: string | undefined): Record { if (!createTableQuery) return {} - const settingsMatch = createTableQuery.match(/\bSETTINGS\b([\s\S]*?)(?:;|$)/i) + const settingsMatch = extractTableOptions(createTableQuery).match(/\bSETTINGS\b([\s\S]*?)(?:;|$)/i) if (!settingsMatch?.[1]) return {} const rawSettings = settingsMatch[1].trim() if (!rawSettings) return {} @@ -81,7 +107,7 @@ export function parseSettingsFromCreateTableQuery(createTableQuery: string | und export function parseTTLFromCreateTableQuery(createTableQuery: string | undefined): string | undefined { if (!createTableQuery) return undefined - const ttlMatch = createTableQuery.match(/\bTTL\b([\s\S]*?)(?:\bSETTINGS\b|;|$)/i) + const ttlMatch = extractTableOptions(createTableQuery).match(/\bTTL\b([\s\S]*?)(?:\bSETTINGS\b|;|$)/i) const raw = ttlMatch?.[1]?.trim() if (!raw) return undefined return normalizeSQLFragment(raw) diff --git a/packages/clickhouse/src/index.test.ts b/packages/clickhouse/src/index.test.ts index 22cabae..1dff8ed 100644 --- a/packages/clickhouse/src/index.test.ts +++ b/packages/clickhouse/src/index.test.ts @@ -471,6 +471,38 @@ ORDER BY a` { name: 'p_one', index: 'b', type: 'basic' }, ]) }) + + // Regression for #190: a PROJECTION whose SELECT body contains ORDER BY sits + // in the column list, before the table-level clauses. The parsers used to + // match that inner ORDER BY and swallow the engine into orderBy/primaryKey. + test('ignores clause keywords inside a projection SELECT body', () => { + const query = `CREATE TABLE bi.price_history (\`day\` Date, \`csin\` String, \`min_price\` UInt32, \`_version\` UInt64 DEFAULT now64(), PROJECTION by_csin_day (SELECT csin, day, min_price ORDER BY csin, day)) ENGINE = ReplicatedReplacingMergeTree('/clickhouse/tables/{cluster}/bi/price_history_new', '{replica}', _version) PARTITION BY toYYYYMM(day) ORDER BY (day, csin) TTL day + toIntervalYear(5) SETTINGS index_granularity = 8192, deduplicate_merge_projection_mode = 'rebuild'` + + expect(parseEngineFromCreateTableQuery(query)).toBe( + "ReplicatedReplacingMergeTree('/clickhouse/tables/{cluster}/bi/price_history_new', '{replica}', _version)" + ) + expect(parseOrderByFromCreateTableQuery(query)).toBe('(day, csin)') + expect(parsePrimaryKeyFromCreateTableQuery(query)).toBeUndefined() + expect(parsePartitionByFromCreateTableQuery(query)).toBe('toYYYYMM(day)') + expect(parseTTLFromCreateTableQuery(query)).toBe('day + toIntervalYear(5)') + expect(parseSettingsFromCreateTableQuery(query)).toEqual({ + index_granularity: '8192', + deduplicate_merge_projection_mode: "'rebuild'", + }) + expect(parseProjectionsFromCreateTableQuery(query)).toEqual([ + { name: 'by_csin_day', query: 'SELECT csin, day, min_price ORDER BY csin, day' }, + ]) + }) + + // A column-level TTL lives in the column list too, and must not be mistaken + // for the table-level TTL / SETTINGS. + test('reads table-level TTL past a column-level TTL', () => { + const query = `CREATE TABLE app.events (\`id\` UInt64, \`ts\` DateTime, \`tmp\` String TTL ts + toIntervalDay(1)) ENGINE = MergeTree ORDER BY id TTL ts + toIntervalYear(1) SETTINGS index_granularity = 8192` + + expect(parseTTLFromCreateTableQuery(query)).toBe('ts + toIntervalYear(1)') + expect(parseOrderByFromCreateTableQuery(query)).toBe('id') + expect(parseSettingsFromCreateTableQuery(query)).toEqual({ index_granularity: '8192' }) + }) }) describe('formatConnectionError', () => {