chore: propagate SqlBuilder to Antlr Expression visitor [DHIS-16705] #19362
Conversation
luciano-fiandesio
requested review from
larshelge,
jimgrace and
maikelarabori
and removed request for
larshelge
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Used to collect the string replacements to build a description. This may contain names of | ||
| * metadata from the database. | ||
| */ | ||
| @Builder.Default private Map<String, String> itemDescriptions = new HashMap<>(); | ||
| private Map<String, String> itemDescriptions; |
Check notice
Code scanning / CodeQL
Exposing internal representation Note
| // Custom constructor | ||
| // ------------------------------------------------------------------------- | ||
|
|
||
| @Builder(toBuilder = true) |
Check notice
Code scanning / CodeQL
Use of default toString() Note
| @@ -71,4 +87,25 @@ public DefaultOutboundMessageBatchService defaultOutboundMessageBatchService( | |||
| public ResourceBundleManager resourceBundleManager() { | |||
| return new DefaultResourceBundleManager(); | |||
| } | |||
|
|
|||
| @Bean("org.hisp.dhis.expression.ExpressionService") | |||
There was a problem hiding this comment.
I am wondering if this definition is actually unnecessary. Since SqlBuilder is now a bean, you can now inject it into DefaultExpressionService as usual with (Lombok) constructor injection.
There was a problem hiding this comment.
Yes, good catch, removed.
| @@ -146,6 +147,10 @@ public class DefaultProgramIndicatorService implements ProgramIndicatorService { | |||
|
|
|||
| private final Cache<String> analyticsSqlCache; | |||
|
|
|||
| private final SqlBuilder sqlBuilder; | |||
|
|
|||
| public static ImmutableMap<Integer, ExpressionItem> PROGRAM_INDICATOR_ITEMS; | |||
There was a problem hiding this comment.
It is a little bit of an anti-pattern to initialize static variables in constructors in singletons. Since this is a Spring singleton bean, the constructor is guaranteed to be invoked once. It is more natural to make it consistent with other dependencies, and instead use private final:
private final Map<Integer, ExpressionItem> programIndicatorItems;
There was a problem hiding this comment.
Sorry, my bad, this part is actually no longer like this. The problem is that this PROGRAM_INDICATOR_ITEMS variable is only used by Unit Tests and not by the runtime. But it can no longer be static, since we need to pass the SqlBuilder. Fixing the unit tests, this will change too.
There was a problem hiding this comment.
Understand. Yes good to get it out of the implementation if it is only used in tests.
| @@ -173,57 +180,63 @@ public DefaultProgramIndicatorService( | |||
| this.dimensionService = dimensionService; | |||
| this.i18nManager = i18nManager; | |||
| this.analyticsSqlCache = cacheProvider.createAnalyticsSqlCache(); | |||
| this.sqlBuilder = sqlBuilder; | |||
|
|
|||
| PROGRAM_INDICATOR_ITEMS = buildExpressionMap(); | |||
There was a problem hiding this comment.
this.programIndicatorItems = buildExpressionMap();
There was a problem hiding this comment.
Yes, see comment above. The PROGRAM_INDICATOR_ITEMS variable no longer exists;
| @Override | ||
| public String cast(String column, DataType dataType) { | ||
| return switch (dataType) { | ||
| case NUMERIC -> "toDecimal64(" + column + ", 8)"; // 8 decimal places precision |
There was a problem hiding this comment.
Use String.format over String concatenation.
| @Override | ||
| public String cast(String column, DataType dataType) { | ||
| return switch (dataType) { | ||
| case NUMERIC -> "CAST(" + column + " AS DECIMAL)"; |
There was a problem hiding this comment.
Use String.format over String concatenation. Use lower-case SQL by default.
dhis-2/dhis-support/dhis-support-sql/src/main/java/org/hisp/dhis/db/sql/PostgreSqlBuilder.java
Show resolved
Hide resolved
| @@ -287,6 +288,16 @@ public interface SqlBuilder { | |||
| */ | |||
| String jsonExtractNested(String json, String... expression); | |||
|
|
|||
| /** | |||
| * Generates a database-specific SQL casting expression for the given column or expression. | |||
There was a problem hiding this comment.
We don't have to say "database-specific" in the Javadoc, as everything in SqlBuilder is database specific.
|
Looks good! Great to see these incremental improvements. |
|
larshelge
changed the title
Description
This PR attempts at propagating the
SqlBuilderinto the Antlr parser, so that parser can generate SQL expression compatible with the underlying analytics database.Changes
Configure theExpressionServiceSpring Bean into aServiceConfig, so that theSqlBuildercan be injected explicitly.DefaultProgramIndicatorServiceso that the expression map is now built externally (seeExpressionMapBuilder). This allow to use the newwithSqlBuildermethod on the expressions in the map and pass theSqlBuilderto each expression.withSqlBuildermethod toProgramExpressionItem.CommonExpressionVisitorto make possible to initialize the existingDefaultStatementBuilderwithSqlBuildercastfunction toSqlBuilderto hanlde SQL casting