From 450fdb01d3c40646ec4c8e642ff38286748594d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Victor=20Ara=C3=BAjo?= Date: Thu, 24 Sep 2026 21:16:51 -0300 Subject: [PATCH 1/2] fix(drift): read skip index arguments from type_full and ignore stored parens system.data_skipping_indices.type holds only the index name, so introspection parsed every argument of set, bloom_filter, tokenbf_v1, and ngrambf_v1 as 0 and drift reported index_mismatch right after migrate. type_full carries the arguments. chkit renders INDEX name (expr), and ClickHouse keeps those parentheses in expr, so the comparison also drops one pair when it encloses the whole expression. --- .changeset/drift-index-arguments.md | 7 +++ .../src/chkit/clickhouse/introspect.py | 2 +- packages/cli/src/commands/drift/compare.ts | 16 +++++- packages/cli/src/test/drift.e2e.test.ts | 52 +++++++++++++++++++ packages/cli/src/test/drift.test.ts | 46 ++++++++++++++++ packages/clickhouse/src/index.ts | 2 +- .../src/query/remote-executor.ts | 2 +- 7 files changed, 123 insertions(+), 4 deletions(-) create mode 100644 .changeset/drift-index-arguments.md diff --git a/.changeset/drift-index-arguments.md b/.changeset/drift-index-arguments.md new file mode 100644 index 00000000..9e051089 --- /dev/null +++ b/.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. diff --git a/chkit_python/src/chkit/clickhouse/introspect.py b/chkit_python/src/chkit/clickhouse/introspect.py index 2af4d32d..1d05d8aa 100644 --- a/chkit_python/src/chkit/clickhouse/introspect.py +++ b/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 diff --git a/packages/cli/src/commands/drift/compare.ts b/packages/cli/src/commands/drift/compare.ts index 43520b16..009fae9a 100644 --- a/packages/cli/src/commands/drift/compare.ts +++ b/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('|') diff --git a/packages/cli/src/test/drift.e2e.test.ts b/packages/cli/src/test/drift.e2e.test.ts index 5b9b8da2..971d92f8 100644 --- a/packages/cli/src/test/drift.e2e.test.ts +++ b/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 + ) }) diff --git a/packages/cli/src/test/drift.test.ts b/packages/cli/src/test/drift.test.ts index d9afe820..cc74dd5e 100644 --- a/packages/cli/src/test/drift.test.ts +++ b/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', diff --git a/packages/clickhouse/src/index.ts b/packages/clickhouse/src/index.ts index 8a20c013..bc3376fd 100644 --- a/packages/clickhouse/src/index.ts +++ b/packages/clickhouse/src/index.ts @@ -887,7 +887,7 @@ FROM system.columns WHERE database IN (${quotedDatabases})`, ) const indexes = await this.query( - `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})`, ) diff --git a/packages/plugin-obsessiondb/src/query/remote-executor.ts b/packages/plugin-obsessiondb/src/query/remote-executor.ts index 5ebac942..222f7c83 100644 --- a/packages/plugin-obsessiondb/src/query/remote-executor.ts +++ b/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( - `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})`, ), ]) From c6625d95e33920a22afae0865121b74edf1e911b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Victor=20Ara=C3=BAjo?= Date: Thu, 24 Sep 2026 21:35:10 -0300 Subject: [PATCH 2/2] fix(drift): ignore stored index parens in chkit-py as well The TypeScript comparison already drops one pair of parentheses that encloses the whole index expression. chkit-py still compared the stored `(lower(x))` against `lower(x)`, so `chkit drift --live` reported index_mismatch for every skip index right after migrate. --- .../src/chkit/cli/commands/drift_compare.py | 21 ++++++++++++++++++- chkit_python/tests/test_drift_compare.py | 14 +++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/chkit_python/src/chkit/cli/commands/drift_compare.py b/chkit_python/src/chkit/cli/commands/drift_compare.py index 96b68378..2eacc443 100644 --- a/chkit_python/src/chkit/cli/commands/drift_compare.py +++ b/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}", ] diff --git a/chkit_python/tests/test_drift_compare.py b/chkit_python/tests/test_drift_compare.py index c6b92ed5..b7ce18a0 100644 --- a/chkit_python/tests/test_drift_compare.py +++ b/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}),