Skip to content

fix(pull): capture index-only projections and render them correctly#193

Merged
alvarogar4 merged 2 commits into
mainfrom
alvaro/num-7479-chkit-pull-drops-table-projections-index-only-type-basic-not
Jul 17, 2026
Merged

fix(pull): capture index-only projections and render them correctly#193
alvarogar4 merged 2 commits into
mainfrom
alvaro/num-7479-chkit-pull-drops-table-projections-index-only-type-basic-not

Conversation

@alvarogar4

Copy link
Copy Markdown
Member

Summary

Fixes #183. chkit pull parsed only the SELECT form of a PROJECTION, so an index-only projection (PROJECTION p INDEX (a, b) TYPE basic) never matched the parser regex and was skipped. A pulled schema then recreated the table without it — silently dropping the reverse-lookup pruning — and drift never read clean. The DSL couldn't express one either: ProjectionDefinition was { name, query } and the renderer always wrapped the body in parens, which is invalid for the index-only form.

One scope correction to the issue: SELECT-style projections were already pulled correctly. Verified against live ClickHouse — only index-only projections were dropped, which is why the reporter's table (whose sole projection is index-only) appeared to lose projections entirely.

  • coreProjectionDefinition becomes a union of the existing { name, query } and a new { name, index, type }. Existing schemas are unaffected; the renderer branches and emits the index-only form without wrapping parens.
  • core — index expressions are normalized the way ClickHouse normalizes them: a single expression bare (INDEX b), several as a tuple (INDEX (a, b)). This matters beyond cosmetics — ClickHouse rewrites INDEX (b) to INDEX b and rejects INDEX a, b with a syntax error, so without normalization a pulled schema would drift against its own source table forever.
  • clickhouse — parse the index-only form out of create_table_query (system.projections is not readable by the default ObsessionDB user, so parsing stays the only viable source).
  • pull / drift — render and fingerprint both projection kinds.

Test plan

  • typecheck + lint clean across all packages
  • Unit tests for both renderer forms, the ClickHouse normalization matrix, planner add/drop, parser (fixture is verbatim SHOW CREATE TABLE output from a live 26.3 instance), pull rendering, and drift equivalence/mismatch
  • New live E2E round-trips an index-only projection: pull → recreate → introspect, asserting the recreated table's projections match the source
  • Mutation-checked — reverting the normalization fails 5 tests, disabling the parser branch fails 2
  • Full suite green in every touched package (792 tests). plugin-backfill has 14 pre-existing failures (missing chunking fixture table) that reproduce identically on clean main
  • End-to-end against live ClickHouse 26.3 using the issue's exact repro table: pull captures the projection, generate emits it, migrate --apply reproduces it byte-for-byte, and drift reports no projection_mismatch

Note for reviewers

While verifying, I found a separate pre-existing bug, left unfixed to keep this PR scoped: a pulled table whose PRIMARY KEY is derived from ORDER BY always reports primary_key_mismatch on drift. It reproduces on a plain MergeTree table with no projections at all, so it is independent of this change — likely the same area as #190.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu

alvarogar4 and others added 2 commits July 17, 2026 12:40
`chkit pull` parsed only the SELECT form of a PROJECTION, so an index-only
projection (`PROJECTION p INDEX (a, b) TYPE basic`) never matched and was
skipped. A pulled schema then recreated the table without it, silently
dropping the pruning it provided, and drift never read clean.

The DSL could not express one either: `ProjectionDefinition` was
`{ name, query }` and the renderer always wrapped the body in parens, which
is invalid for the index-only form.

- core: `ProjectionDefinition` becomes a union of the existing
  `{ name, query }` and a new `{ name, index, type }`; the renderer branches
  on it and emits the index-only form without wrapping parens.
- core: normalize index expressions the way ClickHouse does — a single
  expression bare, several as a tuple. ClickHouse rewrites `INDEX (b)` to
  `INDEX b` and rejects `INDEX a, b`, so without this a pulled schema drifts
  against its own source table forever.
- clickhouse: parse the index-only form out of create_table_query.
- pull/drift: render and fingerprint both kinds.

Verified end to end against live ClickHouse 26.3: pull captures the
projection, generate emits it, migrate reproduces it byte-for-byte, and
drift reports no projection_mismatch.

Closes #183

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu
Follow-up from review of the index-only projection support. Three real
defects, each verified against live ClickHouse 26.3.

Normalization did not match what ClickHouse actually stores:

- It stripped only one layer of parens, so `((a,b))` normalized to `(a,b)`
  on the first pass and `(a, b)` on the second. Since the index is
  normalized at canonicalize time and again at render time, that
  non-idempotence made every `generate` re-emit a drop + rebuild of the
  projection. Peel every layer instead.
- It only peeled at the top level, but ClickHouse reports `(a, (b))` back
  as `(a, b)` while keeping a genuine nested tuple `(a, (b, c))`. Peel each
  element recursively.
- It left nested commas alone, so `concat(a,b)` stayed put while ClickHouse
  stores `concat(a, b)` — permanent drift no migration could fix. Space
  after every argument separator.

All 18 forms are now checked against a live instance: what chkit emits is
byte-for-byte what ClickHouse stores back.

Two silent failures are now validation errors:

- `projection_ambiguous_kind` — an entry setting both `query` and `index`
  satisfies the union, so TypeScript admits it; it rendered as index-only
  and discarded the SELECT body with no warning.
- `projection_empty_index` — an empty index rendered `INDEX  TYPE basic`,
  failing at migrate time with an opaque ClickHouse syntax error.

Also make stripWrappingParens quote-aware, matching splitTopLevelComma: a
paren inside a quoted identifier is text, not nesting.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Biy2vG7iixRiBsBBsFg5Nu
@alvarogar4

Copy link
Copy Markdown
Member Author

Pushed 9fe24fe after an adversarial review pass. It found three real defects in the first commit that my own testing missed — all now fixed and re-verified against live ClickHouse 26.3.

The normalization did not match what ClickHouse actually stores:

Input Was ClickHouse stores
((a,b)) (a,b), then (a, b) on re-normalize (a, b)
(a, (b)) (a, (b)) (a, b)
concat(a,b) concat(a,b) concat(a, b)

The first was the worst: the index is normalized at canonicalize time and at render time, so a non-idempotent form made every generate re-emit a drop + rebuild of the projection. My original idempotency check missed it because I happened to test ((a, b)) with a space, which normalizes correctly on the first pass. The others produced permanent drift no migration could fix.

Normalization now peels every paren layer, recurses into each tuple element (while preserving a genuine nested tuple like (a, (b, c))), and spaces argument separators. All 18 forms are now asserted against a live instance — for each one I create a table with what chkit emits and confirm ClickHouse echoes back exactly that. A second generate against the issue's repro table now yields Operations: 0.

Two silent failures are now validation errors:

  • projection_ambiguous_kind — an entry setting both query and index satisfies the union, so TypeScript admits it. It rendered as index-only and silently discarded the SELECT body — a plausible mistake when converting a projection from SELECT to index-only.
  • projection_empty_index — an empty index emitted INDEX TYPE basic, failing at migrate time with an opaque syntax error.

Deliberately not fixed (out of scope, both pre-existing and reproducible on main):

  • normalizeSQLFragment only collapses whitespace, so the same comma-spacing drift affects skip indexes (cityHash64(a,b)). Fixing it globally would change the canonical form of every SQL fragment and churn existing users' snapshots — worth its own ticket.
  • extractCreateTableBody tracks ' and " but not backticks, so a backtick identifier containing a paren truncates the parsed body. Pathological, and untouched by this PR.
  • plugin-pull has its own normalizeWrappedTuple duplicating the paren-stripping. It serves key clauses, not projections, and overlaps the primary_key_mismatch bug noted above — left alone.

@alvarogar4

Copy link
Copy Markdown
Member Author

Filed the deferred findings as issues so they don't get lost with this branch:

Each has a minimal repro verified against live ClickHouse 26.3.9.1. #197 notes that its fix is best sequenced with #194/#190, since it touches the key-clause parsing path.

@alvarogar4
alvarogar4 merged commit 3f1db03 into main Jul 17, 2026
6 checks passed
@alvarogar4
alvarogar4 deleted the alvaro/num-7479-chkit-pull-drops-table-projections-index-only-type-basic-not branch July 17, 2026 14:49
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: chkit pull drops PROJECTIONs; core DSL can't represent index-only (TYPE basic) projections

1 participant