-
Notifications
You must be signed in to change notification settings - Fork 358
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
DHIS-16705 Program Indicator Expression transformer #19964
base: master
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
private record ProcessedExpressions(List<Expression> expressions, boolean hasChanges) {} |
Check notice
Code scanning / CodeQL
Unused classes and interfaces Note
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.
mmm, the class is actually used...
GroupByElement groupBy = new GroupByElement(); | ||
List<Expression> groupByExpressions = new ArrayList<>(); | ||
groupByExpressions.add(new Column("enrollment")); | ||
groupBy.setGroupByExpressions(groupByExpressions); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
GroupByElement.setGroupByExpressions
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 3 days ago
To fix the problem, we need to replace the deprecated setGroupByExpressions
method with its recommended alternative. According to the JSqlParser library documentation, the setGroupByExpressions
method has been replaced by the addGroupByExpression
method, which allows adding expressions individually to the GroupByElement
.
We will modify the code to use the addGroupByExpression
method instead of setGroupByExpressions
. This change will be made in the dataElementCountCte
method of the SubqueryTransformer
class.
-
Copy modified lines R101-R103
@@ -100,3 +100,5 @@ | ||
groupByExpressions.add(new Column("enrollment")); | ||
groupBy.setGroupByExpressions(groupByExpressions); | ||
for (Expression expr : groupByExpressions) { | ||
groupBy.addGroupByExpression(expr); | ||
} | ||
plainSelect.setGroupByElement(groupBy); |
...c/main/java/org/hisp/dhis/analytics/util/optimizer/cte/matcher/AbstractLastValueMatcher.java
Fixed
Show fixed
Hide fixed
...c/main/java/org/hisp/dhis/analytics/util/optimizer/cte/matcher/AbstractLastValueMatcher.java
Fixed
Show fixed
Hide fixed
...rc/main/java/org/hisp/dhis/analytics/util/optimizer/cte/matcher/DataElementCountMatcher.java
Fixed
Show fixed
Hide fixed
...c/main/java/org/hisp/dhis/analytics/util/optimizer/cte/matcher/RelationshipCountMatcher.java
Fixed
Show fixed
Hide fixed
...ytics/src/main/java/org/hisp/dhis/analytics/util/optimizer/cte/pipeline/CteSqlRebuilder.java
Fixed
Show fixed
Hide fixed
private void handleGroupBy(PlainSelect oldSelect, String fromAlias) { | ||
GroupByElement groupBy = oldSelect.getGroupBy(); | ||
if (groupBy != null) { | ||
List<Expression> groupByExpressions = groupBy.getGroupByExpressions(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
GroupByElement.getGroupByExpressions
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 3 days ago
To fix the problem, we need to replace the usage of the deprecated getGroupByExpressions()
method with its recommended alternative. According to the JSqlParser library documentation, the getGroupByExpressions()
method has been replaced by getGroupByExpressionList()
. We will update the code to use this new method.
- Replace the call to
groupBy.getGroupByExpressions()
withgroupBy.getGroupByExpressionList()
. - Update the variable name from
groupByExpressions
togroupByExpressionList
to reflect the new method.
-
Copy modified lines R201-R202 -
Copy modified line R204
@@ -200,6 +200,6 @@ | ||
if (groupBy != null) { | ||
List<Expression> groupByExpressions = groupBy.getGroupByExpressions(); | ||
if (groupByExpressions != null) { | ||
List<Expression> groupByExpressionList = groupBy.getGroupByExpressionList(); | ||
if (groupByExpressionList != null) { | ||
List<Expression> newGroupByExpressions = new ArrayList<>(); | ||
for (Expression expr : groupByExpressions) { | ||
for (Expression expr : groupByExpressionList) { | ||
if (expr instanceof Column col) { |
newGroupByExpressions.add(expr); // Keep other expressions | ||
} | ||
} | ||
groupBy.setGroupByExpressions(newGroupByExpressions); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
GroupByElement.setGroupByExpressions
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 3 days ago
To fix the problem, we need to replace the usage of the deprecated setGroupByExpressions
method with its recommended alternative. The GroupByElement
class provides a method addGroupByExpression
which can be used to add expressions to the group by clause individually. We will iterate over the newGroupByExpressions
list and add each expression using the addGroupByExpression
method.
-
Copy modified lines R214-R217
@@ -213,3 +213,6 @@ | ||
} | ||
groupBy.setGroupByExpressions(newGroupByExpressions); | ||
groupBy.getGroupByExpressions().clear(); | ||
for (Expression newExpr : newGroupByExpressions) { | ||
groupBy.addGroupByExpression(newExpr); | ||
} | ||
} |
...vice-analytics/src/test/java/org/hisp/dhis/analytics/util/vis/ExpressionTransformerTest.java
Fixed
Show fixed
Hide fixed
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.
It looks good, just minor comments/suggestions.
Cheers
@@ -474,6 +474,8 @@ public enum ErrorCode { | |||
E7146("A {0} date was not specified in periods, dimensions, filters"), | |||
E7147("Query failed because of a missing column: `{0}`"), | |||
E7148("Could not create CTE SQL query, unexpected error: `{0}`"), | |||
E7149("Could not pre-pend CTEs to Program Indicator CTE, unexpected error: `{0}`"), |
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.
E7149("Could not pre-pend CTEs to Program Indicator CTE, unexpected error: `{0}`"), | |
E7149("Could not prepend CTEs to Program Indicator CTE, unexpected error: `{0}`"), |
: getAggregatedEnrollmentsSql(params, maxLimit); | ||
} | ||
System.out.println(sql); |
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.
Forgotten debug line
* @throws CteOptimizerException if an error occurs during parsing | ||
*/ | ||
public Statement parse(String sql) { | ||
if (StringUtils.isEmpty(sql)) { |
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.
if (StringUtils.isEmpty(sql)) { | |
if (StringUtils.isBlank(sql)) { |
* @param generatedCtes the collection of new CTE definitions. | ||
*/ | ||
public static void appendExtractedCtes(Select select, Map<String, GeneratedCte> generatedCtes) { | ||
List<WithItem> existingWithItems = select.getWithItemsList(); |
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 believe it's old code, but maybe select.getWithItemsList()
could return an empty list, instead of a null?
from %s | ||
where %s is not null | ||
) t | ||
WHERE rn = 1 |
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.
WHERE rn = 1 | |
where rn = 1 |
@@ -1453,12 +1454,11 @@ protected List<String> getSelectColumnsWithCTE(EventQueryParams params, CteConte | |||
if (queryItem.isProgramIndicator()) { | |||
// For program indicators, use CTE reference | |||
String piUid = queryItem.getItem().getUid(); | |||
CteDefinition cteDef = cteContext.getDefinitionByItemUid(piUid); | |||
// COALESCE(fbyta.value, 0) as CH6wamtY9kK |
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.
Forgotten comment
return Optional.of((PlainSelect) selectBody); | ||
} | ||
|
||
protected Optional<Expression> hasSingleExpression(PlainSelect select) { |
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.
Should it be getSingleExpression
?
|
||
protected Optional<SelectExpressionItem> extractSingleSelectExpressionItem(PlainSelect plain) { | ||
List<SelectItem> items = plain.getSelectItems(); | ||
if (items == null || items.size() != 1) { |
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.
if (items == null || items.size() != 1) { | |
if (CollectionUtils.size(items) != 1) { |
|
||
protected Optional<Expression> hasSingleExpression(PlainSelect select) { | ||
List<SelectItem> selectItems = select.getSelectItems(); | ||
if (selectItems == null || selectItems.size() != 1) { |
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.
if (selectItems == null || selectItems.size() != 1) { | |
if (CollectionUtils.size(selectItems) != 1) { |
boolean changed = false; | ||
|
||
for (Expression expr : expressions) { | ||
if (expr == null) { |
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 to add null
to the list?
I have the impression that it will not affect consumers: if the expression is null, there is nothing to deal with, so should we even set null values?
c5c674f
to
c9212dc
Compare
c9212dc
to
5c78ff5
Compare
|
Summary
Work in progress
This PR addresses an issue with Enrollments and Events queries that contain a PI expression with internal subqueries as
where
condition.These type of queries do not run in Doris, because Doris does not support correlation with outer layers of the parent query.
Example of a non supported query:
This PR introduces a new component named
CteOptimizationPipeline
that has the following responsibilities:org.hisp.dhis.parser.expression.statement.DefaultStatementBuilder
so they are easy to identify).where
condition so that the CTEs are referenced withjoin
statements and thewhere
condition is preserved (including the function chain).Transformation examples
Source
Target
What is not working
Support for multiple PI CTEs