DuckDB MIMIC-III concepts implementation #1529

SphtKr · 2023-04-28T20:34:05Z

These changes expand on #1489 / #1239 by adding a Python version that ingests the data and creates the concepts views in the DuckDB database. The python version requires only the duckdb python package and not the command-line executable. Also requires sqlglot.

Run python import-duckdb.py --help for usage, it's pretty simple.

Tested on the MIMIC-III demo and the full version (1.4)... it seems to work, though I'm not sure if there's a better way to test (i.e. compare actual values... I have the Postgres version running in a container if so).

Resolves #1495

…)...committing before gutting dead code.

…es without a large amount of RAM, and this way it can be skipped with `--skip-indexes`

alistairewj

Some initial thoughts from a scan of the code.

I also think there are a lot of if/else statements littered throughout, but haven't thought about it enough to simplify that yet.

mimic-iii/buildmimic/duckdb/README.md

alistairewj · 2023-05-01T13:23:56Z

mimic-iii/buildmimic/duckdb/duckdb_add_tables.sql

I was hoping to avoid having a separate create table statement for duckdb - for MIMIC-III it's not a big deal (the tables have been static for ages), so this can be left as is.

For MIMIC-IV, we are continuing to update it, so this results in some inconsistencies (which either need to be checked with a gh action, which is hard, or avoided by having one source of SQL DDL for creating tables).

I can try putting this through sqlglot--I think I made that SQL script before I started using that. At present, there is a bug (referenced in the comment in that file) that causes creation of the table to fail with the primary key in place (index creation causes OOM)... so until that's fixed the table creation will need at least that tweak.

It would be nice to have a json used to generate the DDL for postgres/bigquery/mysql/duckdb - but it also seems like a separate PR to duckdb concepts

I suppose the index OOM bug is a duckdb bug?

I suppose the index OOM bug is a duckdb bug?

Yep, it's resolved in their repo but not released yet.

mimic-iii/buildmimic/duckdb/concepts/icustay_hours.sql

alistairewj · 2023-05-01T13:55:35Z

mimic-iii/buildmimic/duckdb/concepts/icustay_hours.sql

for consistency this file would be better placed in mimic-iii/concepts_duckdb/icustay_hours.sql - this seems doable with the python script

alistairewj · 2023-05-01T13:59:48Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+                sql = fp.read()
+                result = connection.execute(sql).fetchall()
+                for row in result:
+                    print(f"{row[0]}: {row[2]} records ({row[1]} expected) - {row[3]}")


This will print all fails if we load the demo. Might be nice to have a --demo flag to alternate to a demo validation check?

I was thinking about --skip-checks... should I just make --run-checks (off by default)? Don't love the idea of another set of static values...though I suppose they wouldn't change much.

I ended up with a validate.sql and a validate_demo.sql with MIMIC-IV fwiw

Though again, the row counts are something that screams "this should be a json somewhere else in the repo", rather than hard-coded into all these destined to rot SQL files

alistairewj · 2023-05-01T14:11:41Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+
+        print("Creating tables...")
+
+        # ccs_dx is an outlier...this is adapted from the BigQuery version...


why is it an outlier?

Yes, comment should probably be more specific. For the record, 1. because it's the only non-query-based table (that gets loaded with data) in the concepts build process, 2. the Postgres DB puts it in the public schema instead of mimiciii... which I am guessing is because it's also applicable to the MIMIC-IV concepts?

alistairewj · 2023-05-01T14:16:28Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+                #cProfile.run('...')
+                #print(f"Making view {key}...")
+                db = concept_name_map[key].get("db", "bigquery")
+                if db == "duckdb":


I think this is unnecessarily abstract. You might as well just hard-code the make_duckdb_query function to do something special for icustay_hours. Or, alternatively, have the subfunction contain a hard-coded list of queries which it parses as duckdb, that way you can expand it to more queries in the future if you plan to move more over

Yes.... earlier I was pulling some queries from the BigQuery version and others from the Postgres version, and Postgres was actually the default...later I got them all to work translated from BigQuery except this one...which now looks odd.

alistairewj · 2023-05-01T14:17:20Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+            sql = _duckdb_rewrite_schema(sql, schema)
+
+        try:         
+            conn.execute(f"CREATE VIEW {qname} AS " + sql)


missing a drop view if exists -> will be fixed by harmonizing duckdb/bigquery subfunctions

alistairewj · 2023-05-01T14:18:11Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+    with open(qfile, "r") as fp:
+        sql = fp.read()
+        sql = re.sub(_too_many_backslashes_re, '\\$1', sql) 
+        try:


i would rewrite this merging the bigquery/duckdb subfunctions into one, and having an if statement which calls the sql modification if needed, and then continuing on as usual. this would avoid mild divergences in behaviour observed e.g. forgetting to drop the view for duckdb but doing it for bigquery

Probably valid.... these two were originally far more divergent but are indeed quite similar now.

alistairewj · 2023-05-01T14:20:02Z

mimic-iii/buildmimic/duckdb/import_duckdb.py

+
+            conn.execute(f"DROP VIEW IF EXISTS {qname}")
+            try:         
+                conn.execute(f"CREATE VIEW {qname} AS " + sql)


What about creating either materialized views or simply tables? A view would rerun the code over and over (and be slow). I think most users will be happy with trading off the disk space for faster queries, so I'd say the default should be to create tables and if a --view argument is passed it creates views instead (or two arguments, --make-concepts and --make-concepts-as-views).

Probably a good idea. Looks like DuckDB doesn't have materialized views (yet), but for this use case tables are perfectly valid.

alistairewj · 2023-05-02T00:26:09Z

Seems like there are a few valid changes, feel free to update in your own time and just ping me with a re-review when you think it's ready

SphtKr · 2023-05-08T21:12:31Z

I have a very interesting problem.

I noticed that the DuckDB version here is producing integer results for DATETIME_DIFF-type operations, e.g. icustay_detail.los_icu, but the PostgreSQL version (implemented in postgres-functions.sql) returns fractional values, e.g. 3.14 days vs. 3 days (the DuckDB version is in fact a floor of the Postgres version, it appears). So I was going to fix that...

Then, since I am actually converting the queries from the BigQuery versions, I consulted the BigQuery documentation for DATETIME_DIFF which says "Returns the whole number of specified part intervals..." so it looks like the BigQuery version of the concepts may differ from the Postgres versions in this area--though I don't have easy access to the BigQuery version to test.

It looks like I can do either in the DuckDB version by changing how the SQL is translated... I may even put in an option to do either... but which one should it be, if there is a "should"? If nothing else I wanted to bring this up if it's an issue to have the discrepancy between the two current versions.

alistairewj · 2023-05-17T18:29:09Z

Definitely an inconsistency, good spot. I can confirm BigQuery is giving integers after calculating a floor. Immediately I think the easiest fix is to add a FLOOR() call to each of the postgresql functions which emulate BigQuery behaviour (which can be done in a separate PR). I'll raise an issue for that. I think there are a few queries where this would obviously impact the output as it's generated from a DATETIME_DIFF call (age, icustay_detail), and some queries where it's more subtle (e.g. urine_output_rate).

For duckdb I think return whole integers is fine.

SphtKr added 15 commits April 26, 2023 00:51

First "working" version (all SQL runs except for a few pivot concepts…

b1ac9a5

…)...committing before gutting dead code.

Stripped dead code

4da73b2

Simplify pathing, strip more dead code

4eb302f

Align with shell script version

d1fb76b

Add requirements.txt

8274982

Pulled sql out of .sh plus minor related mods

4b0ac76

Add table creation/loading

a7d0597

Schema support (option) to mirror psql version

92c7388

Another chunk of dead code

be78589

Big bug in DatetimeDiff implementation!

43a67ad

Adding missing fluid_balance views

382c7ff

missed deletion

99ef1f3

Rename in line with Postgres

2b877a8

Adding indexes

f74a4eb

Added checks

91fed32

SphtKr changed the title ~~DuckDB concepts implementation~~ DuckDB MIMIC-III concepts implementation Apr 28, 2023

SphtKr and others added 3 commits April 28, 2023 23:57

Updating README.md

9dea287

Missed file rename

060cc71

Move fake CHARTEVENTS PK to indexes script -- this may fail on machin…

e36f49c

…es without a large amount of RAM, and this way it can be skipped with `--skip-indexes`

alistairewj requested changes May 1, 2023

View reviewed changes

Fixed outright errors

6ba9ce7

Experimental option to use integer or fractional DATETIME_DIFF function

206c0f7

alistairewj mentioned this pull request May 17, 2023

DATETIME_DIFF implementation varies between postgres and bigquery #1549

Open

alistairewj mentioned this pull request Jan 6, 2024

Add duckdb build/concepts and use SQLGlot to convert BigQuery SQL into other dialects #1689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DuckDB MIMIC-III concepts implementation #1529

DuckDB MIMIC-III concepts implementation #1529

SphtKr commented Apr 28, 2023 •

edited

Loading

alistairewj left a comment

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj May 2, 2023

SphtKr May 2, 2023

alistairewj May 1, 2023

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj May 2, 2023

alistairewj May 2, 2023

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj May 1, 2023

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj May 1, 2023

SphtKr May 1, 2023

alistairewj commented May 2, 2023

SphtKr commented May 8, 2023

alistairewj commented May 17, 2023


		print("Creating tables...")

		# ccs_dx is an outlier...this is adapted from the BigQuery version...

DuckDB MIMIC-III concepts implementation #1529

Are you sure you want to change the base?

DuckDB MIMIC-III concepts implementation #1529

Conversation

SphtKr commented Apr 28, 2023 • edited Loading

alistairewj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alistairewj commented May 2, 2023

SphtKr commented May 8, 2023

alistairewj commented May 17, 2023

SphtKr commented Apr 28, 2023 •

edited

Loading