Skip to content

fix: quote loader paths and import CSV inputs - #2128

Open
Chessing234 wants to merge 2 commits into
MIT-LCP:mainfrom
Chessing234:fix/load-path-infra
Open

Chessing234 wants to merge 2 commits into
MIT-LCP:mainfrom
Chessing234:fix/load-path-infra

Conversation

@Chessing234

@Chessing234 Chessing234 commented Jul 25, 2026 •

Copy link
Copy Markdown
Contributor

Fix loader failures caused by quoted paths and SQL identifiers in the MIMIC-III Postgres/DuckDB loaders and the MIMIC-IV ED/Note DuckDB loaders.

  • Quote the Postgres data-directory argument consistently, including compressed imports, and quote download destinations.
  • Escape apostrophes in DuckDB COPY filenames and quote generated Postgres user/database identifiers and password literals.
  • Discover both .csv and .csv.gz files in the ED/Note loaders and report when no expected files were loaded.

The current diff contains five loader files. It contains no concept ordering, SQL transpiler, validation-table or workflow changes. The previously listed transpiler test does not validate this scope.

This description was corrected with AI assistance after comparing the current diff. No new loader execution or demo-data validation was performed in this maintenance pass; those checks remain necessary before claiming the load paths work end to end.

@Chessing234

Copy link
Copy Markdown
Contributor Author

Infra-only follow-up to the too-large #2124. Concept changes stay in #2126 / separate PRs.

@alistairewj alistairewj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes are scattered and not very cohesive but with some cleaning up we can merge

Comment thread .github/workflows/lint_sqlfluff.yml Outdated
Comment thread mimic-iii/buildmimic/duckdb/import_duckdb.sh Outdated
Comment thread src/mimic_utils/sqlglot_dialects/duckdb.py Outdated
Comment thread mimic-iii/buildmimic/postgres/Makefile Outdated
Comment thread mimic-iv-cxr/txt/chexpert/run_chexpert_on_files.sh Outdated
Comment thread .github/workflows/lint_sqlfluff.yml Outdated
Comment thread mimic-iv-cxr/txt/negbio/run_negbio.sh
Comment thread mimic-iii/buildmimic/sqlite/import.py Outdated
Comment thread mimic-iv/concepts/validate_concepts.sh Outdated
@Chessing234
Chessing234 force-pushed the fix/load-path-infra branch from a093a10 to 0d36c15 Compare July 29, 2026 15:28
@Chessing234

Copy link
Copy Markdown
Contributor Author

Addressed review feedback in 270b6d3:

  • SQLFluff: removed continue-on-error; added permissions: contents: read + checks: write; use ignore-unauthorized-error: true
  • Reverted duckdb usage text (no .tar.gz), DuckDB GENERATE_ARRAY transform + related concept/test, Makefile concepts target, CheXpert/NegBio defensive rewrites, and validate_concepts awk change
  • Removed sqlite TABLE_DTYPES from this PR (will split to a follow-up with CI as requested); kept plain .csv loading

@Chessing234

Copy link
Copy Markdown
Contributor Author

split the review asks into focused follow-ups: sqlfluff annotate permissions are in #2148; the sqlite import.py csv/.csv.gz discovery (with unit tests + ci) is in #2149. happy to trim the remaining infra bits on this branch further if you want them dropped.

@Chessing234
Chessing234 force-pushed the fix/load-path-infra branch from 427d504 to 3015382 Compare August 5, 2026 10:34
@acnimma

acnimma commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@alistairewj Ready for another look.

Your July 25 review asks are addressed as follows:

Ask Status
SQLFluff continue-on-error too blunt Removed; permissions + ignore-unauthorized-error moved to #2148
DuckDB .tar.gz usage text Reverted
DuckDB GENERATE_ARRAY transform Reverted
Makefile concepts target Reverted
CheXpert / NegBio defensive path edits Reverted
sqlite CSV discovery Split to #2149 with tests/CI
validate_concepts.sh awk / comment Reverted

This PR now keeps only the remaining load-path / transpile / duckdb import / validate_demo infra. Happy to cut anything else that still feels out of scope.

@Chessing234

Copy link
Copy Markdown
Contributor Author

reverted the make-concepts.sh quote-only hunks too. should be down to duckdb path escaping, postgres datadir/user quoting, sofa/oasis build order, transpile utf-8, and validate_demo tables. sqlfluff + sqlite csv bits stay in #2148 / #2149.

@Chessing234

Copy link
Copy Markdown
Contributor Author

@alistairewj resolved the review threads — ready for another look when you have time.

@Chessing234

Copy link
Copy Markdown
Contributor Author

@alistairewj re-review please when you get a chance — pushed after your review to address each point:

kept this PR scoped to load-path / transpile / import infra only.

Rebased onto main and squashed to the net change after several rounds of
trimming; the sqlfluff and sqlite hunks are in MIT-LCP#2148 / MIT-LCP#2149.

Covers duckdb load-path quoting for mimic-iii, -ed and -note, the postgres
datadir/user quoting and concepts build order, transpile UTF-8 handling with
its test, and the validate_demo table lists.

The mimic-iv duckdb hunk is dropped: main replaced
mimic-iv/buildmimic/duckdb/import_duckdb.sh with build_mimic.sh in MIT-LCP#1757, so
that file no longer exists.
@Chessing234 Chessing234 changed the title fix: load paths, imports, transpile, and CI permissions fix: quote load paths, imports, and transpile inputs Aug 23, 2026
Keep the path/SQL quoting fixes Alistair's review left standing. Revert
demo-row, concept-order, and DATETIME_DIFF MONTH changes so this PR stays
about load paths; sqlite discovery already lives in MIT-LCP#2149.
@Chessing234

Copy link
Copy Markdown
Contributor Author

trimmed this to the load-path/quoting fixes. dropped the tar.gz usage text, demo validate rows, concept reorder, and datetime_diff month bits — sqlite csv discovery is already in #2149 and the sqlfluff annotate follow-up in #2148.

@Chessing234 Chessing234 changed the title fix: quote load paths, imports, and transpile inputs fix: quote loader paths and import CSV inputs Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants