Skip to content

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

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

nicoloesch wants to merge 15 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:
merge_replace/merge_insert/merge_upsert built raw, unqualified text() SQL; staging-table
index inspection used a schema-blind sa.inspect(); materialized-view lifecycle methods took a
hand-threaded schema string instead of resolving through the connection; and 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.
  • Fixed a data-loss bug in staging-table reflection: it now reflects through the session's own
    connection() instead of opening a second one off the bare engine — on SQLite's
    SingletonThreadPool that second connection is the same underlying DBAPI connection, and closing
    it silently discarded the session's own uncommitted work.
  • _target_has_rows() simplified: dropped a dead cross-table re-reflection branch.

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_role__, defaulting to
    Role.PRIMARY), not a hand-threaded schema parameter.
  • The Postgres-only dialect guard is now a single check inside the shared _as_connection()
    context manager (every bind-taking backend method routes through it), 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
    schema_inspect(session, role=role_of_table(cls.__table__)).
  • New role_of_table() helper: reads a mapped table's own declared schema tag back as a Role.

Split primary/vocab connections

  • Loading and merging now works correctly against genuinely separate primary/vocab connections
    (different engines, potentially different dialects), not just role-tagged schemas on one shared
    connection.

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, instead of
    depending on some other module happening to be imported first.
  • OrmLoaderConfig.test_orm_db split into test_orm_db_pg/test_orm_db_sqlite, so dialect-specific
    integration tests always run against the right kind of database regardless of local config.
  • CI split into separate SQLite and Postgres build-test jobs (previously one Postgres-only job).
  • engine_with_replica_role now uses autocommit_connection()'s Engine branch directly instead of
    opening a connection first and handing it in, removing a redundant step.
  • Its own Dialect enum is gone; imports the shared one from oa-configurator instead, with no
    change in dispatch behaviour.
  • Adopts oa-configurator's new test infrastructure (isolated_test_database, DIALECT_PARAMS,
    the 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, not just declared).

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
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.

1 participant