Overhaul database architecture and management - #17
Conversation
… a pure Python runner
| dataset_id INTEGER NOT NULL | ||
| REFERENCES datasets (dataset_id) | ||
| ON DELETE CASCADE, | ||
| name TEXT NOT NULL, |
There was a problem hiding this comment.
Could you please remove "ON DELETE CASCADE" since it's not available with duckdb?
There was a problem hiding this comment.
I have removed "ON DELETE CASCADE" in the new version.
| tables = {} | ||
| for block in re.finditer(r"CREATE TABLE\s+(?:IF NOT EXISTS\s+)?(\w+)\s*\((.*?)\);", sql, re.S | re.I): | ||
| table, body = block.group(1), block.group(2) | ||
| tables[table] = [ | ||
| line.strip().rstrip(",").split()[0] | ||
| for line in body.splitlines() | ||
| if line.strip() and not re.match(r"(PRIMARY|FOREIGN|UNIQUE|CHECK|CONSTRAINT)", line.strip(), re.I) | ||
| ] | ||
|
|
||
| # SHOW TABLES | ||
| print("\nSHOW TABLES") | ||
| print("=" * 32) | ||
| for name in sorted(tables): | ||
| print(f" {name}") | ||
| print("-" * 32) | ||
| print(f" {len(tables)} table(s)\n") | ||
|
|
||
| # DESCRIBE <table> | ||
| for table, columns in sorted(tables.items()): | ||
| print(f"DESCRIBE {table}") | ||
| print("=" * 40) | ||
| for i, col in enumerate(columns, 1): | ||
| print(f" {i:<4} {col}") | ||
| print(f" {len(columns)} column(s)\n") |
There was a problem hiding this comment.
As we discussed, could you please remove these lines?
There was a problem hiding this comment.
We don't want to parse the SQL file.
There was a problem hiding this comment.
Done, removed the SQL parsing block. The script now just creates the database.
| file_type_id INTEGER PRIMARY KEY DEFAULT nextval('file_types_seq'), | ||
| name TEXT NOT NULL UNIQUE, | ||
| comment TEXT | ||
| ); |
There was a problem hiding this comment.
Nice. Could you please ensure data types are regular duckdb data types as listed here: https://duckdb.org/docs/current/sql/data_types/overview ?
For instance, TEXT should be VARCHAR
And maybe, all dates should be of TIMESTAMP type (to be checked)
There was a problem hiding this comment.
Done! Replaced all TEXT with VARCHAR, date columns changed to TIMESTAMP, and also updated boolean INTEGER columns to BOOLEAN.
- Replace all TEXT with VARCHAR - Change date columns to TIMESTAMP - Change boolean-like INTEGER columns to BOOLEAN - Add report_duckdb.py to display row and column counts per table
|
Thanks @sheikhPHD |
Here is the estimate for ZENODO: source type time |
Requires DuckDB >= 1.5.3. Earlier versions had a memory corruption bug triggered when deleting large sources via the Python library after an input() call.
| _S_DS = "SELECT dataset_id FROM datasets WHERE data_source_id = (SELECT data_source_id FROM data_sources WHERE name = $1)" | ||
| _S_FI = f"SELECT file_id FROM files WHERE dataset_id IN ({_S_DS})" | ||
| _S_AN = f"SELECT annotation_id FROM annotations WHERE dataset_id IN ({_S_DS})" | ||
| _S_MO = f"SELECT molecule_id FROM molecules WHERE annotation_id IN ({_S_AN})" |
There was a problem hiding this comment.
Please use meaningful variable names
There was a problem hiding this comment.
Done — eliminated entirely. Instead of renamed subquery variables, IDs are now fetched into Python lists via collect_ids_for_dataset() and collect_ids_for_source(), then accessed as ids["dataset_ids"], ids["file_ids"], etc.
|
|
||
| # ── Core deletion logic ──────────────────────────────────────────────────────── | ||
|
|
||
| def _count(conn: duckdb.DuckDBPyConnection, param: list, ds: str, fi: str, an: str, mo: str) -> dict[str, int]: |
There was a problem hiding this comment.
Please use meaningful variable names. ds, fi, an... are not very expressive.
There was a problem hiding this comment.
Done — eliminated entirely. Instead of renamed subquery variables, IDs are now fetched into Python lists via collect_ids_for_dataset() and collect_ids_for_source(), then accessed as ids["dataset_ids"], ids["file_ids"], etc.
| def run(label: str, count_sql: str, delete_sql: str) -> None: | ||
| counts[label] = conn.execute(count_sql, param).fetchone()[0] | ||
| conn.execute(delete_sql, param) | ||
|
|
There was a problem hiding this comment.
Could you move this function outside the _delete function and rename it with a more meaningful name?
There was a problem hiding this comment.
Done — moved outside and split into count_rows_by_ids() and delete_rows_by_ids().
| """Remove a single dataset and all its related records.""" | ||
| print(f"INFO | Mode: DELETE DATASET | datarepo='{source_name}' dataset='{id_in_source}'") | ||
| if dry_run: | ||
| print("WARN | DRY-RUN — no changes will be written.") |
There was a problem hiding this comment.
Please use proper loger with loguru
There was a problem hiding this comment.
Done — replaced all print() calls with loguru logger.info/warning/error/success.
| Note on transactions: | ||
| DuckDB v1.x enforces FK constraints per-statement even inside BEGIN/COMMIT, | ||
| making transactional multi-table cascades impossible with FK constraints. | ||
| The standard workaround is autocommit with strict child-first deletion order, | ||
| which guarantees no orphaned rows at any point. Each DELETE is individually | ||
| atomic; a mid-run failure leaves the database consistent and retryable. |
There was a problem hiding this comment.
Unclear to me. Could you explain a bit more?
There was a problem hiding this comment.
DuckDB enforces foreign key constraints immediately after each DELETE statement, even inside a BEGIN/COMMIT block. This means we cannot wrap all deletions in a single transaction — deleting a parent row (e.g. datasets) while child rows still exist in another table (e.g. files) raises a FK violation, even if those child rows would be deleted later in the same transaction.
The workaround is autocommit with strict child-first ordering: we always delete the deepest child table first and work up to the parent. Each DELETE commits immediately, so by the time we delete a parent row, all its children are already gone. I verified this limitation is still present in DuckDB v1.5.3 with explicit tests.
|
|
||
| # dataset mode subqueries | ||
| _D_DS = "SELECT $1::INTEGER" |
There was a problem hiding this comment.
Instead of using Duckdb subqueries, could you:
- List all file_ids involved into the deletion:
- if a file_id is provided: the file itself and all its related files if its a zip file
- if a repo name is provided: all files from the repo and all files from zip files in this repo
- then run the deletion
There was a problem hiding this comment.
Done — refactored to fetch all IDs (dataset_ids, file_ids, annotation_ids, molecule_ids) into Python lists first using collect_ids_for_dataset() and collect_ids_for_source(), then delete using those lists.
…fetch IDs into Python, move run() helper outside _delete
… connection handling
- for SQLite3 deletion - for data downloading - for parameter/topology/trajectory ingestion
| """CLI entry point for data deletion.""" | ||
| logger = create_logger(Path("logs/delete_data.log")) | ||
| start = time.perf_counter() | ||
| run_deletion(db_path, source_name, id_in_source, dry_run) |
… authors, and AI models
…reign key constraints
No description provided.