Conversation
There was a problem hiding this comment.
Pull request overview
Adds deterministic quote-handling for Athena vocabulary loads (addressing Issue #26), shifting the default behavior away from content sniffing so tab-delimited files preserve literal double-quotes.
Changes:
- Default vocabulary load
quote_modechanged to"by_delimiter"and propagated through the staged CSV loader. - Added
--quote-modeCLI option for explicitly controlling quote handling. - Bumped dependency versions (project version
0.8.1,orm-loader==0.5.2) and updated the changelog/tests accordingly.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
omop_alchemy/maintenance/cli_vocab.py |
Introduces QuoteMode, changes default to by_delimiter, and adds a CLI flag to control quote handling. |
tests/test_load_vocab_source.py |
Updates expectations for the new default quote mode and adjusts regression coverage around defaults. |
pyproject.toml |
Bumps project version and updates orm-loader pin to 0.5.2. |
uv.lock |
Lockfile update reflecting version/dependency changes. |
CHANGELOG.md |
Adds a 0.8.1 entry for the behavior change and dependency bump. |
Comments suppressed due to low confidence (1)
omop_alchemy/maintenance/cli_vocab.py:599
- There are no unit tests asserting that the new CLI option
--quote-modeis parsed and forwarded intoload_vocab_source(...)(or that invalid values are rejected). Adding a small CLI test would prevent regressions where the option is wired incorrectly, especially sincequote_modeis now a key safety/default behavior for vocabulary loads.
report = load_vocab_source(
engine,
source_path=effective_athena_source,
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,
progress_callback=_update_progress,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
nicoloesch
left a comment
nicoloesch
left a comment
There was a problem hiding this comment.
The core fix is correct: by_delimiter is properly implemented in the already-published orm-loader==0.5.2, resolves to literal for tab and csv for comma, and directly addresses the reported issue.
I'd approve with the comments above as follow-ups, except for the two I'd ask to fix before merge:
- the stale
"auto"default in_load_vocab_model_csv, and - the missing
--quote-modedocumentation for a change.
One note: We are currently having orm-loader hard-pinned to 0.5.2. We need to have this as the lower option from now on if we loosen the versioning eventually to not accidentally default back to the old behaviour and having to fix this quotation mode for the 4th time.
| model: VocabularyModel, | ||
| csv_path: Path, | ||
| merge_strategy: MergeStrategy, | ||
| quote_mode: str = "auto", |
There was a problem hiding this comment.
Still defaults to "auto" mode despite other defaults in this PR moved to "by_delimiter". This is masked by load_vocab_source passing it as quote_mode=quote_mode explicitly. However, test_load_vocab_model_csv_on_postgres (L.165; I'll mark it there too) calls this function directly without quote_mode, defaulting silently to the "old" "auto" mode. Should move this to "by_delimiter".
| source_path = _copy_fixture_source(tmp_path) | ||
| csv_path = source_path / "CONCEPT.csv" | ||
|
|
||
| row_count = _load_vocab_model_csv( |
There was a problem hiding this comment.
Silently uses the old "auto" mode as indicated by my previous comment since _load_vocab_model_csv has it still as the default.
| db_schema: str | None = None, | ||
| dry_run: bool = False, | ||
| merge_strategy: MergeStrategy = "replace", | ||
| quote_mode: QuoteMode = "by_delimiter", |
There was a problem hiding this comment.
New --quote-mode not reflected in docs/cli/reference.md (around Line 74).
Same for docs/getting-started/maintence.md in "Fresh database setup" or "Full vocabulary reload".
I just did a search for load-vocab-source in all *.md files. There are some other locations of this too but I don't think it is as important as those two files.
| 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)}) | ||
|
|
There was a problem hiding this comment.
Shouldn't that utilise the fixture of _make_concept_source (same file, L33)?
| 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] |
There was a problem hiding this comment.
Shouldn't that utilise the fixture of _make_concept_source (same file, L33)?
There was a problem hiding this comment.
All the sqlite-backed tests here monkeypatch out the CSV loader and only check that the quote_mode string is passed through correctly; none of them exercise real quote parsing. sqlite has no test coverage for the actual by_delimiter/literal/csv resolution behavior this PR introduces, only Postgres does (test_load_vocab_postgres.py).
We probably should include a test for that, don't you think?
Fixes #26