From b49c69b1880807c647be2d2768110143c5f38f4d Mon Sep 17 00:00:00 2001 From: gkennos Date: Sun, 5 Jul 2026 23:12:05 +1000 Subject: [PATCH 1/5] updated handling of quotes for tab-delimited files --- omop_alchemy/maintenance/cli_vocab.py | 20 ++++++++-- tests/test_load_vocab_source.py | 54 ++++++++++++--------------- 2 files changed, 41 insertions(+), 33 deletions(-) diff --git a/omop_alchemy/maintenance/cli_vocab.py b/omop_alchemy/maintenance/cli_vocab.py index ca5a711..af5048a 100644 --- a/omop_alchemy/maintenance/cli_vocab.py +++ b/omop_alchemy/maintenance/cli_vocab.py @@ -44,6 +44,7 @@ ) MergeStrategy: TypeAlias = Literal["replace", "upsert", "insert_if_empty"] +QuoteMode: TypeAlias = Literal["by_delimiter", "auto", "csv", "literal"] VocabularyModel: TypeAlias = type[CSVTableProtocol] VocabularyLoadProgressCallback: TypeAlias = Callable[["VocabularyLoadProgress"], None] @@ -250,6 +251,7 @@ def load_vocab_source( db_schema: str | None = None, dry_run: bool = False, merge_strategy: MergeStrategy = "replace", + quote_mode: QuoteMode = "by_delimiter", chunksize: int | None = 100_000, bulk_mode: bool = True, merge_batch_size: int | None = None, @@ -365,7 +367,7 @@ def _set_search_path(dbapi_conn, _record): row_count=None, csv_path=str(csv_path), required=required, - detail="Athena CSV would be loaded via staged ORM CSV loader using tab-delimited input and auto-detected quote mode", + detail=f"Athena CSV would be loaded via staged ORM CSV loader (quote_mode={quote_mode!r})", )) continue @@ -396,7 +398,7 @@ def _set_search_path(dbapi_conn, _record): model=model, csv_path=csv_path, merge_strategy=merge_strategy, - quote_mode="auto", + quote_mode=quote_mode, index_strategy="keep" if _use_bulk_mode else "auto", chunksize=chunksize, merge_batch_size=merge_batch_size, @@ -423,7 +425,7 @@ def _set_search_path(dbapi_conn, _record): row_count=row_count, csv_path=str(csv_path), required=required, - detail="Athena CSV loaded via staged ORM CSV loader using tab-delimited input and auto-detected quote mode", + detail=f"Athena CSV loaded via staged ORM CSV loader (quote_mode={quote_mode!r})", )) _emit( @@ -503,6 +505,17 @@ def load_vocab_source_command( "`insert_if_empty` is the fast path for a fresh empty target." ), ), + quote_mode: QuoteMode = typer.Option( + "by_delimiter", + help=( + "How the COPY fast-path interprets double-quotes. `by_delimiter` (default) " + "derives the mode from the delimiter with no content scan: tab-delimited input " + "is treated as literal (double-quotes are data), comma-delimited input uses " + "RFC-4180 quoting. This is deterministic and safe for a fresh full vocabulary " + "load. `auto` sniffs a sample of rows and can misfire when quoting first appears " + "beyond the sample window. `csv` / `literal` force one mode for every file." + ), + ), staging_chunk_size: int | None = typer.Option( 100_000, help=( @@ -578,6 +591,7 @@ def _update_progress(event: VocabularyLoadProgress) -> None: db_schema=conn.db_schema, dry_run=dry_run, merge_strategy=merge_strategy, + quote_mode=quote_mode, chunksize=None if staging_chunk_size == 0 else staging_chunk_size, bulk_mode=bulk_mode, merge_batch_size=merge_batch_size, diff --git a/tests/test_load_vocab_source.py b/tests/test_load_vocab_source.py index 44ebf02..3116394 100644 --- a/tests/test_load_vocab_source.py +++ b/tests/test_load_vocab_source.py @@ -11,6 +11,7 @@ OPTIONAL_VOCAB_MODELS, REQUIRED_VOCAB_MODELS, MergeStrategy, + QuoteMode, _load_vocab_model_csv, load_vocab_source, ) @@ -93,7 +94,7 @@ def fake_load_vocab_model_csv( assert all(result_by_name[model.__tablename__].status == "loaded" for model in REQUIRED_VOCAB_MODELS) assert all(result_by_name[model.__tablename__].status == "skipped" for model in OPTIONAL_VOCAB_MODELS) assert all(merge_strategy == "replace" for _, merge_strategy, _, _ in loaded_tables) - assert all(quote_mode == "auto" for _, _, quote_mode, _ in loaded_tables) + assert all(quote_mode == "by_delimiter" for _, _, quote_mode, _ in loaded_tables) assert {table_name for table_name, _, _, _ in loaded_tables} == { model.__tablename__ for model in REQUIRED_VOCAB_MODELS @@ -176,6 +177,7 @@ def fake_load_vocab_source( db_schema: str | None = None, dry_run: bool = False, merge_strategy: MergeStrategy = "replace", + quote_mode: QuoteMode = "by_delimiter", chunksize: int | None = None, bulk_mode: bool = True, merge_batch_size: int = 1_000_000, @@ -458,32 +460,23 @@ def fail_load_vocab_source(*args, **kwargs): assert "value too long for type character varying(255)" in result.stdout -def test_load_vocab_source_uses_auto_not_literal_quote_mode(monkeypatch, tmp_path): - """Regression: Athena load must use auto quote mode so that quoted concept_name - values are not padded with surrounding double-quote characters, which would - cause 'value too long for type character varying(255)' on CONCEPT.csv.""" - engine = sa.create_engine(f"sqlite:///{tmp_path / 'quote_mode_regression.db'}", future=True) - - # Build a tab-delimited CSV where concept_name is exactly 255 chars when - # unquoted, but would be 257 chars if the surrounding CSV quotes were kept - # as literal characters (the literal-mode bug). - source_path = tmp_path / "athena_source" - source_path.mkdir() - - long_name = "A" * 255 - for model in REQUIRED_VOCAB_MODELS: - table_name = model.__tablename__.upper() - csv_path = source_path / f"{table_name}.csv" - if table_name == "CONCEPT": - csv_path.write_text( - "concept_id\tconcept_name\tdomain_id\tvocabulary_id\t" - "concept_class_id\tstandard_concept\tconcept_code\t" - "valid_start_date\tvalid_end_date\tinvalid_reason\n" - f'4715176\t"{long_name}"\t...\t...\t...\t\t...\t20000101\t20991231\t\n', - encoding="utf-8", - ) - else: - csv_path.write_text("stub\n", encoding="utf-8") +def test_load_vocab_source_defaults_to_by_delimiter_quote_mode(monkeypatch, tmp_path): + """The vocab load must default to `by_delimiter`, never `auto`/`csv`. + + Real Athena vocabulary CSVs are tab-delimited with double-quotes as *literal* + data characters, not RFC-4180 field wrappers — e.g. CONCEPT_SYNONYM rows such + as '"Morning after" intrauterine contraceptive device fitted'. Interpreting + them as csv/RFC-4180 (which `auto` may pick, since its sample can miss the + discriminating rows) silently strips those quotes, which: + * corrupts concept/synonym names, and + * collapses two genuinely-distinct synonyms of the same concept into an + identical (concept_id, concept_synonym_name, language_concept_id) tuple, + violating Concept_Synonym's composite primary key on load. + `by_delimiter` resolves tab-delimited input to literal (quotes preserved) and + comma-delimited input to csv, deterministically and per file. + """ + engine = sa.create_engine(f"sqlite:///{tmp_path / 'quote_mode_default.db'}", future=True) + source_path = _build_required_athena_source(tmp_path) received_quote_modes: list[str] = [] @@ -508,7 +501,8 @@ def fake_load_vocab_model_csv( load_vocab_source(engine, source_path=source_path) - assert all(mode == "auto" for mode in received_quote_modes), ( - f"Expected all tables to use quote_mode='auto', got: {received_quote_modes}" + assert all(mode == "by_delimiter" for mode in received_quote_modes), ( + f"Expected all tables to use quote_mode='by_delimiter', got: {received_quote_modes}" ) - assert "literal" not in received_quote_modes + assert "auto" not in received_quote_modes + assert "csv" not in received_quote_modes From ff8a2b6374fd1738e736fda0b2483ebc9893537a Mon Sep 17 00:00:00 2001 From: gkennos Date: Sun, 5 Jul 2026 23:33:26 +1000 Subject: [PATCH 2/5] version bump --- CHANGELOG.md | 5 +++++ pyproject.toml | 4 ++-- uv.lock | 10 +++++----- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1fd51f8..c812f21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,3 +118,8 @@ - `manage_indexes()` now runs `ANALYZE` immediately after creating any new index, so the planner picks it up right away instead of waiting on autovacuum - added `--cluster/--no-cluster` flag to `indexes enable`, to skip the full heap rewrite on large vocabulary tables when disk headroom is limited - `indexes enable` now reports a pre-existing expression-based index as skipped instead of raising, since SQLite's reflection can't see it to begin with (affects the new functional indexes under the test backend) + + +## 0.8.1 +- change to handling for vocab files by default +- bump orm loader version \ No newline at end of file diff --git a/pyproject.toml b/pyproject.toml index 4c05b25..a3ec637 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "omop-alchemy" -version = "0.8.0" +version = "0.8.1" description = "SQLAlchemy-based models, validation, and utilities for the OHDSI OMOP Common Data Model" readme = "README.md" requires-python = ">=3.12" @@ -36,7 +36,7 @@ dependencies = [ "oa-configurator==0.1.2", "typer>=0.12", "rich>=13.0", - "orm-loader==0.5.1", + "orm-loader==0.5.2", ] [project.optional-dependencies] diff --git a/uv.lock b/uv.lock index 49ef5a0..2d37e23 100644 --- a/uv.lock +++ b/uv.lock @@ -1048,7 +1048,7 @@ postgres = [ [[package]] name = "omop-alchemy" -version = "0.8.0" +version = "0.8.1" source = { editable = "." } dependencies = [ { name = "oa-configurator" }, @@ -1093,7 +1093,7 @@ requires-dist = [ { name = "myst-parser", marker = "extra == 'docs'" }, { name = "oa-configurator", specifier = "==0.1.2" }, { name = "oa-configurator", extras = ["dev", "postgres"], marker = "extra == 'dev'", specifier = "==0.1.2" }, - { name = "orm-loader", specifier = "==0.5.1" }, + { name = "orm-loader", specifier = "==0.5.2" }, { name = "pandas", specifier = ">=2.0" }, { name = "psycopg", extras = ["binary"], marker = "extra == 'postgres'", specifier = ">=3.2" }, { name = "pytest", marker = "extra == 'dev'", specifier = ">=9.0.3" }, @@ -1110,7 +1110,7 @@ provides-extras = ["postgres", "dev", "docs"] [[package]] name = "orm-loader" -version = "0.5.1" +version = "0.5.2" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "chardet" }, @@ -1119,9 +1119,9 @@ dependencies = [ { name = "pyarrow" }, { name = "sqlalchemy" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/a9/04/a85b019eb07524547b73505286a6ad19b953edda79791935b779d956f0a7/orm_loader-0.5.1.tar.gz", hash = "sha256:885f13a804dad1272bbe0377a5ae0501d8403081254660552663cd13009ee542", size = 39873, upload-time = "2026-06-25T01:01:04.161Z" } +sdist = { url = "https://files.pythonhosted.org/packages/82/12/7e7edd85675755340ca7365137b0128ba7ff9f38912544e05a311bbb6a2c/orm_loader-0.5.2.tar.gz", hash = "sha256:ea5b127d930f0c90623558b1b7dd53a3503e9d95944b3bb119a460da43e173b6", size = 40520, upload-time = "2026-07-05T13:27:44.919Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/2f/4f/b9ccba8f63ca2e7f466bbce7cfb09b11e7df048854097e82e0178b970dd4/orm_loader-0.5.1-py3-none-any.whl", hash = "sha256:da6359fab5afc5a32e4d21a66064aed503a7d9b185967e5df2ed84ecbb4d0da7", size = 55058, upload-time = "2026-06-25T01:01:02.751Z" }, + { url = "https://files.pythonhosted.org/packages/87/5f/ef09bcc838ecf0bf5bd68db95ab251395d70ef3501427cc2d4cfb06e14ff/orm_loader-0.5.2-py3-none-any.whl", hash = "sha256:cfcd68cb4c407adb1ff1ca2f9f6bb803505d1ec8973658edeed4b980abf048a9", size = 55781, upload-time = "2026-07-05T13:27:43.659Z" }, ] [[package]] From d77ec5e20ceeb875533fa336f65ebdfe793201b0 Mon Sep 17 00:00:00 2001 From: gkennos Date: Sun, 5 Jul 2026 23:37:17 +1000 Subject: [PATCH 3/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c812f21..a047726 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,5 +121,5 @@ ## 0.8.1 -- change to handling for vocab files by default -- bump orm loader version \ No newline at end of file +- Default vocabulary load quote mode is now `by_delimiter` (tab-delimited Athena files preserve literal double-quotes; comma-delimited files use RFC-4180 CSV quoting). Add `--quote-mode {by_delimiter,auto,csv,literal}` to override. +- bump orm-loader version to 0.5.2 \ No newline at end of file From f61c67636812e647821cd3410bfa4be3dd15a29b Mon Sep 17 00:00:00 2001 From: gkennos Date: Sun, 5 Jul 2026 23:37:40 +1000 Subject: [PATCH 4/5] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- omop_alchemy/maintenance/cli_vocab.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/omop_alchemy/maintenance/cli_vocab.py b/omop_alchemy/maintenance/cli_vocab.py index af5048a..a2df4db 100644 --- a/omop_alchemy/maintenance/cli_vocab.py +++ b/omop_alchemy/maintenance/cli_vocab.py @@ -508,7 +508,7 @@ def load_vocab_source_command( quote_mode: QuoteMode = typer.Option( "by_delimiter", help=( - "How the COPY fast-path interprets double-quotes. `by_delimiter` (default) " + "How the vocabulary CSV loader interprets double-quotes. `by_delimiter` (default) " "derives the mode from the delimiter with no content scan: tab-delimited input " "is treated as literal (double-quotes are data), comma-delimited input uses " "RFC-4180 quoting. This is deterministic and safe for a fresh full vocabulary " From 4944c3c8bdae102071d8b315867f848f6027f7a8 Mon Sep 17 00:00:00 2001 From: gkennos Date: Sun, 5 Jul 2026 23:54:24 +1000 Subject: [PATCH 5/5] merge conflict --- CHANGELOG.md | 4 +- tests/test_load_vocab_postgres.py | 64 ++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a047726..3df1ae8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,5 +121,5 @@ ## 0.8.1 -- Default vocabulary load quote mode is now `by_delimiter` (tab-delimited Athena files preserve literal double-quotes; comma-delimited files use RFC-4180 CSV quoting). Add `--quote-mode {by_delimiter,auto,csv,literal}` to override. -- bump orm-loader version to 0.5.2 \ No newline at end of file +- **breaking:** `load-vocab-source` now defaults `quote_mode` to `by_delimiter` (was `auto`). For tab-delimited Athena input this resolves to `literal`: double-quotes are treated as data and preserved verbatim, rather than sniffed and potentially stripped. This is deterministic and avoids `auto` misfiring when quoting first appears beyond the sample window. Sources that genuinely RFC-4180-wrap fields (see 0.6.3) must now pass `--quote-mode csv` (or `auto`) explicitly to strip wrapping quotes and avoid overflowing `VARCHAR(255)` columns such as `concept_name`. +- bump orm loader version \ No newline at end of file diff --git a/tests/test_load_vocab_postgres.py b/tests/test_load_vocab_postgres.py index fed6fcc..a637f2f 100644 --- a/tests/test_load_vocab_postgres.py +++ b/tests/test_load_vocab_postgres.py @@ -75,41 +75,79 @@ def test_end_to_end_vocab_load_on_postgres(pg_session, pg_engine, tmp_path): @pytest.mark.requires_resource(OmopAlchemyConfig.TEST_DB) -def test_quote_mode_auto_regression_on_postgres(pg_session, pg_engine, tmp_path): +def test_default_quote_mode_preserves_literal_quotes_on_postgres(pg_session, pg_engine, tmp_path): """ - quote_mode='auto' strips RFC-4180 double-quotes via PostgreSQL COPY. + The default quote_mode='by_delimiter' treats double-quotes in tab-delimited + Athena input as literal data, not RFC-4180 field wrappers, so a + concept_name containing double-quotes is stored verbatim via COPY. + + Athena vocabulary exports are tab-delimited and do not RFC-4180-quote + fields, so a double-quote in the source is part of the value and must + survive the load intact. (Callers with genuinely quote-wrapped sources can + opt into stripping with quote_mode='csv' — see the companion test below.) + """ + source_path = tmp_path / "athena_source" + source_path.mkdir() + + # Begins and ends with a literal double-quote: under RFC-4180 (csv) these + # boundary quotes would be stripped as wrappers; under the by_delimiter + # default (tab -> literal) they are data and must be preserved verbatim. + # Comfortably within VARCHAR(255). + quoted_name = '"BRCA1 positive"' + + for table_name, data in _ATHENA_FIXTURE_DATA.items(): + if table_name != "concept": + _write_fixture_csv(source_path, table_name, data) + + concept_cols = list(_ATHENA_FIXTURE_DATA["concept"].keys()) + concept_row = [1, quoted_name, "Gender", "Gender", "Gender", "S", "TEST", "19700101", "20991231", None] + _write_fixture_csv(source_path, "concept", {col: (val,) for col, val in zip(concept_cols, concept_row)}) - Under the old quote_mode='literal' a concept_name of exactly 255 chars - wrapped in double-quotes would be stored as 257 chars and violate the - VARCHAR(255) constraint. This test would fail under literal mode. + # Default quote_mode resolves by_delimiter -> literal for tab-delimited input. + load_vocab_source(pg_engine, source_path=source_path) + + concept_name = pg_session.execute( + sa.text("SELECT concept_name FROM concept WHERE concept_id = 1") + ).scalar() + assert concept_name == quoted_name, ( + f"by_delimiter default must preserve literal double-quotes verbatim; got {concept_name!r}" + ) + + +@pytest.mark.requires_resource(OmopAlchemyConfig.TEST_DB) +def test_explicit_csv_quote_mode_strips_quotes_on_postgres(pg_session, pg_engine, tmp_path): + """ + Passing quote_mode='csv' explicitly still applies RFC-4180 stripping via + PostgreSQL COPY, for sources that genuinely wrap fields in double-quotes. + + A concept_name of exactly 255 chars wrapped in quotes is 257 chars in the + file; csv mode strips the wrappers back to 255 so it fits VARCHAR(255). + Under the by_delimiter default this same input would overflow, which is the + reason the opt-in exists (see CHANGELOG 0.6.3 / 0.8.1). """ source_path = tmp_path / "athena_source" source_path.mkdir() - long_name = "A" * 255 # exactly at VARCHAR(255) limit when unquoted + long_name = "A" * 255 # exactly at the VARCHAR(255) limit once unwrapped - # All tables except concept get the standard fixture data. for table_name, data in _ATHENA_FIXTURE_DATA.items(): if table_name != "concept": _write_fixture_csv(source_path, table_name, data) - # Concept gets a single row whose name is wrapped in RFC-4180 double-quotes - # so the raw file value is 257 chars. quote_mode='auto' must strip them. concept_cols = list(_ATHENA_FIXTURE_DATA["concept"].keys()) concept_row = [1, f'"{long_name}"', "Gender", "Gender", "Gender", "S", "TEST", "19700101", "20991231", None] _write_fixture_csv(source_path, "concept", {col: (val,) for col, val in zip(concept_cols, concept_row)}) - # Should not raise: literal mode would produce a 257-char value and fail. - load_vocab_source(pg_engine, source_path=source_path) + load_vocab_source(pg_engine, source_path=source_path, quote_mode="csv") concept_name = pg_session.execute( sa.text("SELECT concept_name FROM concept WHERE concept_id = 1") ).scalar() assert concept_name is not None assert len(concept_name) == 255, ( - f"Expected 255-char name; got {len(concept_name)}: {concept_name!r}" + f"Expected 255-char name after csv stripping; got {len(concept_name)}: {concept_name!r}" ) - assert not concept_name.startswith('"'), "Surrounding quotes were not stripped" + assert not concept_name.startswith('"'), "csv mode should strip surrounding quotes"