Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
… limit A single UPDATE with one 4-term AND predicate per pending chunk exceeds SQLITE_MAX_EXPR_DEPTH above ~995 deletions. Delete in batches of 200 and keep the pending-row match semantics. Add a 1200-row regression test. Signed-off-by: MbappeWU <MbappeWU@users.noreply.github.com>
3be6c4a to
b18b506
Compare
MbappeWU
changed the title
|
|
1 similar comment
|
|
|
I have read the CLA Document and I hereby sign the CLA |
|
@CLAassistant check |
|
Thanks, @MbappeWU, for identifying the SQLite expression-depth failure and contributing the batching fix and regression test! We cherry-picked your commit into #1865, preserving your authorship and sign-off, and added real SQLite integration coverage for large deletes, generation guards, and project isolation. Closing this PR in favor of #1865, which is approved and passing CI. Thank you for the contribution! |
…1865) ## Why Two reported SQLite failures block reindexing or startup: deleting a large chunk manifest exceeds SQLite's expression-depth limit (#1850), and an interrupted script-ngram migration leaves a scratch table that prevents the next startup (#1853). Both reproduce on current main with the contributor's regression tests. Fixes #1850. Fixes #1853. ## What Changed - Cherry-pick @MbappeWU's fixes from #1857 and #1859, retaining authorship and sign-offs. - Batch pending-vector deletion predicates and row-id deletes in groups of 200. - Drop a leftover `search_index_rebuilt` scratch table before rebuilding the intact source index. - Add real SQLite integration tests covering a 1,201-record delete, protected generations/project partitions, repeated deletion, and upgrade/downgrade retries with empty or populated scratch tables. ## Implementation Details Deletion retains the existing project, key, source-hash and pending-status guards in one transaction. Batching limits expression depth and bound parameters without changing the concurrency protocol. The migration rebuilds from `search_index`; the scratch table has no authoritative state. Integration tests commit the interrupted state before opening a new transaction and verify preserved full-text matches afterward. They use Alembic's real Operations context and sqlite-vec without application mocks. Contributor workflow-evidence notes are omitted from the final diff. ## Testing - Before fixes: the two new contributor regressions fail on main with `Expression tree is too large` and `table search_index_rebuilt already exists`. - `uv run pytest tests/repository/test_sqlite_vector_search_repository.py tests/test_script_ngram_search_migration.py test-int/test_sqlite_index_recovery.py -q -o addopts= --tb=short`: 46 passed. - `just fast-check`: lint, format and full typecheck passed. - `just doctor`: sandboxed write, index and search passed. - `git diff --check`: passed. ## Risks / Follow-ups Changes are SQLite-specific; PostgreSQL production paths are unchanged. The recovery assumes the original FTS table remains intact, matching the reported interrupted upgrade. #1861's watcher lifecycle changes are reviewed separately. --------- Signed-off-by: MbappeWU <MbappeWU@users.noreply.github.com> Signed-off-by: phernandez <paul@basicmachines.co> Co-authored-by: MbappeWU <MbappeWU@users.noreply.github.com>
|
Thanks @phernandez — glad to see this landed via #1865. Appreciate the cherry-pick and authorship credit. |
Fixes #1850.
Summary
SQLiteVecIndex.delete()built oneUPDATE … WHERE project_id AND (p0 OR p1 OR …)statement with a 4-term AND predicate per pending chunk. SQLite'sSQLITE_MAX_EXPR_DEPTH(1000) fails above ~995 deletions.This batches the OR-chain work (
_DELETE_BATCH_SIZE = 200), keeps the pending-row match semantics, and deletes embeddings/chunks from the collected rowids afterward.Tests
pytest tests/repository/test_sqlite_vector_search_repository.py::test_sqlite_vec_delete_batches_above_expression_depth_limit tests/repository/test_sqlite_vector_search_repository.py::test_sqlite_vec_delete_requires_pending_source_generation -q→ 2 passed (includes a new 1200-row regression)