Skip to content
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

[DRAFT] fix: Use CTE in Enrollment analytics queries [DHIS-16705] #19519

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

luciano-fiandesio
Copy link
Contributor

@luciano-fiandesio luciano-fiandesio commented Dec 18, 2024

WIP

Changes

This PR addresses the generation of SQL queries for Enrollment analytics

Problem

Currently, the Enrollment queries are structured so that sub-queries are used to fetch events values from the analytics_event_* tables.
For instance:

select
    enrollment,
    ...
    ax."ou",
    (select
         "fyjPqlHE7Dn"
     from
	 analytics_event_M3xtLkYBlKI
     where
	 analytics_event_M3xtLkYBlKI.eventstatus != 'SCHEDULE'
	 and analytics_event_M3xtLkYBlKI.enrollment = ax.enrollment
	 and ps = 'CWaAcQYKVpq'
limit 1 ) as "CWaAcQYKVpq[-1].fyjPqlHE7Dn",
from
     analytics_enrollment_m3xtlkyblki as ax
...

The above query works in Postgres but does not work in Doris, because correlation with outer layers of the parent query is not supported.

Mitigation

The current approach is trying to refactor the Enrollment query so that the subqueries (both as select and as where conditions) are "moved" into CTE (Common Table Expressions).
Common Table Expressions are temporary result sets in SQL, defined within a WITH clause, that simplify complex queries by improving readability and modularizing logic. They are particularly useful for recursive queries and can be referenced multiple times within the main query.

The above query can be rewritten like so:

with ps_cwaacqykvpq_fyjpqlhe7dn as (
select
	distinct on
	(enrollment) enrollment,
	"fyjPqlHE7Dn" as value
from
	analytics_event_M3xtLkYBlKI
where
	eventstatus != 'SCHEDULE'
	and ps = 'CWaAcQYKVpq'
	
order by
	enrollment,
	occurreddate desc,
	created desc)
select
	ax.enrollment,
	...
	ps_cwaacqykvpq_fyjpqlhe7dn.value as "CWaAcQYKVpq.fyjPqlHE7Dn"
from
	analytics_enrollment_m3xtlkyblki as ax
left join ps_cwaacqykvpq_fyjpqlhe7dn on
	ax.enrollment = ps_cwaacqykvpq_fyjpqlhe7dn.enrollment
where
	(((enrollmentdate >= '2021-01-01'
	and enrollmentdate < '2022-01-01')))
	and (ax."uidlevel1" = 'ImspTQPwCqd' )
...

The above query structure is compatible with Doris and it makes the execution of the query faster.
As a comparison:

Original query with sub-select

image

Refactored query with CTEs

image

Testing strategy

I am using the e2e project and aim at having 100% green tests on both Postgres and Doris.


void contributeCTE(
ProgramIndicator programIndicator,
RelationshipType relationshipType,

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'relationshipType' is never used.
return fromClause.toString();
}

protected String getSortClause(EventQueryParams params) {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
AbstractJdbcEventAnalyticsManager.getSortClause
; it is advisable to add an Override annotation.
@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch from 8c33952 to 3e070ce Compare December 18, 2024 16:56
@larshelge larshelge changed the title DHIS-16705 Use CTE in Enrollment analytics queries [DRAFT] Use CTE in Enrollment analytics queries [DHIS-16705] Dec 19, 2024
@larshelge larshelge changed the title [DRAFT] Use CTE in Enrollment analytics queries [DHIS-16705] [DRAFT] fix: Use CTE in Enrollment analytics queries [DHIS-16705] Dec 19, 2024
@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch from 5170115 to d8119c0 Compare December 20, 2024 15:19
@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch 2 times, most recently from 5c4dba5 to ef43c5b Compare January 3, 2025 16:19
// The CTE definition (the SQL query)
@Getter private final String cteDefinition;
// The calculated offset
@Getter private final List<Integer> offsets = new ArrayList<>();

Check notice

Code scanning / CodeQL

Exposing internal representation Note

getOffsets exposes the internal representation stored in field offsets. The value may be modified
after this call to getOffsets
.
this.cteDefinition = cteDefinition;
this.offsets.add(offset);
// one alias per offset
this.alias = new RandomStringGenerator.Builder().withinRange('a', 'z').build().generate(5);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
Builder.build
should be avoided because it has been deprecated.
this.programIndicatorUid = programIndicatorUid;
this.programStageUid = null;
// ignore offset
this.alias = new RandomStringGenerator.Builder().withinRange('a', 'z').build().generate(5);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
Builder.build
should be avoided because it has been deprecated.
this.programIndicatorUid = null;
this.programStageUid = programStageUid;
// ignore offset
this.alias = new RandomStringGenerator.Builder().withinRange('a', 'z').build().generate(5);

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
Builder.build
should be avoided because it has been deprecated.
}

