Repository navigation
Derive lossless NOT NULL relaxation from durable table declarations - #2896
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
debt-src at 086cf07, every finding: |
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.
Toad saves an ACP launch before its provider handshake completes. Its provider session ID therefore widens from str to str | None, but Core's durable schema derivation refuses the earlier NOT NULL declaration even though every retained row satisfies the new declaration.
Extend the existing derivation owner: TypedTable._table_sql renders an earlier declaration's required nullable fields; derive_relaxed_fields reads column names and NOT NULL flags through one typed pragma_table_info query. It rebuilds only when columns match exactly and the stored SQL equals that exact earlier declaration. Generated fields, primary keys and all other structural differences retain the existing refusal. derive_schema invokes it immediately after derive_nullable_fields.
Add one test beside the existing nullable-field derivation test: two retained values survive a durable reconcile and a third row accepts NULL. The test was not run. The only runtime check was DB(path=COPY).prepare() on a private copy of the live toad.db, using this Core head and Toad #1162. The first attempt failed during imports because this fresh worktree lacked its pinned refactor-audit submodule; no schema changed. After initializing that pinned submodule, the copy check exited 0.
Copy: /home/ts/.cache/agent-scratch/toad-nullability.qsTIUt/toad.db (0600), cp --reflink=auto from /home/ts/.local/state/toad/toad.db; no WAL/SHM files existed beside the source at copying. The new code opened only the copy. The private ccea runtime supplied dependencies, with sys.path selecting /home/ts/wt/installer-relaxed-fields/src and /home/ts/wt/installer-toad-incoming/src.
Before (0 rows):
After (0 rows):
The source copy contains no session rows; this runtime check demonstrates the schema widening, while the added unrun test exercises preservation of nonempty rows.
Required by OpenHCSDev/toad#1162. No Toad carry tool, install hook, sentinel provider ID or second sessions table. Neither PR enables auto-merge.
Assumed: writer-owned reconciliation remains the schema-change authority; exact earlier SQL excludes every other structural change; only nullable writable non-key fields can widen; the private runtime dependencies are suitable for this schema-open check, which passed; no application restart or live installation was performed.