Repository navigation
Conversation
…atabase Adopting effect-orm on a database that already has tables meant writing every CH.table by hand before the first typed query. `pull` reads the database the config's driver connects to and writes them. - migrate/introspect.ts reads system.tables, system.columns and system.data_skipping_indices into entities, written the way a table definition produces them. Keys come from engine_full as declared; a Replicated or Shared engine becomes its plain family. - kit/codegen.ts writes one CH.table per table. Column types map to the built-in that writes the same SQL, else CH.custom with a fitting schema, else CH.untyped. Views and non-MergeTree tables are external; materialized views, SAMPLE BY and EPHEMERAL columns are listed for review. - `effect-orm pull [--out] [--tables] [--force]`, and `pull` from /kit. - verify reads sorting and primary keys from engine_full. sorting_key has a VersionedCollapsingMergeTree's version appended, so such a table always showed drift. Tested offline by generating from a catalog captured from ClickHouse 26.2 and checking entitiesOf on the committed output equals the input, and live: plain DDL, pull, generate --baseline, migrate, then generate finds nothing and verify reports only the views. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Maple review🟡 Confidence 6/10 · needs attention
Findings🟠 Warning · F1 · An index name that is not a plain identifier makes the pulled module fail
|
| Change | Kind | Observable | Evidence |
|---|---|---|---|
| effect-orm pull CLI command | cli | no | No tracer, OTel dependency or span helper anywhere in src, as in the existing generate/migrate/status commands; nothing for this diff to join. |
6792323 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely identifies the main change: adding effect-orm pull to generate definitions from an existing ClickHouse database. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 9… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
| if (indexes.length > 0) { | ||
| l.push("\tindexes: [") | ||
| for (const index of indexes) { | ||
| l.push(`\t\tCH.index(${str(index.name)}, ${str(index.expr)}, ${str(index.type)}, ${index.granularity}),`) |
There was a problem hiding this comment.
Warning
An index name that is not a plain identifier makes the pulled module fail generate --baseline
F1 · Warning · correctness
introspect routes a non-plain table or column name to external: true (introspect.ts:248), but it never checks index names, and buildTable rejects one: checkIdentifier(problems, ${name} index, spec.name) at define.ts:361. Pulling a table with INDEX `idx-a` ... therefore writes a modeled table whose module fails entitiesOf, and the effect-orm generate --baseline the CLI tells the user to run next dies with t index: "idx-a" is not a plain identifier ([A-Za-z_][A-Za-z0-9_]*), which no edit of the generated file can fix while keeping the index.
In `tableCode`, skip an index whose name fails `IDENTIFIER` from `../schema/problems` and push a line to `out.todos` (they are printed in the header and returned in `PullResult.review`), the way `SAMPLE BY` and `EPHEMERAL` are reported; or add the index names to `unplain` in introspect.ts:248 so the table is written `external: true`.
🤖 Prompt to fix with an AI agent
In `src/kit/codegen.ts:227`: An index name that is not a plain identifier makes the pulled module fail `generate --baseline`.
`introspect` routes a non-plain table or column name to `external: true` (introspect.ts:248), but it never checks index names, and `buildTable` rejects one: `checkIdentifier(problems, `${name} index`, spec.name)` at define.ts:361. Pulling a table with ``INDEX `idx-a` ...`` therefore writes a *modeled* table whose module fails `entitiesOf`, and the `effect-orm generate --baseline` the CLI tells the user to run next dies with `t index: "idx-a" is not a plain identifier ([A-Za-z_][A-Za-z0-9_]*)`, which no edit of the generated file can fix while keeping the index.
Suggested fix: In `tableCode`, skip an index whose name fails `IDENTIFIER` from `../schema/problems` and push a line to `out.todos` (they are printed in the header and returned in `PullResult.review`), the way `SAMPLE BY` and `EPHEMERAL` are reported; or add the index names to `unplain` in introspect.ts:248 so the table is written `external: true`.
Verify the problem exists at that location before changing it, and keep the fix to those lines.
- External tables keep their columns' DEFAULT / MATERIALIZED / ALIAS kinds, so inserts may leave out what the server fills and never write computed columns. - A Decimal wider than 15 digits is flagged for review: it decodes to a number, which rounds. - A MergeTree in the old MergeTree(date, key, granularity) syntax is written as external rather than without its ORDER BY. - Projections and constraints are noted instead of dropped silently. - An EPHEMERAL column no longer decides whether a table's names are plain. - pull defaults --out to a one-module schema array, and refuses an empty --tables. - One stripDatabase for pull and verify, which no longer strips `db.` out of `my-db.`; one io helper; verify parses each engine_full once; columns and indexes are grouped by table once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Maple review🟢 Confidence 7/10 · likely safe to merge Adds
Still open from earlier reviews
What was checked
Observability coverage: 0 of 2 changes observable
|
Pulling ClickHouse's public playground as its read-only user failed outright: it may read system.tables and system.columns but not system.data_skipping_indices. pull now writes the tables without indexes and says why in the review list. verify still fails there, since skipping the check would hide drift. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Maple review🟢 Confidence 8/10 · likely safe to merge This head lets
Still open from earlier reviews
What was checked
|
1 participant
Why
Adopting effect-orm on a database that already has tables meant hand-writing every
CH.tablebefore the first typed query.pullwrites them from the live database.What
src/migrate/introspect.tsreadssystem.tables,system.columnsandsystem.data_skipping_indicesinto entities, written the way atabledefinition produces them. Keys come fromengine_fullas declared. AReplicated*/Shared*engine becomes its plain family, because replication is a render option.src/kit/codegen.tswrites oneCH.tableper table: engine and arguments, keys, partition key, TTL, settings, comments,DEFAULT/MATERIALIZED/ALIAS, codecs and skipping indexes. Expressions are kept as the server's text.CH.customwith a fitting schema (UUID,Date,Decimal,Enum8→ its labels, …), orCH.untypedas a last resort.external: true.SAMPLE BYandEPHEMERALcolumns are listed in the file header for review.effect-orm pull.pullis also exported from/kit.verifyfix: sorting and primary keys are now read fromengine_full. It comparedsorting_key, which has aVersionedCollapsingMergeTree's version column appended, so those tables always showed drift.Postgres is not covered yet.
Testing
src/kit/__fixtures__/pulled-clickhouse.ts, which is committed sotscchecks generated code.entitiesOfon that module must equal the input entities.tests/pull.clickhouse.test.ts): plain DDL →pull→generate --baseline→migrate. Thengeneratefinds nothing to do, andverifyreports only the view and materialized view as unexpected.typecheck,bun run test(728 passed), and the live suite against ClickHouse 26.2 (303 passed) all pass.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit
New Features
effect-orm pullfor ClickHouse databases. It generates schema definitions from existing tables, supports selecting tables and choosing an output file, and protects existing files from being overwritten unless requested.Bug Fixes