if (offset < 0) {
return (-1 * offset);

Check failure

Code scanning / CodeQL

User-controlled data in arithmetic expression High

This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.
This arithmetic expression depends on a
user-provided value
, potentially causing an underflow.

Copilot Autofix AI about 5 hours ago

To fix the problem, we need to validate the offset parameter before performing arithmetic operations on it. Specifically, we should ensure that the offset value is within a safe range to prevent overflow or underflow. We can achieve this by adding a guard clause that checks the value of offset and handles extreme values appropriately.

The best way to fix the problem without changing existing functionality is to add a validation step in the computeRowNumberOffset method. We will check if the offset value is within the range of Integer.MIN_VALUE to Integer.MAX_VALUE and handle cases where it is not.

Suggested changeset 1
dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java
--- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java
+++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/event/data/JdbcEnrollmentAnalyticsManager.java
@@ -1167,4 +1167,10 @@
     if (offset < 0) {
+      if (offset == Integer.MIN_VALUE) {
+        throw new IllegalArgumentException("Offset value is too small and causes underflow.");
+      }
       return (-1 * offset);
     } else {
+      if (offset == Integer.MAX_VALUE) {
+        throw new IllegalArgumentException("Offset value is too large and causes overflow.");
+      }
       return (offset - 1);
EOF
@@ -1167,4 +1167,10 @@
if (offset < 0) {
if (offset == Integer.MIN_VALUE) {
throw new IllegalArgumentException("Offset value is too small and causes underflow.");
}
return (-1 * offset);
} else {
if (offset == Integer.MAX_VALUE) {
throw new IllegalArgumentException("Offset value is too large and causes overflow.");
}
return (offset - 1);
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch 2 times, most recently from aa533b4 to 5f05bb5 Compare January 6, 2025 13:50
@@ -578,6 +735,29 @@
return ColumnAndAlias.EMPTY;
}

protected String getColumnWithCte(QueryItem item, String suffix, CTEContext cteContext) {
List<String> columns = new ArrayList<>();
String colName = item.getItemName();

Check notice

Code scanning / CodeQL

Unread local variable Note

Variable 'String colName' is never read.
import org.hisp.dhis.program.ProgramIndicator;
import org.hisp.dhis.program.ProgramStage;

public class CTEContext {
Copy link
Member

@larshelge larshelge Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to CteContext. Use Pascal-case for acronyms in class names in Java.

https://www.oreilly.com/library/view/java-8-pocket/9781491901083/ch01.html

* @param offset The calculated offset
* @param isRowContext Whether the CTE is a row context
*/
public void addCTE(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to addCte.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, renamed also other methods CTE -> Cte on the same class

return "";
}

StringBuilder sb = new StringBuilder("WITH ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use lower-case for SQL keywords in Java, i.e. with.

case LT -> "<";
case GE -> ">=";
case LE -> "<=";
case IN -> "IN";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lower-case for SQL keywords in Java, e.g. in.


/** Returns true if incident date criteria exists in dimensions or filters. */
public boolean hasIncidentDateCriteria() {
return getDimensionOrFilterItems("incidentDate").size() > 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use !isEmpty() over .size() > 0 for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed function, as it looks like it wasn't used

@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch from 5f05bb5 to 4e085d3 Compare January 7, 2025 09:08
@luciano-fiandesio luciano-fiandesio force-pushed the DHIS-16705_ENROLLMENT_WITH_CTE branch from 4e085d3 to 62c2237 Compare January 7, 2025 09:09
Copy link

sonarqubecloud bot commented Jan 7, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
23 New issues
23 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

void contributeCTE(
ProgramIndicator programIndicator,
RelationshipType relationshipType,
AnalyticsType outerSqlEntity,

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'outerSqlEntity' is never used.
/**
* Returns a select SQL clause for the given query.
*
* @param params the {@link EventQueryParams}.
*/
protected abstract String getSelectClause(EventQueryParams params);

protected abstract String getColumnWithCte(QueryItem item, String suffix, CteContext cteContext);

Check notice

Code scanning / CodeQL

Useless parameter Note

The parameter 'suffix' is never used.
@@ -565,6 +722,29 @@
return ColumnAndAlias.EMPTY;
}

protected String getColumnWithCte(QueryItem item, String suffix, CteContext cteContext) {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
AbstractJdbcEventAnalyticsManager.getColumnWithCte
; it is advisable to add an Override annotation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants