sqlite-utils/create-table-parser-feedback.md

Review: create-table-parser branch

Feedback on the create-table-parser branch (commits ed1de15, 95d6ff4, 407db59 — 819-line create_table_parser.py plus JSON test corpora), reviewed as the proposed foundation for 4.1 transform() improvements that make UNIQUE and CHECK constraints supported instead of silently dropped.

Verdict: good fit for 4.1 — right design, right release — but it has two silent-corruption bugs that must be fixed before transform() can trust it, and a few design decisions to settle.

Why 4.1 is the right home

• The parser is purely additive: a new module and a new capability. Nothing about it needs to land in 4.0, so it does not constrain or delay the stable release.
• The transform() improvement it enables — preserving CHECK and UNIQUE constraints instead of silently dropping them — is a behavior change in the bug-fix direction, appropriate for a minor release with a prominent changelog note.
• SQLite exposes no introspection for CHECK constraints (and only partial detail elsewhere), so parsing the sqlite_master DDL is the correct — and only — approach. The module docstring says exactly this.

What holds up well (verified empirically)

• Corpus results: all 272 statements in tests/valid_create_table.json parse without crashing; the 230 that are independently executable were cross-checked against SQLite's own PRAGMA table_xinfo — 0 column-name or primary-key mismatches. The 29 invalid statements also do not crash the parser.
• Survived adversarial probing: identifier quoting in all four styles including embedded escaped quotes ("col ""x""", [square], backticks), strings containing ,) and keywords inside CHECK expressions, col IN (...) option extraction with escaped quotes, generated columns (GENERATED ALWAYS AS ... STORED and bare AS), FK action clauses (ON DELETE SET NULL ON UPDATE CASCADE DEFERRABLE INITIALLY DEFERRED), WITHOUT ROWID, STRICT trailers, keyword-named columns ("unique"), multi-word types with parenthesized arguments, and CREATE TABLE AS SELECT.
• The design is genuinely good: recursive-descent functions mirroring the SQLite grammar are readable; the constraint list as source of truth with derived accessor properties (table.checks, col.not_null, table.primary_key) is the right model — adding a constraint kind means adding a dataclass and a property. The group-capturing tokenizer (parenthesized groups taken whole so nested commas never reach the parser) is a clean way to sidestep expression parsing.

Must-fix before transform() builds on it

1. Comments produce phantom columns (silent corruption)

The tokenizer does not handle -- line comments or /* */ block comments, and sqlite_master stores DDL verbatim. Reproduced:

CREATE TABLE t (
  a TEXT, -- user's name, (important)
  b INTEGER
)
→ columns parsed as ['a', '-']        (real: ['a', 'b'])

CREATE TABLE t (a TEXT /* legacy, do not use */, b INTEGER)
→ columns parsed as ['a', 'do', 'b']  (real: ['a', 'b'])

Hand-written schemas — exactly the ones that have CHECK constraints — are exactly the ones with comments. If transform() rebuilds a table from a model with wrong columns, that is data loss. Comment handling belongs in _tokenize / _locate_body / _split_top_level (all three scan raw text).

2. Numeric literal defaults are truncated (silent corruption)

_tokenize splits 1.5 into 1 / . / 5 and _default_value consumes one token. Reproduced:

DDL	parsed default	correct
DEFAULT 1.5	'1'	1.5
DEFAULT -1.5	'-1'	-1.5
DEFAULT 1e-3	'1e'	1e-3
DEFAULT x'0102'	'x'	x'0102'

Re-emitting these in a transform rewrites DEFAULT 1.5 as DEFAULT 1 — silent schema corruption. Notably the corpus already contains DEFAULT -45.8e22, but nothing catches this because the fixtures are input-only (see next point). Fix by lexing numeric literals (including sign, decimal point, exponent) and blob literals (x'...') as single tokens, or by making _default_value consume the full literal.

3. No tests are actually executed

The JSON corpora are not wired into pytest — there is no test_*.py on the branch, so the parser currently has zero executed tests. Beyond wiring the corpus in, add expected-output snapshots (parse each valid statement, assert the full structured result), not just "doesn't crash": the DEFAULT -45.8e22 bug sat undetected in the corpus precisely because only inputs are recorded. A cheap high-value addition: property test that for every executable statement, parsed column names / pk / not-null match PRAGMA table_xinfo.

Design decisions to settle

Discard-vs-error policy for grammar the model does not capture

The plan is presumably parse → modify model → serialize, which matches how transform() already rebuilds tables. That makes "what the model discards" the critical list, because anything discarded is silently stripped from the user's schema on transform:

• ON CONFLICT clauses on PRIMARY KEY / UNIQUE / NOT NULL (parsed by _conflict_clause, thrown away)
• FK MATCH and DEFERRABLE INITIALLY DEFERRED (parsed, thrown away — deferred FKs are real in the wild)
• ASC / DESC in table-level PRIMARY KEY (a DESC) / UNIQUE (...) column lists (_column_list_group keeps only the leading identifier; inline single-column PK order is captured)
• Unknown column constraints (_column_constraint consumes one token and returns None)

Either capture these in the model, or have transform() raise TransformError when the source DDL contains grammar it cannot round-trip — the honest-failure pattern 3.38 established for un-recreatable indexes. Silence is the one wrong answer.

Same question for column renames: a renamed column referenced inside a CHECK expression (stored as a raw string) cannot be mechanically rewritten, so transform(rename=...) intersecting a check should raise TransformError rather than emit a stale constraint. Likewise dropping a column referenced by a table-level CHECK or multi-column UNIQUE.

Ship it private in 4.1

The module defines Table, Column, and ForeignKey — the package already has db.Table and a db.ForeignKey namedtuple with different, incompatible fields ((table, column, other_table, other_column) vs (columns, table, references, ...)). Two public ForeignKey types in one package is a confusion trap, and 4.0 is a fresh reminder that public surface is forever. Recommendation: land it as a private module (sqlite_utils._parser or similar) powering transform() in 4.1, promote to documented public API in a later minor once battle-tested. Public-later is additive; public-now-retract-later is breaking.

Related naming nit: the parser's Table.schema is the attached-database name (main/temp), while db.Table.schema is the DDL string. Rename one (e.g. database or schema_name) before anything goes public.

Housekeeping

• create_table_parser.py lives at the repo root; it needs to move inside the sqlite_utils/ package.
• parse_checks is documented as a "backwards-compatible helper" but nothing in sqlite-utils exists for it to be compatible with — stale comment from an earlier draft.
• The valid corpus is strong on grammar coverage but light on real-world mess: add fixtures with comments, tabs/newlines in odd places, numeric and blob defaults, and a couple of schemas generated by sqlite-utils itself (double-quoted everything) and by common tools.

Bottom line

Hold it for 4.1 exactly as planned — it has no 4.0 dependency and should not delay the stable release. Before transform() builds on it: fix comment handling and numeric/blob literal lexing, wire the fixtures into pytest with expected-output snapshots, decide the discard-vs-error policy, and keep the module private for one release. The bones are solid; both bugs are tokenizer-level and small.