Skip to content

Make D2 dates related functions SqlBuilder aware (DHIS-16705) #19380

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.

Sign up for GitHub

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

Merged
luciano-fiandesio merged 5 commits into from
Dec 5, 2024
Merged

Make D2 dates related functions SqlBuilder aware (DHIS-16705) #19380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ea488d
Make D2 dates related functions SqlBuilder aware (DHIS-16705)
luciano-fiandesio Dec 4, 2024
0b49641
Merge branch 'master' into DHIS-16705_D2_DATE_EXPRESSIONS_SQLBUILDER
luciano-fiandesio Dec 4, 2024
ebb87e3
Fix unit tests
luciano-fiandesio Dec 4, 2024
e4cd6a5
Fix Sonar comments
luciano-fiandesio Dec 4, 2024
053eab9
Fix Unit + Integration tests
luciano-fiandesio Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...dhis-service-core/src/main/java/org/hisp/dhis/program/DefaultProgramIndicatorService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public DefaultProgramIndicatorService(
this.analyticsSqlCache = cacheProvider.createAnalyticsSqlCache();
this.sqlBuilder = sqlBuilder;

this.programIndicatorItems = new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap();
this.programIndicatorItems = new ExpressionMapBuilder().getExpressionItemMap();
Copy link
Contributor

@gnespolino gnespolino Dec 4, 2024

Choose a reason for hiding this comment

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

I’m curious — was there a particular reason we opted not to use dependency injection in this case?

Copy link
Contributor Author

@luciano-fiandesio luciano-fiandesio Dec 4, 2024

Choose a reason for hiding this comment

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

Bit of a long story, in reality this new ExpressionMapBuilder is no longer needed, because @jimgrace showed me a better/simpler way to access the SqlBuilder. But since it was there, I figured that we can keep the list of expressions encapsulated into its own class, and yes, this could use DoI. Not sure, shall I just get rid of ExpressionMapBuilder altogether?

Copy link
Member

@jimgrace jimgrace Dec 4, 2024

Choose a reason for hiding this comment

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

I think it's OK. In DefaultExpressionService these maps are still generated in the service (for all the expression types that are not program indicators), so this is not consistent with that. But all this ANLR code will be ripped out when we transition to Jan B's new parser. (Did you know about that?) I'm OK with keeping it as a separate class or putting it back where it was.

}

// -------------------------------------------------------------------------
Expand Down
43 changes: 21 additions & 22 deletions ...-services/dhis-service-core/src/main/java/org/hisp/dhis/program/ExpressionMapBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@

import com.google.common.collect.ImmutableMap;
import lombok.Getter;
import org.hisp.dhis.db.sql.SqlBuilder;
import org.hisp.dhis.parser.expression.ExpressionItem;
import org.hisp.dhis.parser.expression.function.RepeatableProgramStageOffset;
import org.hisp.dhis.parser.expression.function.VectorAvg;
Expand Down Expand Up @@ -72,7 +71,7 @@ public class ExpressionMapBuilder {

private final ImmutableMap<Integer, ExpressionItem> expressionItemMap;

public ExpressionMapBuilder(SqlBuilder sqlBuilder) {
public ExpressionMapBuilder() {
expressionItemMap =
ImmutableMap.<Integer, ExpressionItem>builder()

Expand All @@ -82,22 +81,22 @@ public ExpressionMapBuilder(SqlBuilder sqlBuilder) {

// Program functions

.put(D2_CONDITION, new D2Condition().withSqlBuilder(sqlBuilder))
.put(D2_COUNT, new D2Count().withSqlBuilder(sqlBuilder))
.put(D2_COUNT_IF_CONDITION, new D2CountIfCondition().withSqlBuilder(sqlBuilder))
.put(D2_COUNT_IF_VALUE, new D2CountIfValue().withSqlBuilder(sqlBuilder))
.put(D2_DAYS_BETWEEN, new D2DaysBetween().withSqlBuilder(sqlBuilder))
.put(D2_HAS_VALUE, new D2HasValue().withSqlBuilder(sqlBuilder))
.put(D2_MAX_VALUE, new D2MaxValue().withSqlBuilder(sqlBuilder))
.put(D2_MINUTES_BETWEEN, new D2MinutesBetween().withSqlBuilder(sqlBuilder))
.put(D2_MIN_VALUE, new D2MinValue().withSqlBuilder(sqlBuilder))
.put(D2_MONTHS_BETWEEN, new D2MonthsBetween().withSqlBuilder(sqlBuilder))
.put(D2_OIZP, new D2Oizp().withSqlBuilder(sqlBuilder))
.put(D2_RELATIONSHIP_COUNT, new D2RelationshipCount().withSqlBuilder(sqlBuilder))
.put(D2_WEEKS_BETWEEN, new D2WeeksBetween().withSqlBuilder(sqlBuilder))
.put(D2_YEARS_BETWEEN, new D2YearsBetween().withSqlBuilder(sqlBuilder))
.put(D2_ZING, new D2Zing().withSqlBuilder(sqlBuilder))
.put(D2_ZPVC, new D2Zpvc().withSqlBuilder(sqlBuilder))
.put(D2_CONDITION, new D2Condition())
.put(D2_COUNT, new D2Count())
.put(D2_COUNT_IF_CONDITION, new D2CountIfCondition())
.put(D2_COUNT_IF_VALUE, new D2CountIfValue())
.put(D2_DAYS_BETWEEN, new D2DaysBetween())
.put(D2_HAS_VALUE, new D2HasValue())
.put(D2_MAX_VALUE, new D2MaxValue())
.put(D2_MINUTES_BETWEEN, new D2MinutesBetween())
.put(D2_MIN_VALUE, new D2MinValue())
.put(D2_MONTHS_BETWEEN, new D2MonthsBetween())
.put(D2_OIZP, new D2Oizp())
.put(D2_RELATIONSHIP_COUNT, new D2RelationshipCount())
.put(D2_WEEKS_BETWEEN, new D2WeeksBetween())
.put(D2_YEARS_BETWEEN, new D2YearsBetween())
.put(D2_ZING, new D2Zing())
.put(D2_ZPVC, new D2Zpvc())

// Program functions for custom aggregation

Expand All @@ -111,13 +110,13 @@ public ExpressionMapBuilder(SqlBuilder sqlBuilder) {

// Data items

.put(HASH_BRACE, new ProgramItemStageElement().withSqlBuilder(sqlBuilder))
.put(A_BRACE, new ProgramItemAttribute().withSqlBuilder(sqlBuilder))
.put(PS_EVENTDATE, new ProgramItemPsEventdate().withSqlBuilder(sqlBuilder))
.put(HASH_BRACE, new ProgramItemStageElement())
.put(A_BRACE, new ProgramItemAttribute())
.put(PS_EVENTDATE, new ProgramItemPsEventdate())

// Program variables

.put(V_BRACE, new ProgramVariableItem().withSqlBuilder(sqlBuilder))
.put(V_BRACE, new ProgramVariableItem())

// . functions
.put(STAGE_OFFSET, new RepeatableProgramStageOffset())
Expand Down
10 changes: 1 addition & 9 deletions ...services/dhis-service-core/src/main/java/org/hisp/dhis/program/ProgramExpressionItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import org.hisp.dhis.analytics.DataType;
import org.hisp.dhis.antlr.ParserExceptionWithoutContext;
import org.hisp.dhis.common.ValueType;
import org.hisp.dhis.db.sql.SqlBuilder;
import org.hisp.dhis.parser.expression.CommonExpressionVisitor;
import org.hisp.dhis.parser.expression.ExpressionItem;
import org.hisp.dhis.program.dataitem.ProgramItemAttribute;
Expand All @@ -58,8 +57,6 @@
*/
public abstract class ProgramExpressionItem implements ExpressionItem {

private SqlBuilder sqlBuilder;

@Override
public final Object getExpressionInfo(ExprContext ctx, CommonExpressionVisitor visitor) {
throw new ParserExceptionWithoutContext(
Expand Down Expand Up @@ -124,11 +121,6 @@ protected String replaceNullSqlValues(
if (dataType == NUMERIC || dataType == BOOLEAN) {
dataType = visitor.getParams().getDataType() == BOOLEAN ? BOOLEAN : NUMERIC;
}
return replaceSqlNull(sqlBuilder.cast(column, dataType), dataType);
}

protected ExpressionItem withSqlBuilder(SqlBuilder sqlBuilder) {
this.sqlBuilder = sqlBuilder;
return this;
return replaceSqlNull(visitor.getSqlBuilder().cast(column, dataType), dataType);
}
}
8 changes: 6 additions & 2 deletions ...ervices/dhis-service-core/src/main/java/org/hisp/dhis/program/function/D2DaysBetween.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,18 @@
*/
package org.hisp.dhis.program.function;

import org.hisp.dhis.db.sql.SqlBuilder;
import org.hisp.dhis.parser.expression.CommonExpressionVisitor;

/**
* Program indicator function: d2 days between
*
* @author Jim Grace
*/
public class D2DaysBetween extends ProgramBetweenFunction {
@Override
public Object getSqlBetweenDates(String startDate, String endDate) {
return "(cast(" + endDate + " as date) - cast(" + startDate + " as date))";
public Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor) {
return visitor.getSqlBuilder().dateDifference(startDate, endDate, SqlBuilder.DateUnit.DAYS);
}
}
13 changes: 7 additions & 6 deletions ...ices/dhis-service-core/src/main/java/org/hisp/dhis/program/function/D2MinutesBetween.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,19 @@
*/
package org.hisp.dhis.program.function;

import static org.hisp.dhis.db.sql.SqlBuilder.DateUnit.MINUTES;

import org.hisp.dhis.parser.expression.CommonExpressionVisitor;

/**
* Program indicator function: d2 minutes between
*
* @author Jim Grace
*/
public class D2MinutesBetween extends ProgramBetweenFunction {
@Override
public Object getSqlBetweenDates(String startDate, String endDate) {
return "(extract(epoch from (cast("
+ endDate
+ " as timestamp) - cast("
+ startDate
+ " as timestamp))) / 60)";
public Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor) {
return visitor.getSqlBuilder().dateDifference(startDate, endDate, MINUTES);
}
}
18 changes: 7 additions & 11 deletions ...vices/dhis-service-core/src/main/java/org/hisp/dhis/program/function/D2MonthsBetween.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,19 @@
*/
package org.hisp.dhis.program.function;

import static org.hisp.dhis.db.sql.SqlBuilder.DateUnit.MONTHS;

import org.hisp.dhis.parser.expression.CommonExpressionVisitor;

/**
* Program indicator function: d2 months between
*
* @author Jim Grace
*/
public class D2MonthsBetween extends ProgramBetweenFunction {
@Override
public Object getSqlBetweenDates(String startDate, String endDate) {
return "((date_part('year',age(cast("
+ endDate
+ " as date), cast("
+ startDate
+ " as date)))) * 12 + "
+ "date_part('month',age(cast("
+ endDate
+ " as date), cast("
+ startDate
+ " as date))))";
public Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor) {
return visitor.getSqlBuilder().dateDifference(startDate, endDate, MONTHS);
}
}
9 changes: 7 additions & 2 deletions ...rvices/dhis-service-core/src/main/java/org/hisp/dhis/program/function/D2WeeksBetween.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,19 @@
*/
package org.hisp.dhis.program.function;

import static org.hisp.dhis.db.sql.SqlBuilder.DateUnit.WEEKS;

import org.hisp.dhis.parser.expression.CommonExpressionVisitor;

/**
* Program indicator function: d2 weeks between
*
* @author Jim Grace
*/
public class D2WeeksBetween extends ProgramBetweenFunction {
@Override
public Object getSqlBetweenDates(String startDate, String endDate) {
return "((cast(" + endDate + " as date) - cast(" + startDate + " as date))/7)";
public Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor) {
return visitor.getSqlBuilder().dateDifference(startDate, endDate, WEEKS);
}
}
13 changes: 7 additions & 6 deletions ...rvices/dhis-service-core/src/main/java/org/hisp/dhis/program/function/D2YearsBetween.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,19 @@
*/
package org.hisp.dhis.program.function;

import static org.hisp.dhis.db.sql.SqlBuilder.DateUnit.YEARS;

import org.hisp.dhis.parser.expression.CommonExpressionVisitor;

/**
* Program indicator function: d2 years between
*
* @author Jim Grace
*/
public class D2YearsBetween extends ProgramBetweenFunction {
@Override
public Object getSqlBetweenDates(String startDate, String endDate) {
return "(date_part('year',age(cast("
+ endDate
+ " as date), cast("
+ startDate
+ " as date))))";
public Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor) {
return visitor.getSqlBuilder().dateDifference(startDate, endDate, YEARS);
}
}
5 changes: 3 additions & 2 deletions ...his-service-core/src/main/java/org/hisp/dhis/program/function/ProgramBetweenFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public final Object getSql(ExprContext ctx, CommonExpressionVisitor visitor) {
String startDate = castString(visitor.visitAllowingNulls(ctx.expr(0)));
String endDate = castString(visitor.visitAllowingNulls(ctx.expr(1)));

return getSqlBetweenDates(startDate, endDate);
return getSqlBetweenDates(startDate, endDate, visitor);
}

/**
Expand All @@ -64,5 +64,6 @@ public final Object getSql(ExprContext ctx, CommonExpressionVisitor visitor) {
* @param endDate ending date
* @return the SQL to compare the dates based on the time/date unit
*/
public abstract Object getSqlBetweenDates(String startDate, String endDate);
public abstract Object getSqlBetweenDates(
String startDate, String endDate, CommonExpressionVisitor visitor);
}
4 changes: 2 additions & 2 deletions ...is-service-core/src/test/java/org/hisp/dhis/program/ProgramSqlGeneratorFunctionsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,7 @@ void testWeeksBetween() {
sql,
is(
"((cast(case when ax.\"ps\" = 'ProgrmStagA' then \"DataElmentD\" else null end as date) - "
+ "cast(case when ax.\"ps\" = 'ProgrmStagA' then \"DataElmentC\" else null end as date))/7)"));
+ "cast(case when ax.\"ps\" = 'ProgrmStagA' then \"DataElmentC\" else null end as date)) / 7)"));
}

@Test
Expand Down Expand Up @@ -800,7 +800,7 @@ private Object test(
.programIndicatorService(programIndicatorService)
.programStageService(programStageService)
.i18nSupplier(() -> new I18n(null, null))
.itemMap(new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap())
.itemMap(new ExpressionMapBuilder().getExpressionItemMap())
.itemMethod(itemMethod)
.params(params)
.progParams(progParams)
Expand Down
2 changes: 1 addition & 1 deletion ...s/dhis-service-core/src/test/java/org/hisp/dhis/program/ProgramSqlGeneratorItemsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ private Object test(
.programStageService(programStageService)
.i18nSupplier(() -> new I18n(null, null))
.constantMap(constantMap)
.itemMap(new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap())
.itemMap(new ExpressionMapBuilder().getExpressionItemMap())
.itemMethod(itemMethod)
.params(params)
.progParams(progParams)
Expand Down
2 changes: 1 addition & 1 deletion ...is-service-core/src/test/java/org/hisp/dhis/program/ProgramSqlGeneratorVariablesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ private Object test(
.programIndicatorService(programIndicatorService)
.programStageService(programStageService)
.i18nSupplier(() -> new I18n(null, null))
.itemMap(new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap())
.itemMap(new ExpressionMapBuilder().getExpressionItemMap())
.itemMethod(ITEM_GET_SQL)
.params(params)
.progParams(progParams)
Expand Down
Loading
Loading