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

[alvarogar4]

Bug Description

normalizeSQLFragment only collapses whitespace runs:

export function normalizeSQLFragment(value: string): string {
  return value.replace(/\s+/g, ' ').trim()
}

ClickHouse, however, reformats expressions when it stores them — most visibly, it prints one space after every argument separator. So a schema written as cityHash64(a,b) is stored by ClickHouse as cityHash64(a, b), and because both the canonical form and the drift fingerprint run through normalizeSQLFragment, the two never compare equal.

The result is drift that reports forever and that no migration can fix — chkit generate sees no diff, because it canonicalizes both of its own sides identically.

Reproduction

CREATE TABLE default.t (a String, b String, INDEX skip_idx cityHash64(a,b) TYPE minmax GRANULARITY 1)
ENGINE = MergeTree ORDER BY a;

ClickHouse echoes it back reformatted (verified on 26.3.9.1):

INDEX skip_idx cityHash64(a, b) TYPE minmax GRANULARITY 1

With the schema declaring the un-spaced spelling:

indexes: [{ name: 'i', expression: 'cityHash64(a,b)', type: 'minmax', granularity: 1 }]

compareTableShape against the live shape returns reasonCodes: ['index_mismatch'], indexDiffs: ['i'].

Impact

• Drift never reads clean for any skip index whose expression calls a 2+ argument function without a space after the comma (cityHash64(user_id,ts), concat(a,b), substring(s,1,3)).
• chkit check fails in CI on a difference that does not exist, and chkit generate produces no migration that would resolve it. The only workaround is to hand-edit the schema to match ClickHouse's formatting.
• The same fragment normalizer backs ttl and partitionBy, so those are exposed to the same class of mismatch.

Investigation Notes

• packages/core/src/sql-normalizer.ts — normalizeSQLFragment collapses whitespace and nothing else.
• packages/core/src/canonical.ts:40 — expression: normalizeSQLFragment(index.expression).
• packages/cli/src/commands/drift/compare.ts:223 — expr=${normalizeSQLFragment(index.expression)}.

Both sides apply the same lossy normalizer, so the author's spelling survives on the expected side while ClickHouse's spelling arrives on the actual side.

Index-only projections were given a faithful normalizer in PR #193 (normalizeProjectionIndex in packages/core/src/projection.ts) that peels redundant parens and spaces argument separators, validated form-by-form against a live instance. That fix was deliberately scoped to projections: changing normalizeSQLFragment globally would alter the canonical form of every existing SQL fragment and churn users' snapshots on upgrade, which needs its own change and a migration story.

Expected Behavior

A schema fragment and the live fragment ClickHouse stores for it compare equal whenever they describe the same expression, regardless of the author's spacing.

Proposed Fix

Two options, in increasing order of ambition:

1. Extend normalizeSQLFragment to match ClickHouse's printer for the mechanical parts (space after argument separators, at minimum), reusing the quote-aware scanning already in spaceAfterCommas (packages/core/src/projection.ts). Note the blast radius: this changes the canonical form of indexes, TTL, and partitionBy, so existing snapshots would need a rewrite or a compatibility pass to avoid spurious migrations on upgrade.
2. Compare fragments semantically rather than by string, e.g. by asking ClickHouse to normalize both sides, or by parsing to an AST. Robust but a much larger change.

Whichever is chosen, normalizeProjectionIndex should collapse into the shared helper so projections and skip indexes cannot diverge.

Verification

• Unit: compareTableShape with expected cityHash64(a,b) and actual cityHash64(a, b) returns null.
• Idempotence: normalizing twice is a fixed point (this bit projections in PR fix(pull): capture index-only projections and render them correctly #193 — a non-idempotent canonical form makes every generate re-emit a drop + rebuild).
• Snapshot compatibility: confirm an existing snapshot written before the change does not produce spurious operations after it.
• E2E against a live instance: create a skip index with un-spaced arguments, pull, generate, drift → clean.

---

Found while verifying #183 / PR #193.

[alvarogar4]

Investigation update — the fix is not a string normalizer

Probed ClickHouse 26.3.9.1 to see exactly what it does to a stored expression. It runs a full expression printer, not just comma-spacing:

Written	ClickHouse stores
cityHash64(a,b)	cityHash64(a, b)
substring(s,1,3)	substring(s, 1, 3)
n+1	n + 1
a=b	a = b
n*2+1	(n * 2) + 1
(n)	n

So it also spaces operators, adds precedence parentheses, and strips redundant parens — and for TTL, it rewrites INTERVAL 5 YEAR → toIntervalYear(5). A hand-rolled string normalizer (the "targeted fragment normalizer" I suggested when filing this) cannot reproduce precedence rewriting or the INTERVAL→function rewrite; it would fix cityHash64(a,b) but still false-drift on n*2+1 and on most real TTLs.

The only correct approach: normalize through ClickHouse itself

ClickHouse's own formatter produces the canonical form, and it batches in a single query (verified live):

SELECT arrayMap(e -> formatQuerySingleLine('SELECT ' || e),
                ['cityHash64(a,b)', 'n*2+1', 'toYYYYMM(day)'])
-- ['SELECT cityHash64(a, b)', 'SELECT (n * 2) + 1', 'SELECT toYYYYMM(day)']

So the fix is drift-only (no snapshot churn): in the drift path, format the schema (expected) side's fragments through the live ClickHouse the command is already connected to, strip the SELECT prefix, and compare against the actual side (already CH-canonical, since it's parsed from create_table_query). One batched query, with graceful fallback to today's normalizeSQLFragment when offline or on a format error (a malformed fragment errors the whole batch — verified — so failures must be isolated or fall back coarsely).

Scope notes for whoever implements this

• Composes cleanly (single expressions / SELECT-wrappable): skip-index expressions, partitionBy, SELECT-projection queries (formatQuerySingleLine(query) directly), and per-element orderBy/primaryKey/uniqueKey (split on top-level comma, format each).
• Harder: full TTL grammar (multiple rules, TO VOLUME/DELETE) is not a SELECT expression, so SELECT-wrapping only handles the simple expr form; complex TTL needs its own handling or a documented fallback.
• Architecture: compareTableShape is currently pure/sync. Cleanest injection is to pre-resolve a Map<raw, canonical> in the async payload.ts (which already holds the connection) and pass a sync lookup (s) => map.get(s) ?? normalizeSQLFragment(s) into the comparer.

Confirmed the previously-fixed projection normalizer (normalizeProjectionIndex, PR #193) has the same theoretical gap (comma-spacing only, no precedence), but projection index expressions are rarely operator expressions, so it's low-risk there.

Holding a PR for now — flagged that this is meatier than a string tweak and should be weighed against #196/#197/#126 rather than done reflexively.