fix: Slow lower search functionality and cost.py fix#22
Merged
Conversation
gkennos
requested changes
Jun 23, 2026
gkennos
approved these changes
Jun 25, 2026
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.
Fix
Added two functional btree indexes:
ix_concept_concept_name_loweronconcept (lower(concept_name))ix_concept_synonym_concept_synonym_name_loweronconcept_synonym (lower(concept_synonym_name))Declared via
omop_index()in theConcept/Concept_SynonymORM__table_args__, applied through the existingomop-alchemy indexes enable --vocabmaintenance CLI.ANALYZEnow runs automatically right after index creation so the planner picks up new indexes immediately (previously only autovacuum's load-time analyze covered existing indexes; a newly added index gets no stats until something explicitly analyzes it). Also added a--cluster/--no-clusterflag toindexes enableto skip the full heap rewrite on large vocabulary tables when disk headroom is limited, and hardened index creation against SQLite's inability to reflect expression-based indexes (a duplicate-create attempt on an existing functional index is now reported as skipped instead of raising).concept.concept_name/concept_synonym.concept_synonym_name#21Why OMOP_Alchemy
These are general-purpose, exact-match indexes useful to anything querying concept/synonym names directly, not specific to a single consumer, so they belong with the rest of the CDM schema/index definitions here.
Result
EXPLAIN ANALYZEconfirms the plan changed from sequential scan to Index Scan: ~24,000ms → ~0.05ms.Due to the importance of it, this PR also includes the following bug fixes found:
manage_indexes()raisedKeyErrorforcost.cost_type_concept_idunder any schema-qualified connection:costwas the only model using an implicitly-named column index (required_concept_fk(index=True)), whose auto-generated name embeds the schema and drifts from the unqualified name the maintenance CLI looks up by. Fixed by switching to an explicitomop_index(...), matching every other model.fact_relationship's primary key was silently missingfact_id_1/fact_id_2:merge_table_args()dropped the explicitPrimaryKeyConstraintbecause it's falsy (zero-length.columns) until attached to a table, so the constraint never took effect and the table's real PK was only 3 of its intended 5 columns. Fixed by declaringprimary_key=Trueper column, matching every other composite-PK table.Hardened
merge_table_args()to filter onis Nonerather than truthiness, so a Constraint/Index built from unresolved column-name strings can no longer be silently dropped from__table_args__: this is what let thefact_relationshipbug above go unnoticed.Breaking: removed the
index: boolparameter fromrequired_concept_fk(),optional_concept_fk(),optional_fk(): it produced schema-unsafe implicit index names and had no remaining call sites; index FK columns via an explicitomop_index(...)in__table_args__instead.Fixes
cost.pydoes not useomop_index()resulting in wrong index naming convention with schemas #23Fixes Pin oa-configurator to an exact version due to expected breaking changes #24