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', () => {