fix(drift): read skip index arguments from type_full and ignore stored parens

.changeset/drift-index-arguments.md

@@ -0,0 +1,7 @@
+---
+"@chkit/clickhouse": patch
+"@chkit/plugin-obsessiondb": patch
+"chkit": patch
+---
+
+Stop `chkit drift` from reporting `index_mismatch` for skip indexes it just created. Introspection read `system.data_skipping_indices.type`, which holds only the index name (`ngrambf_v1`), so every argument parsed as 0; it now reads `type_full` (`ngrambf_v1(3, 4096, 2, 0)`). chkit renders `INDEX name (expr)` and ClickHouse keeps those parentheses in `expr`, so the comparison now drops one pair when it encloses the whole expression. chkit-py introspection reads `type_full` as well.

chkit_python/src/chkit/cli/commands/drift_compare.py

@@ -265,10 +265,29 @@ def _render_index_type_fingerprint(index: SkipIndexDefinition) -> str:
     )
 
 
+def _strip_enclosing_parens(value: str) -> str:
+    """Drop one pair of parentheses only when it encloses the whole value.
+
+    chkit renders ``INDEX name (expr)`` and ClickHouse keeps those parentheses
+    in ``system.data_skipping_indices.expr``; ``(a) || (b)`` stays as written.
+    """
+    if not (value.startswith("(") and value.endswith(")")):
+        return value
+    depth = 0
+    for i, ch in enumerate(value):
+        if ch == "(":
+            depth += 1
+        elif ch == ")":
+            depth -= 1
+        if depth == 0 and i < len(value) - 1:
+            return value
+    return value[1:-1].strip()
+
+
 def _normalize_index_shape(index: SkipIndexDefinition) -> str:
     return "|".join(
         [
-            f"expr={normalize_sql_fragment(index.expression)}",
+            f"expr={_strip_enclosing_parens(normalize_sql_fragment(index.expression))}",
             f"type={_render_index_type_fingerprint(index)}",
             f"granularity={index.granularity}",
         ]

chkit_python/src/chkit/clickhouse/introspect.py

@@ -370,7 +370,7 @@ def list_table_details(client: Any, databases: list[str]) -> list[IntrospectedTa
         f"FROM system.columns WHERE database IN ({quoted})"
     ).rows
     index_rows_raw = client.query(
-        f"SELECT database, table, name, expr, type, granularity "
+        f"SELECT database, table, name, expr, type_full AS type, granularity "
         f"FROM system.data_skipping_indices WHERE database IN ({quoted})"
     ).rows
 

chkit_python/tests/test_drift_compare.py

@@ -249,6 +249,20 @@ def test_table_shape_detects_index_mismatch() -> None:
     assert "index_mismatch" in detail.reason_codes
 
 
+def test_table_shape_ignores_stored_enclosing_index_parens() -> None:
+    expected_idx = SkipIndexMinmax(name="idx_x", expression="lower(x)", granularity=1)
+    stored_idx = SkipIndexMinmax(name="idx_x", expression="(lower(x))", granularity=1)
+    assert compare_table_shape(_t(indexes=[expected_idx]), _it(indexes=[stored_idx])) is None
+
+
+def test_table_shape_keeps_parens_that_do_not_enclose_the_expression() -> None:
+    expected_idx = SkipIndexMinmax(name="idx_x", expression="a || b", granularity=1)
+    stored_idx = SkipIndexMinmax(name="idx_x", expression="(a) || (b)", granularity=1)
+    detail = compare_table_shape(_t(indexes=[expected_idx]), _it(indexes=[stored_idx]))
+    assert detail is not None
+    assert "index_mismatch" in detail.reason_codes
+
+
 def test_table_shape_detects_setting_mismatch() -> None:
     detail = compare_table_shape(
         _t(settings={"index_granularity": 8192}),

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

@@ -220,9 +220,23 @@ function renderIndexTypeFingerprint(index: SkipIndexDefinition): string {
   }
 }
 
+// chkit renders `INDEX name (expr)`, and ClickHouse keeps those parentheses in
+// system.data_skipping_indices.expr. Strip one pair only when it encloses the
+// whole expression, so `(a) + (b)` stays intact.
+function stripEnclosingParens(value: string): string {
+  if (!value.startsWith('(') || !value.endsWith(')')) return value
+  let depth = 0
+  for (let i = 0; i < value.length; i++) {
+    if (value[i] === '(') depth++
+    else if (value[i] === ')') depth--
+    if (depth === 0 && i < value.length - 1) return value
+  }
+  return value.slice(1, -1).trim()
+}
+
 function normalizeIndexShape(index: SkipIndexDefinition): string {
   return [
-    `expr=${normalizeSQLFragment(index.expression)}`,
+    `expr=${stripEnclosingParens(normalizeSQLFragment(index.expression))}`,
     `type=${renderIndexTypeFingerprint(index)}`,
     `granularity=${index.granularity}`,
   ].join('|')

packages/cli/src/test/drift.e2e.test.ts

@@ -25,6 +25,10 @@ function renderUniqueProjectionSchema(database: string, usersTableName: string):
   return `import { schema, table } from '${CORE_ENTRY}'\n\nconst users = table({\n  database: '${database}',\n  name: '${usersTableName}',\n  columns: [\n    { name: 'id', type: 'UInt64' },\n    { name: 'email', type: 'String' },\n  ],\n  engine: 'MergeTree()',\n  primaryKey: ['id'],\n  orderBy: ['id'],\n  uniqueKey: ['id'],\n  projections: [{ name: 'p_recent', query: 'SELECT id ORDER BY id DESC LIMIT 10' }],\n})\n\nexport default schema(users)\n`
 }
 
+function renderIndexedSchema(database: string, usersTableName: string): string {
+  return `import { schema, table } from '${CORE_ENTRY}'\n\nconst users = table({\n  database: '${database}',\n  name: '${usersTableName}',\n  columns: [\n    { name: 'id', type: 'UInt64' },\n    { name: 'email', type: 'String' },\n  ],\n  engine: 'MergeTree()',\n  primaryKey: ['id'],\n  orderBy: ['id'],\n  indexes: [\n    { name: 'idx_set', expression: 'email', type: 'set', maxRows: 0, granularity: 1 },\n    { name: 'idx_bloom', expression: 'email', type: 'bloom_filter', falsePositiveRate: 0.01, granularity: 1 },\n    { name: 'idx_ngram', expression: 'lower(email)', type: 'ngrambf_v1', ngramSize: 3, sizeBytes: 4096, hashFunctions: 2, randomSeed: 0, granularity: 1 },\n  ],\n})\n\nexport default schema(users)\n`
+}
+
 interface E2EFixture {
   dir: string
   configPath: string
@@ -334,4 +338,52 @@ describe('@chkit/cli drift depth env e2e', () => {
     },
     240_000
   )
+
+  test(
+    'reports no drift for parameterised skipping indexes right after migrate',
+    async () => {
+      const executor = createLiveExecutor(liveEnv)
+      const database = liveEnv.clickhouseDatabase
+      const journalTable = createJournalTableName('drift_index_args')
+      const cliEnv = { CHKIT_JOURNAL_TABLE: journalTable }
+      const prefix = createPrefix('drift_index_args')
+      const usersTable = `${prefix}users`
+      const fixture = await createFixture({ database, usersTableName: usersTable })
+
+      try {
+        await writeFile(fixture.schemaPath, renderIndexedSchema(database, usersTable), 'utf8')
+        const generated = runCli(fixture.dir, ['generate', '--config', fixture.configPath, '--json'], cliEnv)
+        expect(generated.exitCode).toBe(0)
+
+        const executed = await runCliWithRetry(
+          fixture.dir,
+          ['migrate', '--config', fixture.configPath, '--execute', '--json'],
+          { extraEnv: cliEnv }
+        )
+        if (executed.exitCode !== 0) {
+          throw new Error(formatTestDiagnostic('migrate --execute failed', executed))
+        }
+        await waitForTable(executor, database, usersTable)
+
+        const driftResult = runCli(
+          fixture.dir,
+          ['drift', '--config', fixture.configPath, '--table', `${database}.${usersTable}`, '--json'],
+          cliEnv
+        )
+        expect(driftResult.exitCode).toBe(0)
+        const driftPayload = JSON.parse(driftResult.stdout) as {
+          drifted: boolean
+          tableDrift: Array<{ table: string; reasonCodes: string[] }>
+        }
+        expect(driftPayload.tableDrift).toEqual([])
+        expect(driftPayload.drifted).toBe(false)
+      } finally {
+        await rm(fixture.dir, { recursive: true, force: true })
+        await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(usersTable)}`)
+        await executor.command(`DROP TABLE IF EXISTS ${quoteIdent(database)}.${quoteIdent(journalTable)}`)
+        await executor.close()
+      }
+    },
+    240_000
+  )
 })

packages/cli/src/test/drift.test.ts

@@ -245,6 +245,52 @@ describe('@chkit/cli drift comparer', () => {
     expect(result).toBeNull()
   })
 
+  // chkit renders `INDEX name (expr)` and ClickHouse keeps the parentheses in
+  // system.data_skipping_indices.expr, so a freshly applied index read back as
+  // drift.
+  test('treats a skip index as clean when ClickHouse keeps the enclosing parens', () => {
+    const expected = table({
+      database: 'app',
+      name: 'events',
+      engine: 'MergeTree()',
+      columns: [
+        { name: 'id', type: 'UInt64' },
+        { name: 'a', type: 'String' },
+        { name: 'b', type: 'String' },
+      ],
+      primaryKey: ['id'],
+      orderBy: ['id'],
+      indexes: [
+        { name: 'idx_lower', expression: 'lower(a)', type: 'ngrambf_v1', ngramSize: 3, sizeBytes: 4096, hashFunctions: 2, randomSeed: 0, granularity: 1 },
+        { name: 'idx_sum', expression: '(a) || (b)', type: 'set', maxRows: 0, granularity: 1 },
+      ],
+    })
+
+    const liveLower = { name: 'idx_lower', expression: '(lower(a))', type: 'ngrambf_v1' as const, ngramSize: 3, sizeBytes: 4096, hashFunctions: 2, randomSeed: 0, granularity: 1 }
+    const liveConcat = { name: 'idx_sum', expression: '((a) || (b))', type: 'set' as const, maxRows: 0, granularity: 1 }
+    const actual = {
+      engine: 'MergeTree()',
+      primaryKey: '(id)',
+      orderBy: '(id)',
+      columns: [
+        { name: 'id', type: 'UInt64' },
+        { name: 'a', type: 'String' },
+        { name: 'b', type: 'String' },
+      ],
+      settings: {},
+      indexes: [liveLower, liveConcat],
+      projections: [],
+    }
+
+    expect(compareTableShape(expected, actual)).toBeNull()
+    expect(
+      compareTableShape(expected, {
+        ...actual,
+        indexes: [liveLower, { ...liveConcat, expression: '(a) || (b) || (a)' }],
+      })?.reasonCodes
+    ).toContain('index_mismatch')
+  })
+
   test('reports projection_mismatch when an index projection changes type', () => {
     const expected = table({
       database: 'app',

packages/clickhouse/src/index.ts

@@ -887,7 +887,7 @@ FROM system.columns
 WHERE database IN (${quotedDatabases})`,
 			)
 			const indexes = await this.query<SystemSkippingIndexRow>(
-				`SELECT database, table, name, expr, type, granularity
+				`SELECT database, table, name, expr, type_full AS type, granularity
 FROM system.data_skipping_indices
 WHERE database IN (${quotedDatabases})`,
 			)

packages/plugin-obsessiondb/src/query/remote-executor.ts

@@ -227,7 +227,7 @@ WHERE is_temporary = 0
 					`SELECT database, \`table\`, name, type, default_kind, default_expression, comment, position FROM system.columns WHERE database IN (${quoted})`,
 				),
 				executor.query<SystemSkippingIndexRow>(
-					`SELECT database, \`table\`, name, expr, type, granularity FROM system.data_skipping_indices WHERE database IN (${quoted})`,
+					`SELECT database, \`table\`, name, expr, type_full AS type, granularity FROM system.data_skipping_indices WHERE database IN (${quoted})`,
 				),
 			])