Skip to content

Add shared ConceptFilter for CDM concept-table queries#37

Merged
nicoloesch merged 6 commits into
mainfrom
11-concept-filter
Jul 24, 2026
Merged

Add shared ConceptFilter for CDM concept-table queries#37
nicoloesch merged 6 commits into
mainfrom
11-concept-filter

Conversation

@nicoloesch

@nicoloesch nicoloesch commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds ConceptFilter (omop_alchemy.cdm.query.ConceptFilter), a shared, reusable way to filter CDM concept-table queries by domain, vocabulary, concept ID, and standard/active status, with an optional row-count limit.

Both omop-emb and omop-graph currently reimplement this same filtering logic independently against their own copies of the Concept model's column names. This gives both a single, shared implementation to depend on instead. Building it here rather than in either downstream package because omop_alchemy is the one place that owns the Concept model directly, so the filter can reference real columns (Concept.domain_id, Concept.standard_concept, etc.) rather than duck-typing against an opaquely-imported table.

Checklist

  • Applied exactly one label (breaking, feature, fix, dependencies, or chore) — feature
  • Tests pass locally (uv run pytest -q)
  • Lint passes (uv run ruff check .)

@nicoloesch
nicoloesch requested a review from gkennos July 21, 2026 04:36
@nicoloesch nicoloesch added the feature New backwards-compatible functionality. MINOR: x.y+1.z label Jul 21, 2026
Comment thread omop_alchemy/cdm/model/vocabulary/concept.py Outdated
@nicoloesch

Copy link
Copy Markdown
Collaborator Author

Investigation

Concept.is_standard/is_valid were briefly implemented as hybrid_property (one definition, usable both as a Python check and a SQL expression). Benchmarked hybrid_property to get to the following conclusion:

  • Benefit: Shared name across Python and SQL context
  • Drawbacks:
    • ~75-80% overhead vs. a plain @property (confirmed on SQLite and Postgres)
    • zero Python-side consumers prior to this change (now used by the ConceptFilter)

Conclusion

Given Concept is a heavily-joined, hot table, paying that tax indefinitely for an unused convenience wasn't worth it.

Decision

Reverted to two explicit definitions instead:

  • a plain @property (is_standard/is_valid, fast Python-side check), and
  • a @classmethod (is_standard_expr()/is_valid_expr(), SQL-side expression used by ConceptFilter).

Both derive from the same normalised_flag_expr + StandardConceptFlag/InvalidReasonFlag enums, so there's one source of truth for what counts as standard/valid. Just two call sites instead of one, chosen deliberately (self-delegation) rather than automatic dispatch.

  • test_is_standard_agrees_with_require_standard_filter/test_is_valid_agrees_with_require_active_filter already assert the two stay in sync, so the drift risk that originally motivated hybrid_property is covered by tests rather than by the mechanism itself.

@gkennos
gkennos self-requested a review July 24, 2026 02:33
@nicoloesch
nicoloesch merged commit 00cb14d into main Jul 24, 2026
3 checks passed
@nicoloesch
nicoloesch deleted the 11-concept-filter branch July 24, 2026 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New backwards-compatible functionality. MINOR: x.y+1.z

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: centralised concept query filtering API

2 participants