Repository navigation
feat(sqlite): table DDL — columns, keys, indexes (#50, reviewed) - #120
Merged
Merged
Conversation
… time sqliteEntitiesOf exists to catch a bad definition before SQLite runs it. It now also reports: - two columns with the same SQL name, compared without case as SQLite does (`Id` and `id`, or a `name:` that repeats another key); - a column listed twice in a primary key or an index; - a defaultExpr that reads a column (SQLite: "default value of column is not constant"); - `default: undefined` or `defaultExpr: undefined`, which made the column optional on insert while writing no DEFAULT, so the insert failed on NOT NULL. `column()` now refuses it in the type too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 59 minutes. View limit details
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 |
This was referenced Oct 25, 2026
…red key checks
- SQLite.timestampNow is a defaultExpr in the text form a timestamp
column compares with. A timestamp column refuses CURRENT_TIMESTAMP,
datetime('now') and the like as a defaultExpr: they write another form
(no milliseconds), which compares as earlier than the same instant.
- The duplicate-column-name and repeated-key checks move to problems.ts
and run for Postgres too, which had the same gap ("column specified
more than once" at run time). SQLite compares names without case.
- The first changeset no longer says SQLite DDL is not implemented
(CodeRabbit on #50).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…time formats
The guard only looked at string defaultExprs, and only at CURRENT_* and
datetime()-style calls. It now checks the rendered SQL of a DSL default
too, and refuses strftime with any format but the column's own
('%Y-%m-%d %H:%M:%f'), and julianday() (independent review).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # src/sqlite/timestamp.ts
Makisuo
added this pull request to stack #124
October 25, 2026 19:16
This was referenced Oct 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is #50 by @tao-io, with review fixes on top. Their commits are unchanged. GitHub doesn't let maintainers push to an organization-owned fork, so the fixes couldn't go onto #50 directly. It's stacked on #119 (SQLite/D1 query dialects). Refs #48.
What #50 adds
See #50:
SQLite.table(name, { columns, primaryKey, indexes })with its DDL,column/index/uniqueIndex, SQLite schema entities,sqliteEntitiesOfandrenderSqliteSchema. The shared DDL expression renderer moves toschema/ddl-expr.ts; Postgres output is unchanged (checked).What this adds
sqliteEntitiesOfexists to catch a bad definition before SQLite runs it. It now also reports these four definitions, each of which rendered fine before and then failed in SQLite:{ id, Id }, ora: column(text, { name: "b" })next tobduplicate column name(SQLite compares names without case)primaryKey: ["id", "id"], orindex(…, ["b", "b"])defaultExpr: ($) => $.bdefault value of column is not constantdefault: undefined(e.g.cond ? x : undefined)The last one is also a type error now. All four have tests (
ddl.test.ts,sqlite.test-d.ts).Verification
bun run typecheck: pass.bun run testwith ClickHouse 26.8.2.7: 65 files, 1,228 tests passed, none skipped. Export catalogs ok (167/sqlite).Left for follow-ups (from review)
sqlite-define.tscopies much ofpg-define.ts, and the same duplicate-name gap exists there for Postgres.1and(1)differ in a snapshot. The migration differ in the next PR in this stack (feat(sqlite): generate migrations #51) is where that matters.Second round (after review)
SQLite.timestampNowis adefaultExprfor atimestampcolumn that writes the column's own text form.CURRENT_TIMESTAMP,datetime('now'),julianday,unixepoch, orstrftimewith any format but'%Y-%m-%d %H:%M:%f'. The check covers string defaults and the rendered SQL of DSL defaults. Those defaults misorder against the column's values.problems.tsand now run for Postgres too, which had the same gap: "column specified more than once" at run time.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.