Skip to content

Rewrite merge methods on real Core constructs so schema_translate_map applies to loaded data - #39

Open
nicoloesch wants to merge 23 commits into
mainfrom
schema-translate-map
Open

nicoloesch wants to merge 23 commits into
mainfrom
schema-translate-map

Conversation

@nicoloesch

@nicoloesch nicoloesch commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Depends on AustralianCancerDataNetwork/oa-configurator#37.

Requires pyproject.toml version update for oa-configurator

Breaking change

OrmLoaderConfig.test_orm_db is gone, replaced by test_orm_db_pg/test_orm_db_sqlite. Anyone with test_orm_db set in their omop.config will need to update it. There is no back-compat shim. Needed because dialect-specific integration tests require a guaranteed-correct-dialect test database each, which one shared field couldn't express.

Root cause

Four related ways schema_translate_map was silently bypassed across orm-loader:

  1. merge_replace/merge_insert/merge_upsert built raw, unqualified text() SQL
  2. staging-table index inspection used a schema-blind sa.inspect()
  3. materialized-view lifecycle methods took a hand-threaded schema string instead of resolving through the connection, and
  4. a staging-table reflection path could open a second connection that, on SQLite, silently discarded a session's own uncommitted work.

Fix

Loaded/merged data

  • merge_replace/merge_insert/merge_upsert rewritten against real Table objects and
    dialect-specific Core constructs (sa.delete, insert().from_select(),
    postgresql.insert().on_conflict_do_nothing(), with a SQLite-specific counterpart), for both backends.
  • Fixed the one remaining raw isolation_level="AUTOCOMMIT" call site.
  • Staging-table reflection reflects now through the session's ownconnection() to prevent data loss

Materialized views

  • create_mv/refresh_mv/drop_mv/index creation are fully role-aware
    • each resolves its physical schema from the bound connection's own schema_translate_map (__mv_schema_tag__, defaulting to Role.PRIMARY)
  • The Postgres-only dialect guard is now a single check inside the shared _as_connection() context manager, replacing a narrower per-method check that only ever guarded direct/manual PostgresBackend() construction.
  • Staging-table index management (manage_indices) now resolves its Inspector schema-aware, via physical_schema_of(session, schema_tag=validate_schema_tag(cls.__table__)).

Split primary/vocab connections

  • Loading and merging now works correctly against genuinely separate primary/vocab connections

Config, CI, test infrastructure

  • STAGING_SCHEMA's reserved-schema registration moved into config.py (the actual omop.config entry point), so it's guaranteed to run whenever this package's config is resolvable
  • OrmLoaderConfig.test_orm_db split into test_orm_db_pg/test_orm_db_sqlite for dialect-specific test configurations
  • CI split into separate SQLite and Postgres build-test jobs
  • engine_with_replica_role now uses autocommit_connection()'s Engine branch directly
  • Removed own Dialect system in favour of the unified oa-configurator one
  • Adopts oa-configurator's new test infrastructure (isolated_test_database, DIALECT_PARAMS, db_dialect marker).
  • New tests:
    -test_schema_translate_map.py: the actual regression test for the originally-reported
    bug,
    • test_split_connection.py, test_shared_backend.py: merge-method contract tests unified across both dialects,
    • test_reserved_schema.py: proves STAGING_SCHEMA registration is actually
      enforced cross-package

Schema drift protection, primitive consolidation

  • helpers/bootstrap.py::create_db/bootstrap() is now wrapped in guard_schema_provenance_for via oa-configurator's open_connection to prevent schema drift
  • helpers/sql.py::qualify_identifier deleted entirely
    • consolidated into oa-configurator's qualified()
  • backends/base.py::_as_connection now delegates its Engine/Connection branch to oa-configurator's open_connection.
  • Adopts physical_schema_of stack-wide
    • fixes validate_schema_tag usage in materialised_view_mixin.py and schema_if_supported usage in backends/sqlite.py.

Checklist

  • Applied exactly one label (breaking, feature, fix, dependencies, or chore)
  • Tests pass locally (uv run pytest -q): 246 passed, 1 xfailed
  • Tests pass locally (uv run pytest -q -m postgresql): 37 passed
  • Lint passes (uv run ruff check .)

@nicoloesch nicoloesch added breaking Incompatible API change. MAJOR: x+1.y.z and removed fix Bug fix, backwards-compatible. PATCH: x.y.z+1 labels Sep 16, 2026
@gkennos
gkennos self-requested a review September 20, 2026 23:50
Comment thread src/orm_loader/helpers/sql.py Outdated
@nicoloesch
nicoloesch requested a review from gkennos September 21, 2026 06:28

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Incompatible API change. MAJOR: x+1.y.z

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants