fix(pull,drift): parse clauses past column list; treat derived primary key as clean

.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).

packages/cli/src/commands/drift/compare.ts

@@ -250,7 +250,7 @@
   return coreNormalizeEngine(normalizeSQLFragment(value)).toLowerCase()
 }
 
 export function compareTableShape(expected: TableDefinition, actual: ActualTableShape): TableDriftDetail | null {
   const columnDiff = diffByName(
     expected.columns,
     actual.columns,
@@ -274,8 +274,14 @@
   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)

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

packages/clickhouse/src/create-table-parser.ts

@@ -10,17 +10,28 @@
   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 @@
     }
     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<string, string> {
   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 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)

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