refactor: lift the SELECT compile pipeline out of the CLI into the library (#695) - #703
Open
dpsiderius wants to merge 2 commits into
Open
dpsiderius wants to merge 2 commits into
dpsiderius wants to merge 2 commits into
Conversation
…brary (#695) `Connection::prepare(sql)` cannot be written today. `compile_statement` handles write and DDL statements only — hand it a `SELECT` and it answers `Unrecognized("SELECT")`, which I hit for real while writing #692's tests. A `SELECT` needs its CTEs and views expanded, its FROM tables resolved and `sqlite_stat1` stats loaded first, and the pipeline that does all of that lived in `src/bin/sqlite-rs/query.rs` — inside the executable, where no library consumer can reach it. That split is a CLI implementation detail, not something a consumer should have to know about. Stock SQLite has exactly one `sqlite3_prepare_v2()`: hand it any statement, get a handle back. Spec 013's `Connection::prepare` has to behave the same way, so the missing half moves into `src/codegen/prepare.rs` where both callers can use it. Not a rewrite. `compile_select_program` and `SelectOutcome` are the CLI's own code relocated, and the 127-line local copy is deleted rather than left to drift — `query.rs` and `repl.rs` now call the library's. `derive_headers` comes along as `result_column_names`, so `Row`'s by-name access and the CLI's column headers will agree by construction instead of by coincidence. One thing changed rather than moved: every error was `.map_err(|e| e.to_string())`, flattening a structured `CodegenError` into a message. That is fine when the next step is printing to a terminal and wrong for a library, where a caller needs to match on what went wrong. Errors are now `PrepareError`, which wraps `CodegenError` and adds the one case that is about the request rather than the SQL: asking for `EXPLAIN QUERY PLAN` on a FROM-less `SELECT`, which is not a `CodegenError::NoFromClause` because `SELECT 1` compiles fine, it just has no access path to explain. `Display` reproduces the old strings exactly, so CLI output is unchanged. `from_less_schema()` stays private. The CLI inlined that 12-field literal; a public `TableSchema::none()` would be new API surface this lift does not need. The CLI behaving identically is the claim that matters here, since this is a refactor of the path every `sqlite-rs query` and REPL statement takes. Evidence: 1575 unit, 388 corpus (which includes the CLI e2e suite) and 15 sqllogictest all pass unchanged, `make lint` clean both clippy passes, and four queries plus `EXPLAIN QUERY PLAN` hand-checked byte-identical against the pinned 3.53.4 oracle — including the FROM-less EQP error, whose message survives the `String` -> `PrepareError` change intact. No `Connection` or `Statement` yet; this is the groundwork they need. PRAGMA dispatch is deliberately not lifted: spec 013's non-goals give the PRAGMA catalogue to V7, and SQE's statement list contains none. Refs: 013/Req-3, #695, #678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… it falsified (#695) The lift commit had no tests of its own. Every existing exercise of `compile_select_program` went through the `sqlite-rs` binary, so the newly-public library surface had zero direct coverage and a regression in it would only have surfaced as a CLI failure. Two files fix that. `tests/unit/prepare_test.rs` (10 tests) pins the dispatch and the error type: one case per `compile_select_program` arm (FROM-less, single-table, joined, compound), `eqp_mode` returning rows rather than a program, `PrepareError::EqpWithoutFrom` including its message being byte-identical to the string the CLI printed before the lift, an unknown table arriving as a matchable `PrepareError::Codegen` rather than a message to parse, and `result_column_names`' positional fallback for joins and compounds — asserted rather than left implicit, because `Row::get_by_name` will be built on it and `column1` is a real answer a consumer can receive. `tests/corpus/prepare_oracle_test.rs` pins the answers. Eleven queries compiled through the library and diffed against the pinned 3.53.4 oracle, on a fixture the oracle itself created — the adoption direction that matters. Shapes include SQE's own: `UNION`, `UNION ALL`, `LIMIT 1` existence probes that hit and miss, a join, aggregates. I had checked four queries by hand while doing the lift, which is worth nothing once the terminal closes; a refactor of the path every `sqlite-rs query` takes deserves a check that runs in CI. Mutation-verified that it really talks to the oracle: perturbing integer rendering fails it. `SelectOutcome` gains `derive(Debug)`. It is public and a consumer matches on it, both payloads already derive it, and without it `unwrap`/`expect_err` on a `Result<SelectOutcome, _>` will not compile — which is a papercut for every caller, not just these tests. Also corrected: `tests/performance/v6.rs`'s comment explaining why that bench duplicates the compile pipeline said the real function "is `pub(crate)` to the binary crate and not callable from an external test/bench crate". The lift made that false. The comment now says so and points at the duplication as removable — deliberately not removed here, since replacing it changes the path this bench measures and that belongs in a change whose bench numbers are the point. One test failure worth recording: `result_column_names_honours_aliases` was written as `SELECT a AS first, b AS second` and failed to parse. Not a bad test — `FIRST` is one of 89 keywords we reserve that SQLite treats as an identifier (`parse.y:272`'s `%fallback ID`). Filed as #696, which also blocks SQE outright: its `iceberg_namespace_properties` has a `key` column, and we cannot create or read that table at all. The test here uses non-keyword aliases and the keyword case is pinned in #696 rather than smuggled in. Verified: 1572 unit (1562 + 10), 388 corpus (387 + 1), 15 sqllogictest, `make lint` clean both passes, `make check-mod-files`, assurance 86/86 and 276/276 with no dead links. Refs: 013/Req-3, #695, #696 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Why
Connection::preparehas to compile any statement, and today it cannotcompile a
SELECT:compile_select_program,SelectOutcomeandderive_headerslive insrc/bin/sqlite-rs/, not in the library.repl.rs::run_one_statementis the only place that unifies the SELECT andwrite/DDL routes, and a library consumer cannot call it.
#695 sized this as the real cost of the facade — "the wrapping is the easy
half" — so it ships as its own PR.
What
src/codegen/prepare.rs.Result<_, String>— a CLI-shapederror a library API should not expose.
sqlite_stat1stats loading and view resolution thread along with it.Verification
The CLI must behave identically — that is this PR's whole risk surface,
and the existing CLI corpus tests (
cli_e2e_test,cli_write_test,repl_test) are the regression evidence. All green.make test1666 passed / 0 failed,make test-corpus404 passed / 0 failed,make lintclean on both clippy passes,make check-mod-files,make check-grammar-drift,make check-assuranceall clean. Verified from adetached clean worktree at the commit, not from the working tree.
The second commit also corrects a bench comment that this refactor
falsified — it claimed the CLI owned the pipeline.
Merge order
This must merge before #705 (the facade), which builds on it. #705
also needs #694 first.
spend: matched the estimate carried on #695 for this portion.
Refs: #695
🤖 Generated with Claude Code