Simplify clinical data binning related SQL #10823

onursumer · 2024-06-06T19:53:33Z

Use clinical data counts derived from the new clinical_data_derived table instead of fetching all the actual clinical data
Simplify NA count logic. Now NA data is actually in the database, so we just need the get that NA count.

onursumer · 2024-06-21T18:00:03Z

src/main/java/org/cbioportal/web/util/ClinicalDataBinUtil.java

@@ -310,17 +310,33 @@ public List<ClinicalDataBin> calculateStaticDataBins(
        List<String> filteredUniqueSampleKeys,
        List<String> filteredUniquePatientKeys
    ) {
-        return NewClinicalDataBinUtil.calculateStaticDataBins(


This is to revert the legacy implementation for the legacy endpoint. We can't use NewClinicalDataBinUtil.calculateDynamicDataBins for legacy endpoint anymore since the underlying logic changed significantly.

onursumer · 2024-06-21T18:18:37Z

...in/resources/org/cbioportal/persistence/mybatisclickhouse/StudyViewClinicalDataBinMapper.xml

+    <select id="getPatientClinicalDataCountsForBinning">
+        SELECT
+            attribute_name as attributeId,
+            if(attribute_value='', 'NA', attribute_value) AS value,
+            count(value) as count
+        FROM clinical_data_derived
+        <include refid="patientClinicalDataFromStudyViewFilter" />
+        AND type = 'patient'
+        GROUP BY attribute_name, value
+    </select>
+
+    <select id="getSampleClinicalDataCountsForBinning">
+        SELECT
+            attribute_name as attributeId,
+            if(attribute_value='', 'NA', attribute_value) AS value,
+            count(value) as count
+        FROM clinical_data_derived
+        <include refid="sampleClinicalDataFromStudyViewFilter" />
+        AND type = 'sample'
+        GROUP BY attribute_name, value
+    </select>


These queries are very similar to getClinicalDataCounts query. We may be able to directly use that one instead of adding new SQL.

cbioportal/src/main/resources/org/cbioportal/persistence/mybatisclickhouse/StudyViewMapper.xml

Lines 66 to 74 in b436b2b

<select id="getClinicalDataCounts" resultType="org.cbioportal.model.ClinicalDataCount">

<include refid="getCategoricalClinicalDataCountsQuerySample">

<property name="table_name_prefix" value="sample"/>

</include>

UNION ALL

<include refid="getCategoricalClinicalDataCountsQueryPatient">

<property name="table_name_prefix" value="patient"/>

</include>

</select>

src/main/java/org/cbioportal/web/columnar/ClinicalDataBinner.java

haynescd · 2024-06-24T15:51:17Z

src/main/java/org/cbioportal/web/columnar/ClinicalDataBinner.java

@@ -26,6 +26,26 @@ public ClinicalDataBinner(
        this.dataBinner = dataBinner;
    }

+    // TODO move this to a utility class?
+    public List<ClinicalData> convertCountsToData(List<ClinicalDataCount> clinicalDataCounts)


This should be private

haynescd · 2024-06-24T16:01:57Z

src/main/java/org/cbioportal/web/util/DataBinner.java

+    /**
+     * Calculate number of clinical data marked actually as "NA", "NAN", or "N/A"
+     */
+    public Long countNAs(List<Binnable> clinicalData) {


This is cast to Long and then later it grabs just the intValue(). Do you need the wrapper class?

bin.setCount(countNAs(clinicalData).intValue());

Probably we don't need a wrapper here. For now, just keeping it consistent with the legacy countNAs method.

cbioportal/src/main/java/org/cbioportal/web/util/DataBinner.java

Line 621 in de88ff7

public Long countNAs(List<Binnable> clinicalData, ClinicalDataType clinicalDataType, List<String> ids) {

We can revisit this once we completely get rid of the legacy functionality.

onursumer · 2024-06-28T14:51:02Z

src/main/resources/org/cbioportal/persistence/mybatisclickhouse/StudyViewMapper.xml

@@ -168,10 +171,13 @@
    </select>


-    <sql id="getCategoricalClinicalDataCountsQuerySample">
+    <sql id="getClinicalDataCountsQuerySample">


renamed because this SQL returns both categorical and numeric

onursumer · 2024-06-28T14:51:07Z

src/main/resources/org/cbioportal/persistence/mybatisclickhouse/StudyViewMapper.xml

@@ -198,10 +204,13 @@
        value
    </sql>

-    <sql id="getCategoricalClinicalDataCountsQueryPatient">
+    <sql id="getClinicalDataCountsQueryPatient">


renamed because this SQL returns both categorical and numeric

onursumer · 2024-06-28T14:55:53Z

src/main/resources/org/cbioportal/persistence/mybatisclickhouse/StudyViewFilterMapper.xml

+                    </include>
+                </if>
+                <if test="dataFilterValue.start != null || dataFilterValue.end != null">
+                    AND match(attribute_value, '^[\d\.]+$')


this ignores edge cases like <18, >90, etc. there is a separate ticket for this cBioPortal/rfc80-team#32

src/main/java/org/cbioportal/web/util/DataBinner.java

src/main/java/org/cbioportal/web/columnar/ClinicalDataBinner.java

alisman · 2024-07-02T17:34:08Z

src/main/java/org/cbioportal/web/util/DataBinner.java

+        List<DataBin> dataBins,
+        ClinicalDataType clinicalDataType,
+        List<Binnable> clinicalData,
+        List<String> ids


can you improve the variable name "ids" to something more decsriptive?

I think this one is now only used by the legacy endpoint. The new one doesn't need to deal with sample/patient ids anymore. We should probably add comments to the methods that are now only needed for the legacy endpoint so that we can remove them later once we completely migrate to the new implementation.

src/main/java/org/cbioportal/web/columnar/util/NewClinicalDataBinUtil.java

sonarqubecloud · 2024-07-10T19:52:37Z

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
35.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

haynescd · 2024-07-11T14:44:31Z

Looks like most of the sonar issues are around the deprecated fn.

except for this one. https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&pullRequest=10823&id=cBioPortal_cbioportal&open=AZCdbAVZBEMFXTbxsIZU

haynescd

👍

* simplify clinical data binning related SQL * fix numericalClinicalDataCountFilter

onursumer force-pushed the count-na-sql branch 3 times, most recently from b34da51 to a3624e0 Compare June 21, 2024 00:16

onursumer changed the title ~~Move NA count logic into SQL~~ Simplify clinical data binning related SQL Jun 21, 2024

onursumer force-pushed the count-na-sql branch from a3624e0 to d927820 Compare June 21, 2024 18:07

onursumer commented Jun 21, 2024

View reviewed changes

onursumer marked this pull request as ready for review June 21, 2024 18:22

onursumer requested review from alisman and haynescd June 21, 2024 18:22

haynescd reviewed Jun 24, 2024

View reviewed changes

src/main/java/org/cbioportal/web/columnar/ClinicalDataBinner.java Show resolved Hide resolved

haynescd reviewed Jun 24, 2024

View reviewed changes

onursumer force-pushed the count-na-sql branch 5 times, most recently from 4b73039 to 1c383c6 Compare June 27, 2024 21:51

onursumer commented Jun 28, 2024

View reviewed changes

onursumer force-pushed the count-na-sql branch 3 times, most recently from 00157cc to c4ae8d0 Compare July 1, 2024 20:22

alisman reviewed Jul 2, 2024

View reviewed changes

src/main/java/org/cbioportal/web/util/DataBinner.java Show resolved Hide resolved

alisman reviewed Jul 2, 2024

View reviewed changes

src/main/java/org/cbioportal/web/columnar/ClinicalDataBinner.java Show resolved Hide resolved

alisman reviewed Jul 2, 2024

View reviewed changes

src/main/java/org/cbioportal/web/columnar/util/NewClinicalDataBinUtil.java Show resolved Hide resolved

onursumer force-pushed the count-na-sql branch 3 times, most recently from 9e0d230 to b2dba31 Compare July 3, 2024 17:47

alisman previously approved these changes Jul 8, 2024

View reviewed changes

onursumer force-pushed the count-na-sql branch from b2dba31 to cd18bd5 Compare July 10, 2024 16:09

onursumer added 2 commits July 10, 2024 15:42

simplify clinical data binning related SQL

9a805a5

fix numericalClinicalDataCountFilter

bd8d8b6

onursumer dismissed alisman’s stale review via bd8d8b6 July 10, 2024 19:45

onursumer force-pushed the count-na-sql branch from cd18bd5 to bd8d8b6 Compare July 10, 2024 19:45

haynescd approved these changes Jul 11, 2024

View reviewed changes

haynescd merged commit 0dfb346 into cBioPortal:demo-rfc80-poc Jul 11, 2024
10 of 13 checks passed

onursumer mentioned this pull request Aug 5, 2024

Move NA count logic to SQL cBioPortal/rfc80-team#16

Closed

onursumer deleted the count-na-sql branch August 22, 2024 12:53

haynescd pushed a commit that referenced this pull request Nov 25, 2024

Simplify clinical data binning related SQL (#10823)

d50433d

* simplify clinical data binning related SQL * fix numericalClinicalDataCountFilter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify clinical data binning related SQL #10823

Simplify clinical data binning related SQL #10823

onursumer commented Jun 6, 2024 •

edited

Loading

onursumer Jun 21, 2024

onursumer Jun 21, 2024

haynescd Jun 24, 2024

haynescd Jun 24, 2024

onursumer Jun 26, 2024

onursumer Jun 28, 2024

onursumer Jun 28, 2024

onursumer Jun 28, 2024

alisman Jul 2, 2024

onursumer Jul 2, 2024

sonarqubecloud bot commented Jul 10, 2024

haynescd commented Jul 11, 2024

haynescd left a comment

	<select id="getClinicalDataCounts" resultType="org.cbioportal.model.ClinicalDataCount">
	<include refid="getCategoricalClinicalDataCountsQuerySample">
	<property name="table_name_prefix" value="sample"/>
	</include>
	UNION ALL
	<include refid="getCategoricalClinicalDataCountsQueryPatient">
	<property name="table_name_prefix" value="patient"/>
	</include>
	</select>

Simplify clinical data binning related SQL #10823

Simplify clinical data binning related SQL #10823

Conversation

onursumer commented Jun 6, 2024 • edited Loading

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

sonarqubecloud bot commented Jul 10, 2024

Quality Gate passed

haynescd commented Jul 11, 2024

haynescd left a comment

Choose a reason for hiding this comment

onursumer commented Jun 6, 2024 •

edited

Loading