Skip to content

fix(drift): compare SQL expressions in ClickHouse's canonical form#201

Open
alvarogar4 wants to merge 2 commits into
mainfrom
alvaro/num-7479-drift-ch-canonical-expressions
Open

fix(drift): compare SQL expressions in ClickHouse's canonical form#201
alvarogar4 wants to merge 2 commits into
mainfrom
alvaro/num-7479-drift-ch-canonical-expressions

Conversation

@alvarogar4

Copy link
Copy Markdown
Member

Summary

Fixes #195. chkit drift reported permanent, unfixable drift whenever a schema expression was spelled differently from how ClickHouse stores it. ClickHouse runs a full expression printer on the way in — it spaces argument separators and operators, adds precedence parentheses (n*2+1(n * 2) + 1), and rewrites INTERVAL 5 YEARtoIntervalYear(5). So a skip index / PARTITION BY / ORDER BY / TTL written cityHash64(a,b) is stored cityHash64(a, b), and chkit check failed in CI on a difference no migration could resolve.

No local string normalizer can reproduce ClickHouse's precedence/INTERVAL rewriting, so only ClickHouse can produce the canonical form. drift now normalizes expressions through the server's own formatter.

How it works

  • The drift command collects every SQL fragment across the compared tables — index expressions, TTL, key/partition clause elements, and SELECT-projection queries — at the exact granularity they're compared.
  • It formats them all in one batched round-trip via formatQuerySingleLineOrNull, which returns NULL for a fragment it can't parse instead of failing the whole batch.
  • A map-backed SqlCanonicalizer is injected into compareTableShape. Each field falls back to the existing string normalization when a fragment isn't in the map — an older server without the function, a parse failure, or offline — so behavior is never worse than today.
  • compareTableShape stays pure and synchronous; the ClickHouse call lives in the async payload builder. New @chkit/clickhouse export: canonicalizeSqlFragments.

Test plan

  • Unit: a skip-index expression differing only in comma spacing reads clean with the canonicalizer and drifts without it; a genuinely different expression (sipHash64 vs cityHash64) still drifts; collectTableSqlFragments gathers the right fragments at the right granularity
  • canonicalizeSqlFragments unit tests (mock executor): SELECT-prefix stripping, no-wrap query mode, NULL omission, quote/backslash escaping, empty-input short-circuit
  • Mutation-checked — making the comparer ignore the canonicalizer fails the reads-clean test
  • Offline path preserved: existing drift suite green unchanged (the fallback path)
  • Full suites green: core (296), clickhouse (36), cli (265), plugin-pull (26), plugin-obsessiondb (149); typecheck/lint/docs build clean
  • Live e2e against ClickHouse 26.3 through the real CLI: a table whose skip index is written cityHash64(a,b) and TTL INTERVAL 30 DAY — both stored reformatted by ClickHouse — now reports Drifted: no (was index_mismatch + ttl_mismatch)

Scope

Covers the fragment types the issue named (skip indexes, TTL, PARTITION BY) plus key clauses and SELECT-projection queries. The canonical snapshot form is left untouched — this is a drift-comparison change only, so no existing snapshots churn on upgrade. Index-only projection expressions keep their existing normalizer (already comma-spaced; operator expressions there are rare).

🤖 Generated with Claude Code

https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu

alvarogar4 and others added 2 commits July 20, 2026 13:57
ClickHouse reformats expressions when it stores them — spacing argument
separators and operators, adding precedence parens, rewriting
`INTERVAL 5 YEAR` to `toIntervalYear(5)`. So a skip index, PARTITION BY,
ORDER BY, or TTL written `cityHash64(a,b)` is stored `cityHash64(a, b)` and
chkit drift reported permanent drift no migration could fix (#195). No local
string normalizer can reproduce that rewriting.

drift now normalizes expressions through ClickHouse's own formatter. The
drift command collects every fragment across the compared tables, formats
them in one batched `formatQuerySingleLineOrNull` round-trip (which yields
NULL per unparseable fragment instead of failing the batch), and injects a
map-backed canonicalizer into compareTableShape. Each field falls back to
the existing string normalization when the fragment isn't in the map — an
older server, a parse failure, or offline — so behavior is never worse than
before. compareTableShape stays pure and synchronous; the ClickHouse call
lives in the async payload builder.

Adds @chkit/clickhouse `canonicalizeSqlFragments`.

Closes #195

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu
Review follow-up. canonicalizeSqlFragments inlined every fragment across all
compared tables into one query. On a large schema the string-literal array
could exceed ClickHouse's default max_query_size (256 KiB), throw, and — since
buildSqlCanonicalizer swallows the error and falls back — silently revert the
whole drift run to string comparison, no-opping the feature exactly where it
matters. Fragments are now split into batches bounded well under the limit,
each run as its own query and merged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu
@alvarogar4
alvarogar4 force-pushed the alvaro/num-7479-drift-ch-canonical-expressions branch from 2a04fa0 to 016a776 Compare July 20, 2026 15:59
@alvarogar4

Copy link
Copy Markdown
Member Author

Ran a high-effort adversarial review on this PR. It surfaced one real robustness gap, now fixed in the latest commit:

Batching (fixed). canonicalizeSqlFragments inlined every fragment across all compared tables into a single query. On a large schema the string-literal array could exceed ClickHouse's default max_query_size (256 KiB), throw, and — because the error is caught and falls back — silently revert the whole drift run to string comparison, no-opping the fix precisely on the large schemas that need it. Fragments are now split into batches bounded well under the limit (128 KB of literals each), run as separate queries and merged. New test forces multiple batches and asserts each query stays bounded while all fragments still map.

The review's other two flags were verified and refuted: the collector/comparer are in exact lockstep today (no silent no-op), and the swallow-and-fall-back is intended, documented graceful degradation. One low-severity efficiency note (fragments normalized twice per run) I left as-is deliberately — it keeps compareTableShape pure and the collector separate, and the cost is a cheap regex on short strings.

The sibling PRs got the same scrutiny: #200 was fuzzed old-vs-new (~1M inputs/function) and found behavior-preserving with no defects; #199 is a small denylist with a regression + mutation test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: SQL fragments are not normalized to ClickHouse's formatting, so argument spacing causes permanent false drift

1 participant