Skip to content

Commit 43d4175

Browse files
larshelgelarshelge
authored
refactor: Remove unnecessary data filter clause [DHIS2-16705] (#19381)
1 parent 8fcf3bf commit 43d4175

File tree

5 files changed

+31
-73
lines changed

5 files changed

+31
-73
lines changed

‎dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/AbstractEventJdbcTableManager.java‎

Lines changed: 8 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -27,18 +27,17 @@
2727
*/
2828
package org.hisp.dhis.analytics.table;
2929

30-
import static org.apache.commons.lang3.StringUtils.EMPTY;
3130
import static org.hisp.dhis.analytics.table.model.Skip.SKIP;
3231
import static org.hisp.dhis.analytics.util.AnalyticsUtils.getClosingParentheses;
3332
import static org.hisp.dhis.analytics.util.AnalyticsUtils.getColumnType;
3433
import static org.hisp.dhis.db.model.DataType.GEOMETRY;
3534
import static org.hisp.dhis.db.model.DataType.TEXT;
36-
import static org.hisp.dhis.system.util.MathUtils.NUMERIC_LENIENT_REGEXP;
3735

3836
import java.util.ArrayList;
3937
import java.util.List;
4038
import java.util.Map;
4139
import java.util.stream.Collectors;
40+
import org.apache.commons.lang3.Validate;
4241
import org.hisp.dhis.analytics.AnalyticsTableHookService;
4342
import org.hisp.dhis.analytics.partition.PartitionManager;
4443
import org.hisp.dhis.analytics.table.model.AnalyticsDimensionType;
@@ -99,14 +98,6 @@ public AbstractEventJdbcTableManager(
9998

10099
public static final String OU_NAME_COL_SUFFIX = "_name";
101100

102-
protected final String getNumericClause() {
103-
return " and " + sqlBuilder.regexpMatch("value", "'" + NUMERIC_LENIENT_REGEXP + "'");
104-
}
105-
106-
protected final String getDateClause() {
107-
return " and " + sqlBuilder.regexpMatch("value", DATE_REGEXP);
108-
}
109-
110101
/**
111102
* Indicates whether creating an index should be skipped.
112103
*
@@ -148,22 +139,6 @@ protected String getColumnExpression(ValueType valueType, String columnExpressio
148139
}
149140
}
150141

151-
/**
152-
* For numeric and date value types, returns a data filter clause for checking whether the value
153-
* is valid according to the value type. For other value types, returns the empty string.
154-
*
155-
* @param attribute the {@link TrackedEntityAttribute}.
156-
* @return a data filter clause.
157-
*/
158-
protected String getDataFilterClause(TrackedEntityAttribute attribute) {
159-
if (attribute.isNumericType()) {
160-
return getNumericClause();
161-
} else if (attribute.isDateType()) {
162-
return getDateClause();
163-
}
164-
return EMPTY;
165-
}
166-
167142
/**
168143
* Returns a cast expression which includes a value filter for the given value type.
169144
*
@@ -225,11 +200,10 @@ protected List<AnalyticsTableColumn> getColumnForAttribute(TrackedEntityAttribut
225200
String valueColumn = String.format("%s.%s", quote(attribute.getUid()), "value");
226201
DataType dataType = getColumnType(attribute.getValueType(), isSpatialSupport());
227202
String selectExpression = getColumnExpression(attribute.getValueType(), valueColumn);
228-
String dataFilterClause = getDataFilterClause(attribute);
229203
Skip skipIndex = skipIndex(attribute.getValueType(), attribute.hasOptionSet());
230204

231205
if (attribute.getValueType().isOrganisationUnit()) {
232-
columns.addAll(getColumnForOrgUnitAttribute(attribute, dataFilterClause));
206+
columns.addAll(getColumnForOrgUnitAttribute(attribute));
233207
}
234208

235209
columns.add(
@@ -248,19 +222,19 @@ protected List<AnalyticsTableColumn> getColumnForAttribute(TrackedEntityAttribut
248222
* Returns a list of columns based on the given attribute.
249223
*
250224
* @param attribute the {@link TrackedEntityAttribute}.
251-
* @param dataFilterClause the data filter clause.
252225
* @return a list of {@link AnalyticsTableColumn}.
253226
*/
254227
private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
255-
TrackedEntityAttribute attribute, String dataFilterClause) {
228+
TrackedEntityAttribute attribute) {
229+
Validate.isTrue(attribute.getValueType().isOrganisationUnit());
256230
List<AnalyticsTableColumn> columns = new ArrayList<>();
257231

258232
String fromClause =
259233
qualifyVariables("from ${organisationunit} ou where ou.uid = (select value");
260234

261235
if (isSpatialSupport()) {
262236
String selectExpression = "ou.geometry " + fromClause;
263-
String ouGeoSql = getSelectSubquery(attribute, selectExpression, dataFilterClause);
237+
String ouGeoSql = getSelectSubquery(attribute, selectExpression);
264238
columns.add(
265239
AnalyticsTableColumn.builder()
266240
.name((attribute.getUid() + OU_GEOMETRY_COL_SUFFIX))
@@ -272,7 +246,7 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
272246
}
273247

274248
String selectExpression = "ou.name " + fromClause;
275-
String ouNameSql = getSelectSubquery(attribute, selectExpression, dataFilterClause);
249+
String ouNameSql = getSelectSubquery(attribute, selectExpression);
276250

277251
columns.add(
278252
AnalyticsTableColumn.builder()
@@ -291,20 +265,17 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitAttribute(
291265
*
292266
* @param attribute the {@link TrackedEntityAttribute}.
293267
* @param selectExpression the select expression.
294-
* @param dataFilterClause the data filter clause.
295268
* @return a select statement.
296269
*/
297-
private String getSelectSubquery(
298-
TrackedEntityAttribute attribute, String selectExpression, String dataFilterClause) {
270+
private String getSelectSubquery(TrackedEntityAttribute attribute, String selectExpression) {
299271
return replaceQualify(
300272
"""
301273
(select ${selectExpression} from ${trackedentityattributevalue} \
302274
where trackedentityid=en.trackedentityid \
303-
and trackedentityattributeid=${attributeId}${dataFilterClause})\
275+
and trackedentityattributeid=${attributeId})\
304276
${closingParentheses} as ${attributeUid}""",
305277
Map.of(
306278
"selectExpression", selectExpression,
307-
"dataFilterClause", dataFilterClause,
308279
"attributeId", String.valueOf(attribute.getId()),
309280
"closingParentheses", getClosingParentheses(selectExpression),
310281
"attributeUid", quote(attribute.getUid())));

‎dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManager.java‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import static org.hisp.dhis.db.model.DataType.GEOMETRY;
4040
import static org.hisp.dhis.db.model.DataType.INTEGER;
4141
import static org.hisp.dhis.db.model.DataType.TEXT;
42+
import static org.hisp.dhis.system.util.MathUtils.NUMERIC_LENIENT_REGEXP;
4243
import static org.hisp.dhis.util.DateUtils.toLongDate;
4344
import static org.hisp.dhis.util.DateUtils.toMediumDate;
4445

@@ -502,7 +503,7 @@ private List<AnalyticsTableColumn> getColumnForDataElement(
502503
}
503504

504505
if (dataElement.getValueType().isOrganisationUnit()) {
505-
columns.addAll(getColumnForOrgUnitDataElement(dataElement, dataFilterClause));
506+
columns.addAll(getColumnForOrgUnitDataElement(dataElement));
506507
}
507508

508509
columns.add(
@@ -524,8 +525,7 @@ private List<AnalyticsTableColumn> getColumnForDataElement(
524525
* @param dataFilterClause the data filter SQL clause.
525526
* @return a list of {@link AnalyticsTableColumn}.
526527
*/
527-
private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(
528-
DataElement dataElement, String dataFilterClause) {
528+
private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(DataElement dataElement) {
529529
List<AnalyticsTableColumn> columns = new ArrayList<>();
530530

531531
String columnExpression =
@@ -535,20 +535,20 @@ private List<AnalyticsTableColumn> getColumnForOrgUnitDataElement(
535535

536536
if (isSpatialSupport()) {
537537
String fromType = "ou.geometry " + fromClause;
538-
String geoSql = getOrgUnitSelectExpression(dataElement, fromType, dataFilterClause);
538+
String geoExpression = getOrgUnitSelectExpression(dataElement, fromType);
539539

540540
columns.add(
541541
AnalyticsTableColumn.builder()
542542
.name((dataElement.getUid() + OU_GEOMETRY_COL_SUFFIX))
543543
.dimensionType(AnalyticsDimensionType.DYNAMIC)
544544
.dataType(GEOMETRY)
545-
.selectExpression(geoSql)
545+
.selectExpression(geoExpression)
546546
.indexType(IndexType.GIST)
547547
.build());
548548
}
549549

550550
String fromTypeSql = "ou.name " + fromClause;
551-
String ouNameSql = getOrgUnitSelectExpression(dataElement, fromTypeSql, dataFilterClause);
551+
String ouNameSql = getOrgUnitSelectExpression(dataElement, fromTypeSql);
552552

553553
columns.add(
554554
AnalyticsTableColumn.builder()
@@ -592,7 +592,7 @@ private List<AnalyticsTableColumn> getAttributeColumns(Program program) {
592592
private List<AnalyticsTableColumn> getColumnForAttributeWithLegendSet(
593593
TrackedEntityAttribute attribute) {
594594
String columnExpression = getColumnExpression(attribute.getValueType(), "value");
595-
String numericClause = getNumericClause();
595+
String numericClause = getNumericClause("value");
596596
String query =
597597
"""
598598
\s(select l.uid from ${maplegend} l \
@@ -630,18 +630,15 @@ private List<AnalyticsTableColumn> getColumnForAttributeWithLegendSet(
630630
*
631631
* @param dataElement the data element to create the select statement for.
632632
* @param selectExpression the select expression.
633-
* @param dataFilterClause the data filter clause.
634633
* @return a select expression.
635634
*/
636-
private String getOrgUnitSelectExpression(
637-
DataElement dataElement, String selectExpression, String dataFilterClause) {
635+
private String getOrgUnitSelectExpression(DataElement dataElement, String selectExpression) {
638636
Validate.isTrue(dataElement.getValueType().isOrganisationUnit());
639637
String prts = getClosingParentheses(selectExpression);
640638
return replaceQualify(
641-
"(select ${selectExpression} ${dataFilterClause})${closingParentheses} as ${uid}",
639+
"(select ${selectExpression})${closingParentheses} as ${uid}",
642640
Map.of(
643641
"selectExpression", selectExpression,
644-
"dataFilterClause", dataFilterClause,
645642
"closingParentheses", prts,
646643
"uid", quote(dataElement.getUid())));
647644
}
@@ -756,6 +753,16 @@ private List<Integer> getDataYears(
756753
return jdbcTemplate.queryForList(sql, Integer.class);
757754
}
758755

756+
/**
757+
* Returns a numeric regexp match expression for the given value.
758+
*
759+
* @param value the value.
760+
* @return a numeric regexp match expression.
761+
*/
762+
private final String getNumericClause(String value) {
763+
return " and " + sqlBuilder.regexpMatch(value, "'" + NUMERIC_LENIENT_REGEXP + "'");
764+
}
765+
759766
/**
760767
* Retrieve years for partition tables. Year will become a partition key. The default return value
761768
* is the list with the recent year.

‎dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerDorisTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ void verifyGetTableWithDataElements() {
169169
"case when json_unquote(json_extract(eventdatavalues, '$.%s.value')) regexp '^\\d{4}-\\d{2}-\\d{2}(\\s|T)?((\\d{2}:)(\\d{2}:)?(\\d{2}))?(|.(\\d{3})|.(\\d{3})Z)?$' then cast(json_unquote(json_extract(eventdatavalues, '$.%s.value')) as datetime) else null end as `%s`";
170170
String aliasE = "json_unquote(json_extract(eventdatavalues, '$.%s.value')) as `%s`";
171171
String aliasF =
172-
"(select ou.name from dhis2.public.`organisationunit` ou where ou.uid = json_unquote(json_extract(eventdatavalues, '$.%s.value')) ) as `%s`";
172+
"(select ou.name from dhis2.public.`organisationunit` ou where ou.uid = json_unquote(json_extract(eventdatavalues, '$.%s.value'))) as `%s`";
173173
String aliasG =
174174
"case when json_unquote(json_extract(eventdatavalues, '$.%s.value')) regexp '^(-?[0-9]+)(\\.[0-9]+)?$' then cast(json_unquote(json_extract(eventdatavalues, '$.%s.value')) as bigint) else null end as `%s`";
175175

‎dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/table/JdbcEventAnalyticsTableManagerTest.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -399,15 +399,13 @@ void verifyGetTableWithDataElements() {
399399
String aliasD5_geo =
400400
"(select ou.geometry from \"organisationunit\" ou where ou.uid = eventdatavalues #>> '{"
401401
+ d5.getUid()
402-
+ ", value}' "
403-
+ ") as \""
402+
+ ", value}') as \""
404403
+ d5.getUid()
405404
+ "\"";
406405
String aliasD5_name =
407406
"(select ou.name from \"organisationunit\" ou where ou.uid = eventdatavalues #>> '{"
408407
+ d5.getUid()
409-
+ ", value}' "
410-
+ ") as \""
408+
+ ", value}') as \""
411409
+ d5.getUid()
412410
+ "\"";
413411
AnalyticsTableUpdateParams params =
@@ -599,7 +597,7 @@ void verifyDataElementTypeOrgUnitFetchesOuNameWhenPopulatingEventAnalyticsTable(
599597
String.format(
600598
"""
601599
(select ou.name from "organisationunit" ou where ou.uid = \
602-
eventdatavalues #>> '{%s, value}' ) as %s""",
600+
eventdatavalues #>> '{%s, value}') as %s""",
603601
d5.getUid(), quote(d5.getUid()));
604602

605603
assertThat(sql.getValue(), containsString(ouUidQuery));

‎dhis-2/dhis-services/dhis-service-analytics/src/test/java/org/hisp/dhis/analytics/util/AnalyticsUtilsTest.java‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,6 @@
9393
import org.hisp.dhis.test.TestBase;
9494
import org.junit.jupiter.api.DisplayName;
9595
import org.junit.jupiter.api.Test;
96-
import org.springframework.jdbc.BadSqlGrammarException;
9796
import org.springframework.jdbc.UncategorizedSQLException;
9897

9998
/**
@@ -763,31 +762,14 @@ void whenUncategorizedSQLException_withTableNotExisting_thenThrowException() {
763762
() -> AnalyticsUtils.withExceptionHandling(supplier, false));
764763
}
765764

766-
@Test
767-
void whenBadSqlGrammarException_withMultipleQueries_thenReturnEmpty() {
768-
SQLException sqlException = new SQLException("syntax error");
769-
BadSqlGrammarException badSqlException =
770-
new BadSqlGrammarException("task", DUMMY_SQL, sqlException);
771-
Supplier<String> supplier =
772-
() -> {
773-
throw badSqlException;
774-
};
775-
776-
Optional<String> result = AnalyticsUtils.withExceptionHandling(supplier, true);
777-
778-
assertFalse(result.isPresent());
779-
}
780-
781765
@Test
782766
void whenQueryRuntimeException_thenRethrow() {
783-
// Arrange
784767
QueryRuntimeException queryException = new QueryRuntimeException("Test error");
785768
Supplier<String> supplier =
786769
() -> {
787770
throw queryException;
788771
};
789772

790-
// Act & Assert
791773
QueryRuntimeException thrown =
792774
assertThrows(
793775
QueryRuntimeException.class,

0 commit comments

Comments
 (0)