Conversation
…n SQL migrations for the builder - use the sql_builder instead of hardcoding db connectivity on the search
First round of advanced search on ducpy
There was a problem hiding this comment.
Pull request overview
Updates the DUC schema’s FTS5 configuration and introduces a Python-side element search API (with tests), while bumping the schema version and adding a migration to rebuild the FTS tables.
Changes:
- Improve FTS5 tables (unicode61 diacritic removal + prefix indexes) and add rebuild/backfill statements.
- Add schema migration
3000002 → 3000003to recreate FTS tables with the new options. - Add
ducpy.searchmodule (search_duc_elements) plus tests, and makeDucSQLauto-apply migrations on open.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/search.sql | Updates FTS5 table definitions (tokenizer/prefix) and adds rebuild statements. |
| schema/migrations/3000002_to_3000003.sql | New migration to drop/recreate FTS5 tables and rebuild indexes; sets user_version. |
| schema/duc.sql | Bumps PRAGMA user_version to 3000003. |
| packages/ducrs/src/api/version_control.rs | Removes an outdated capability bullet from module docs. |
| packages/ducpy/src/ducpy/builders/sql_builder.py | Adds migration discovery/application when opening DBs. |
| packages/ducpy/src/ducpy/search/search_elements.py | New Python search implementation + scoring/grouping logic. |
| packages/ducpy/src/ducpy/search/init.py | Exposes the new search API via package exports. |
| packages/ducpy/src/ducpy/init.py | Re-exports search helpers at top-level ducpy. |
| packages/ducpy/src/tests/src/test_search_module.py | Adds tests covering search behavior and grouping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from .search_elements import (DucElementSearchResult, DucFileSearchResult, | ||
| DucSearchResponse, DucSearchResult, | ||
| search_duc_elements) |
There was a problem hiding this comment.
The multi-line import formatting/indentation is currently misaligned, which makes it harder to read and can cause formatter churn. Reformat the import block (e.g., one name per line aligned within parentheses) for consistent readability.
| from .search_elements import (DucElementSearchResult, DucFileSearchResult, | |
| DucSearchResponse, DucSearchResult, | |
| search_duc_elements) | |
| from .search_elements import ( | |
| DucElementSearchResult, | |
| DucFileSearchResult, | |
| DucSearchResponse, | |
| DucSearchResult, | |
| search_duc_elements, | |
| ) |
| CREATE TRIGGER trg_elements_ai AFTER INSERT ON elements BEGIN | ||
| INSERT INTO search_elements(rowid, label, description) | ||
| VALUES (NEW.rowid, NEW.label, NEW.description); | ||
| END; | ||
| CREATE TRIGGER trg_elements_ad AFTER DELETE ON elements BEGIN | ||
| INSERT INTO search_elements(search_elements, rowid, label, description) | ||
| VALUES ('delete', OLD.rowid, OLD.label, OLD.description); | ||
| END; | ||
| CREATE TRIGGER trg_elements_au AFTER UPDATE OF label, description ON elements BEGIN | ||
| INSERT INTO search_elements(search_elements, rowid, label, description) |
There was a problem hiding this comment.
In SQLite, dropping search_* tables does not automatically drop triggers defined on the base tables (elements, blocks, etc.). As written, these CREATE TRIGGER ... statements can fail with “trigger already exists” on an upgraded DB. Fix by explicitly DROP TRIGGER IF EXISTS ... for each trigger (or omit re-creating them if the existing definitions are unchanged) before recreating the FTS tables.
| -- This migration is necessary because FTS5 virtual tables cannot be altered | ||
| -- to change tokenizer or prefix options - they must be dropped and recreated. | ||
|
|
||
| -- 1. Drop old FTS tables (triggers are automatically dropped) |
There was a problem hiding this comment.
The comment “triggers are automatically dropped” is incorrect here because the triggers are attached to the content tables (elements, element_text, etc.), not the FTS tables being dropped. Please update the migration steps/comment to reflect that triggers must be dropped explicitly (or reused) to avoid migration failures.
| -- 1. Drop old FTS tables (triggers are automatically dropped) | |
| -- 1. Drop old FTS triggers and tables | |
| -- Triggers are attached to the content tables (elements, element_text, etc.), | |
| -- not the FTS tables themselves, so they must be dropped explicitly before | |
| -- recreating them to avoid migration failures. | |
| DROP TRIGGER IF EXISTS trg_elements_ai; | |
| DROP TRIGGER IF EXISTS trg_elements_ad; | |
| DROP TRIGGER IF EXISTS trg_elements_au; | |
| DROP TRIGGER IF EXISTS trg_element_text_ai; |
| @dataclass(slots=True) | ||
| class DucSearchResult: | ||
| """Compatibility placeholder for exported result types.""" | ||
|
|
||
|
|
||
| @dataclass(slots=True) | ||
| class DucElementSearchResult: |
There was a problem hiding this comment.
DucSearchResult is defined twice: first as an empty dataclass, then later rebound to a union type (DucElementSearchResult | DucFileSearchResult). This is confusing and breaks runtime expectations for anyone treating DucSearchResult as a class. Consider removing the placeholder dataclass and instead declaring the union as a TypeAlias (or rename the placeholder if you need a base protocol).
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.7.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.6.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.