SQ-845: Normalize the dimensions table to sample and scaffold FK tables#70
Conversation
I.e. add FK tables that relate to each entry in VCFMetadataDB to samples and scaffolds in separatate child tables. This should make the dimension models 3NF compliant and will probably scale better with the number of samples and scaffolds in a project Also makes use of SQLAlchemy cascade delete-orphan. This should cover the case where the samples in a VCF are added, updated, or removed and delete old orphaned samples. Dimensions update task will call the upsert CRUD and update VCFMetadataDB, which triggers the cascade delete-orphan on the related samples and scaffolds tables, before adding the new samples and scaffolds data into the child tables.
Worker CRUD was updated along with models in previous commit. This uses eager load with selectinload() to load the data from the sample and scaffold child tables in a more efficient manner. Table joins would create a lot of duplicate data that might be memory and bandwith intensive. Lazy loading would create more queries to the db than eager loading, and selectinload should be the most efficient of the three options.
This is the migration script generated by alembic. It will drop the samples and scaffolds columns from the vcf_metadata table and will not try to insert the data into the new child tables. Instead, use divbase-cli dimensions update recreate the entries.
Needs to use FK tables instead of unnesting arrays in VCFMetadataDB model. By reimplementing here, this branch will be the source-of-thruth for the dimensions handling and can be merged into sample-metadata-functionalities later
sample-metadata-functionalities Manual "cherry-pick" of logic developed in that branch to test out the new dimensions model normalization approach.
sample-metadata-functionalities To be able to test that new FK models and CRUD works
From sample-metadata-functionalities
To be able to test the sample metadata validation and queries with larger files. The large files should not be under source control due to their potential size. The scripts use random seeds for reproducibility. Example to generate a mock VCF file with 5000 samples and 100 variants, and then generate the corresponding sample metadata TSV file with four fixed column, one warning column that will trigger validator warnings, followed by 25 random columns. bash scripts/benchmarking/generate_mock_vcf.sh -s 5000 -r 100 will generate a file named mock_vcf_5000s_100r.vcf.gz python scripts/generate_mock_sample_metadata.py --vcf mock_vcf_5000s_100r.vcf.gz --output mock_metadata_mock_vcf_5000s_100r.tsv --columns 25 --add-warning-column
Covers an edge-case that will show up with the migration in 3b3e54f if there was already data in the vcf_metadata table. This allows the dimensions update command to run to completion even if there the same S3 version of that vcf was in the table (with missing data for samples and scaffolds after the migration)
The 5000 sample test case clearly showed that this needed improvement. This adds options to control the output by count, by saving to file, and by printing to stdout. --sample-names-limit (int; default: 20) --sample-names-output <path/to/output> --sample-names-stdout
There was a problem hiding this comment.
Pull request overview
This PR refactors the VCF dimensions database schema to address a critical PostgreSQL limitation where indexing large ARRAY columns (>5000 samples) exceeded the 8KB index size limit. The solution normalizes the database design by moving samples and scaffolds from ARRAY columns to separate foreign key tables (VCFMetadataSamplesDB and VCFMetadataScaffoldsDB), allowing the system to scale to projects with thousands of samples.
Changes:
- Normalized VCF dimensions database schema with separate tables for samples and scaffolds using one-to-many relationships
- Enhanced CLI dimensions show command with new options for managing large sample lists (--unique-samples, --sample-names-limit, --sample-names-output, --sample-names-stdout)
- Added detection and automatic re-indexing of incomplete dimension entries to support migration from old schema
- Improved error handling with new DimensionsNotUpToDateWithBucketError exception for outdated dimensions
- Enhanced benchmarking scripts to support generating mock VCF files with thousands of samples
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/divbase-api/src/divbase_api/models/vcf_dimensions.py | Replaced ARRAY columns with normalized VCFMetadataSamplesDB and VCFMetadataScaffoldsDB tables using one-to-many relationships with cascade delete |
| packages/divbase-api/src/divbase_api/migrations/versions/2026-02-20_2026_02_20_normalize_dimensions_model.py | Migration script to create new FK tables and drop old ARRAY columns (data migration deferred to manual dimensions update) |
| packages/divbase-api/src/divbase_api/worker/crud_dimensions.py | Updated upsert logic to populate samples and scaffolds in separate FK tables with cascade replacement |
| packages/divbase-api/src/divbase_api/crud/vcf_dimensions.py | Added selectinload for eager loading of samples/scaffolds relationships and new functions for retrieving unique samples/scaffolds |
| packages/divbase-api/src/divbase_api/routes/vcf_dimensions.py | Added endpoints for listing unique samples and scaffolds across project VCFs |
| packages/divbase-api/src/divbase_api/routes/queries.py | Added exception handling for DimensionsNotUpToDateWithBucketError in sample_metadata_query |
| packages/divbase-api/src/divbase_api/worker/tasks.py | Added validation check for dimensions staleness and logic to detect/re-index incomplete entries from migration |
| packages/divbase-cli/src/divbase_cli/cli_commands/dimensions_cli.py | Added new CLI options for managing large sample name lists with truncation, file output, and stdout streaming |
| packages/divbase-lib/src/divbase_lib/exceptions.py | Added DimensionsNotUpToDateWithBucketError exception |
| packages/divbase-lib/src/divbase_lib/api_schemas/vcf_dimensions.py | Added DimensionsSamplesResult and DimensionsScaffoldsResult schemas |
| tests/e2e_integration/cli_commands/test_dimensions_cli.py | Added tests for new CLI options (unique-samples, unique-scaffolds, sample-names-output, sample-names-stdout, truncation) |
| tests/e2e_integration/cli_commands/test_query_cli.py | Updated test to verify DimensionsNotUpToDateWithBucketError is raised for outdated dimensions and added test for unindexed files |
| scripts/generate_mock_sample_metadata.py | Enhanced to generate configurable number of random columns and warning columns for testing |
| scripts/benchmarking/generate_mock_vcf.sh | New script to generate mock VCF files with configurable sample and variant counts using Docker |
| docker/benchmarking.dockerfile | Added tabix dependency |
Comments suppressed due to low confidence (1)
packages/divbase-api/src/divbase_api/worker/crud_dimensions.py:40
- The synchronous get_vcf_metadata_by_project function doesn't use eager loading (selectinload) for the samples and scaffolds relationships, which will cause N+1 query problems. When accessing entry.samples and entry.scaffolds in the list comprehension (lines 28-29), SQLAlchemy will issue separate queries for each VCF entry's samples and scaffolds.
The async version of this function (get_vcf_metadata_by_project_async) correctly uses selectinload to avoid this issue. This function should be updated to use the same pattern:
from sqlalchemy.orm import selectinload
stmt = select(VCFMetadataDB).where(VCFMetadataDB.project_id == project_id).options(
selectinload(VCFMetadataDB.samples), selectinload(VCFMetadataDB.scaffolds)
)This will reduce the number of queries from 1 + 2N (where N is the number of VCF files) to just 3 queries total, which is especially important given that this function is used in the query validation path where performance matters.
def get_vcf_metadata_by_project(db: Session, project_id: int) -> dict:
"""
Get all VCF metadata entries for a given project ID.
"""
stmt = select(VCFMetadataDB).where(VCFMetadataDB.project_id == project_id)
db_result = db.execute(stmt)
entries = list(db_result.scalars().all())
result = {
"project_id": project_id,
"vcf_file_count": len(entries),
"vcf_files": [
{
"vcf_file_s3_key": entry.vcf_file_s3_key,
"s3_version_id": entry.s3_version_id,
"samples": [s.sample_name for s in entry.samples],
"scaffolds": [s.scaffold_name for s in entry.scaffolds],
"variant_count": entry.variant_count,
"sample_count": entry.sample_count,
"file_size_bytes": entry.file_size_bytes,
"created_at": entry.created_at.isoformat(),
"updated_at": entry.updated_at.isoformat(),
}
for entry in entries
],
}
return result
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
As suggested by Copilot review. The test test_update_dimensions_reindexes_when_child_rows_missing from 1ce6e4d should account for this change.
RMCrean
left a comment
There was a problem hiding this comment.
Great job. thanks for the commands to test it out! All tests passing on my end too. Cool to see this fix/new approach and that you have such a nice grasp on posgres/sql.
Thanks also for the help debugging the fault on my end with my dodgy vcf file!
All minor comments :)
Feel free to do a new deployment/db migration on the dev cluster when you want, I have no plans with the cluster at the moment.
As suggested in PR review
Suggestion from code review
Suggested in PR review
|
Awesome, thanks for the review, the kind words, and for testing the commands! Fun to have seen how a dodgy vcf file affects the system too. Good idea to try this out on the dev cluster, might do that this afternoon or tomorrow! 🚀 |
Should probably have been commited as part of #70.
Flush+commit (1 transaction) instead of two commits (2 transactions) to make this atomic. If two identical dimensions update jobs are picked up by two different workers, there could a potential concurrency issue. The first upsert has postgres row locks with on_conflict_do_update and should be fine, but the FK table update added in #70 was a second transaction without row lock. In the previous CRUD, there was a window between the first and second transactions where another concurrent identical job could see the first transaction (parent row with updated counts) but see that the child tables would be empty and re-calculate the scaffolds and samples. This was not a breaking concurrency issue (no data corruption), but an inefficient one that could cause redundant work.
This PR is about updating the db models for VCF dimensions due to a recently discovered limitation in the previous implementation. In 17268d8 in the branch
sample-metadata-functionalities, I updated benchmarking scripts to generate a mock VCF with 5000 samples to test how long this would take for the TSV queries to process. It stopped already at thedivbase-cli dimensions updatestep with a postgres error. The previous model implementation used:and when this array was given 5000 elements, it exceeded the postgres byte limit for indexed columns. The default index size limit in postgres is apparently 8kb.
I thought for a while about swiching index type from the default B-tree or use JSONB + GIN indexing to keep using an array. But I think that the simplest would be to use good old database normalization. Since this is a common relational database design pattern, I think it will scale well, but this is something to keep an eye on when we get pilot users.
Normalized tables for VCF dimensions
I moved the
samplesandscaffoldscolumns fromVCFMetadataDBto their own FK tables. This way, we can still index per sample (which I suspect is needed to get the relationships to work at all). The updated models relates each entry ({VCF file, project}) in the parentVCFMetadataDBmodel to the child tablesVCFMetadataSamplesDBandVCFMetadataScaffoldsDB. I foresee that scaffolds will not scale as fast as samples, but they can just as well be treated in the same way.I thought about using
s3_version_idas the FK to account for the rare case that samples change in the VCf files, but realized that it can be easier than that. We can usevcf_metadata.idas the FK and use SQLAlchemy cascases to handle potentially orphaned entries in the child tables. If - and I think this is going to be a rare case that users changer their data like this - the dimensions update task detects that a VCF has changed samples, it will updateVCFMetadataDBand triggercascade="all, delete-orphan"to purge all entries related to that VCF from theVCFMetadataSamplesDBandVCFMetadataScaffoldsDBtables, and then insert the new current sample and scaffold values detected by the task.An alternative to
cascade="all, delete-orphan"would be to selectively update rows based on diffs. Right now, I suspect that the sampleset in each VCF file will seldomly change, so for the sake of simplificy, I decided not to go for that. Again, let's keep an eye on how the pilot users use the system.I did some reading about how to best do a db lookup for thousands of rows from a FK child table. I found that select IN eager load is likely going to be more performant than lazy load, or joined eager load. JOINS would potentially return a lot of duplicated data which would be memory and bandwidth inefficient, and a lazy load would lead to N+2 queries per N VCF files, where as the select IN load would only use 3 queries. I did not benchmark this, but with select IN load,
divbase-cli dimensions showis instantaneously fast for the 5000 samples case.UX for divbase-cli dimensions show
Now that it is possible to have e.g. 5000 samples the project dimensions, it also needs to be handled in a user-friendly way in
divbase-cli dimensions show. This is what I did:Caveat: this was a quick implementation of the first solution that came to mind. It can definitively be polished or reimagined. But I wanted to have at least some way to handle this for now 😄
Migrations
For the new db schema, the alembic-generated migration script will just drop the samples and scaffolds columns from the vcf_metadata table. Just like we assumed it would. I liked the idea we had that it is easier to just re-run
divbase-cli dimensions updatefor the projects than to try to write custom migration logic to transplant the data from the old to the new schema. In that sense, the alembic-generated migration script works fine.If you don't run
divbase-cli dimensions updatefor a project, the show command will print empty lists:Reimplementing model and CRUD related commits from sample-metadata-functionalities
To separate concerns, this PR is about refactoring the dimensions models and will be the source-of-truth for that. It will be merged into sample-metadata-functionalities later. There were quite a bit of updates to CLI, routes, CRUD, and tests related to the dimensions model in sample-metadata-functionalities, so to try to make as non-awkward as possible, I have reimplemented those parts here. I had hoped that direct cherry-picking would work, but it seemed that most of the required commits were not atomic enough for that. After opening this for review, I will merge in this branch to sample-metadata-functionalities and continue working there on the bracket array syntax.
Commands
Restart stack according to Section 4 in
docs/development/database-migrations.md(don't down -v). db_migrator should pick up the new migration and apply it. Rerundivbase-cli dimensions updatefor all existing projects that have old dimensions indexed in your db.To play with the 5000 sample use-case, try: