-
-
Notifications
You must be signed in to change notification settings - Fork 748
Clickhouse only db #11704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Clickhouse only db #11704
Conversation
…1673) Replace foreach loops generating multiple prepared statement parameters with single array parameter using ArrayTypeHandler. This significantly improves performance with ClickHouse JDBC connections by reducing parameter overhead. - Use CONCAT(study_id, ':', patient_id) with ArrayTypeHandler - Add SqlUtils.combineStudyAndPatientIds() utility method - Apply optimization to both patient and sample lookup queries - Maintain security through proper parameter binding 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <[email protected]>
Co-authored-by: Bryan Lai <[email protected]>
…C performance (#11703) Apply ArrayTypeHandler optimization strategy to whereSample include, following the same approach used in PatientMapper (commit 2e2ec22). This significantly improves performance with ClickHouse JDBC connections by reducing parameter overhead. Changes: - Replace foreach loops in whereSample with ArrayTypeHandler for both single-study and multi-study queries - Use SqlUtils.combineStudyAndPatientIds() for multi-study scenarios with CONCAT-based unique key matching - Optimize getClinicalAttributeCountsBySampleIds query performance through updated whereSample include This reduces prepared statement parameters from potentially thousands to single array parameters, maintaining security through proper parameter binding. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <[email protected]>
…rmance Apply ArrayTypeHandler optimization strategy across all high-priority MyBatis mappers to dramatically improve ClickHouse JDBC performance by reducing prepared statement parameter overhead. SqlUtils Enhancements: - Add listToArray() utility method to convert List<String> to String[] for ArrayTypeHandler - Extend combineStudyAndPatientIds() usage for multi-study query optimization Optimized MyBatis Mappers (11 files): - ClinicalAttributeMapper.xml - clinical attribute count queries with sample IDs - ClinicalDataMapper.xml - sample and patient clinical data queries - ClinicalEventMapper.xml - clinical events by sample and patient IDs - CopyNumberSegmentMapper.xml - copy number segment queries - DiscreteCopyNumberMapper.xml - discrete copy number queries - MutationMapper.xml - mutation queries with sample/profile pairs - NamespaceMapper.xml - sample ID namespace queries - SampleMapper.xml - sample queries with study/sample and study/patient pairs - StructuralVariantMapper.xml - structural variant queries - TreatmentMapper.xml - treatment sample ID queries Optimization Strategy Applied: - Single-study queries: Use <bind> + ArrayTypeHandler for direct List→Array conversion - Multi-study queries: Use SqlUtils.combineStudyAndPatientIds() with CONCAT matching - Replace foreach loops generating multiple prepared statement parameters - Use proper <bind> elements to avoid MyBatis parameter binding errors Performance Impact: - Reduces prepared statement parameters from potentially thousands to single arrays - Follows same proven optimization pattern from PatientMapper (commit 2e2ec22) - Maintains security through proper parameter binding - Significant improvement for ClickHouse JDBC connections 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
2e9c7ae to
688a552
Compare
Replace individual fetchGenePanelDataByMolecularProfileId calls with a single fetchGenePanelDataByMolecularProfileIds batch call to reduce database round trips and improve performance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
6f65b0a to
ba502c6
Compare
| structuralVariantQueries) | ||
| .stream()) | ||
| .collect(Collectors.toList()); | ||
| return structuralVariantMapper.fetchStructuralVariants( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change was made because fetching all profiles at once is much more performant. it is always true that sampleId collection and molecular profile collection will be of equal length
Simplified the method to call mutationMapper.getMutationsInMultipleMolecularProfiles directly instead of iterating through grouped cases and making multiple calls. This reduces database round trips and improves performance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Simplified the method to call structuralVariantMapper.fetchStructuralVariants directly with all molecular profile IDs instead of iterating through grouped cases and making multiple calls. This reduces database round trips and improves performance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Co-authored-by: Bryan Lai <[email protected]>
…of profiling status for genes which are covered by the same panels
This commit optimizes the discrete copy number queries in DiscreteCopyNumberMapper by reordering the FROM clause to start with sample_cna_event instead of cna_event, and using subqueries to filter by genetic_profile_id instead of joining the full genetic_profile table. Changes: - Reordered FROM clause to start with sample_cna_event table - Replaced genetic_profile.stable_id joins with subquery lookups - Filters on genetic_profile_id directly in sample_cna_event table - Improves query performance by reducing join complexity This optimization should improve query performance for discrete copy number alteration lookups, especially for large datasets. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
This commit introduces a more efficient studyExists() method for validating study existence without fetching full study objects, and fixes a ClickHouse SQL compatibility issue in the StudyMapper. Changes: 1. Added StudyService.studyExists() method - Fetches only study IDs instead of full objects - Throws StudyNotFoundException if study doesn't exist - More efficient than getStudy() when only validation is needed 2. Replaced getStudy() with studyExists() across codebase - Updated 29 call sites where return value wasn't used - Affected services: Clinical, Sample, Patient, MolecularProfile, etc. 3. Fixed ClickHouse SQL error in StudyMapper.xml - Added cancer_study_identifier to GROUP BY clause - Resolves: "Column is not under aggregate function and not in GROUP BY keys" - Required for ORDER BY cancer_study_identifier to work with ClickHouse 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
* Fix zero value handling in molecular data truncation Add special case handling for zero values in the molecular data value truncation logic to prevent them from being converted incorrectly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * Update src/main/resources/org/cbioportal/legacy/persistence/mybatis/MolecularDataMapper.xml Co-authored-by: Onur Sumer <[email protected]> --------- Co-authored-by: Claude <[email protected]> Co-authored-by: Onur Sumer <[email protected]>
* ♻️ Refactor Cancer Study PermissionEvaluator * 🔒 Add Support for CancerStudyMetadata obj * Update field typeOfCancer to cancerType
* Fix sample list query for clickhouse * Change cancer study details query in sample list query * Clean up SampleListMapper.xml by removing unused prefix properties
Fix SQL issues in Mutation Mapper migrated to mutation_derived table. Fix issues with clickhouse.sql so it runs in test context Fix CH problem with getMutationCountByPosition query update pom.xml with new derived table version
…nly-db # Conflicts: # pom.xml # src/main/resources/org/cbioportal/legacy/persistence/mybatis/CosmicCountMapper.xml # src/test/java/org/cbioportal/legacy/persistence/mybatis/CosmicCountMyBatisRepositoryTest.java
7ff1ddf to
2164b45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this empty file?
| List<ReferenceGenomeGene> genes = geneMemoizerService.fetchGenes(genomeName); | ||
| if (genes == null) { | ||
| genes = referenceGenomeGeneService.fetchAllReferenceGenomeGenes(genomeName); | ||
| geneMemoizerService.cacheGenes(genes, genomeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place where we use the gene memoizer service. We should probably remove the interface and the implementation class as well.
| COUNT(*) AS totalCount, | ||
| COUNT(DISTINCT(CASE_ID)) AS numberOfAlteredCases | ||
| COUNT(DISTINCT(case_id)) AS numberOfAlteredCases, | ||
| 2 AS QueryNumber |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this serve any purpose?
| <!-- TODO: check this--> | ||
| ANY_VALUE(gene1.hugo_gene_symbol) AS gene1HugoGeneSymbol, | ||
| gene2.entrez_gene_id AS gene2EntrezGeneId, | ||
| ANY_VALUE(gene2.hugo_gene_symbol) AS gene2HugoGeneSymbol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to fix the ANY_VALUE issue here anymore, right? I guess we can remove the TODO comment
| <!-- TODO: check this--> | ||
| ANY_VALUE(gene1.hugo_gene_symbol) AS gene1HugoGeneSymbol, | ||
| gene2.entrez_gene_id AS gene2EntrezGeneId, | ||
| ANY_VALUE(gene2.hugo_gene_symbol) AS gene2HugoGeneSymbol, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
| <if test="!clinicalEvents.isEmpty()"> | ||
| AND | ||
| <foreach item="element" collection="clinicalEvents" open="(" separator="OR " close=")"> | ||
| <if test="element.attributes == null || element.attributes.isEmpty()"> | ||
| clinical_event.event_type = #{element.eventType} | ||
| </if> | ||
| <if test="element.attributes != null and !element.attributes.isEmpty()"> | ||
| (CONCAT(clinical_event.event_type, '_', clinical_event_data.key)) IN | ||
| <foreach item="attribute" collection="element.attributes" open="(" separator="," close=")"> | ||
| CONCAT(#{element.eventType}, '_', #{attribute.key}) | ||
| </foreach> | ||
| </if> | ||
| </foreach> | ||
| </if> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a new addition. Probably addressing a legacy bug. This broke one of the legacy tests because of a missing clinicalEvents param.
| <!-- TODO: check this--> | ||
| ANY_VALUE(gene.hugo_gene_symbol) AS "hugoGeneSymbol", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this TODO as well?
| <!-- TODO: check this--> | ||
| ANY_VALUE(GENE.hugoGeneSymbol) AS "hugoGeneSymbol", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this TODO?
| <!--TODO : FIX--> | ||
| <!--COUNT(CASE WHEN sample_list.stable_id = CONCAT(cancer_study.cancer_study_identifier,'_all') THEN 1 ELSE NULL END) AS allSampleCount--> | ||
| 1 AS allSampleCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is fixed in the new clickhouse implementation, so we don't use this getStudies mapping anymore, right?
| <!--TODO: FIX--> | ||
| <!--COUNT(CASE WHEN sample_list.stable_id = CONCAT(cancer_study.cancer_study_identifier,'_all') THEN 1 ELSE NULL END) AS allSampleCount--> | ||
| 1 AS allSampleCount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
|
#11790 addresses some of my review comments above |
The method name combineStudyAndPatientIds was confusing because it was often used with sampleIds and molecularProfileIds, not just patientIds. Renamed to combineStudyAndEntityIds to better reflect its generic purpose of combining study IDs with any type of entity IDs. Updated all 15 usages across MyBatis mapper XML files: - TreatmentMapper.xml - CopyNumberSegmentMapper.xml - DiscreteCopyNumberMapper.xml - MutationMapper.xml - ClinicalEventMapper.xml (2 usages) - NamespaceMapper.xml - StructuralVariantMapper.xml - ClinicalDataMapper.xml (2 usages) - SampleMapper.xml (2 usages) - PatientMapper.xml (2 usages) - ClinicalAttributeMapper.xml 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
No description provided.