Make D2 dates related functions SqlBuilder aware (DHIS-16705) #19380
Conversation
luciano-fiandesio
requested review from
gnespolino,
larshelge and
maikelarabori
maikelarabori
left a comment
maikelarabori
left a comment
There was a problem hiding this comment.
Looks good 👍
Minor suggestion added.
Thx!
| */ | ||
| String dropCatalogIfExists(); | ||
|
|
||
| enum DatePart { |
There was a problem hiding this comment.
Call it DateUnit, maybe?
gnespolino
left a comment
gnespolino
left a comment
There was a problem hiding this comment.
Good job, just a minor comment
| this.sqlBuilder = sqlBuilder; | ||
|
|
||
| this.programIndicatorItems = new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap(); | ||
| this.programIndicatorItems = new ExpressionMapBuilder().getExpressionItemMap(); |
There was a problem hiding this comment.
I’m curious — was there a particular reason we opted not to use dependency injection in this case?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
jimgrace
left a comment
jimgrace
left a comment
There was a problem hiding this comment.
Some code will also need to change at some point under org.hisp.dhis.parser.expression if Doris follows MySQL syntax rather than Postgres. Classes I've noticed in a quick review that will need changing are:
FunctionContains
FunctionContainsItems
OperatorMathPower
| this.sqlBuilder = sqlBuilder; | ||
|
|
||
| this.programIndicatorItems = new ExpressionMapBuilder(sqlBuilder).getExpressionItemMap(); | ||
| this.programIndicatorItems = new ExpressionMapBuilder().getExpressionItemMap(); |
There was a problem hiding this comment.
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.
luciano-fiandesio
deleted the
DHIS-16705_D2_DATE_EXPRESSIONS_SQLBUILDER
branch
Summary
This PR enhances the integration of the
SqlBuilderwith the Antlr parser context and updates D2 functions to utilize the newly introduceddateDifferencefunction.Changes
dateDifferencefunction within theSqlBuilderfor improved date manipulation.D2*Betweenfunctions to incorporate theSqlBuilderfor uniform processing.