gianm commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-718888711
> Hi @gianm , since this macro relies on `PeriodGranularity` which holds
period and timezone extracted from given parameters, I think we could change
the code here to extract period and timezone respectively to avoid construct an
object of `PeriodGranularity`.
I think that sounds good.
> I don't understand why a `concat` function call is needed to construct a
period string instead of using a string "P1M" directly, is there something that
I missed ?
Looks like the relevant code is in TimeArithmeticOperatorConversion:
```
if (rightRexNode.getType().getFamily() ==
SqlTypeFamily.INTERVAL_YEAR_MONTH) {
// timestamp_expr { + | - } <interval_expr> (year-month interval)
// Period is a value in months.
return DruidExpression.fromExpression(
DruidExpression.functionCall(
"timestamp_shift",
leftExpr,
rightExpr.map(
simpleExtraction -> null,
expression -> StringUtils.format("concat('P', %s, 'M')",
expression)
),
DruidExpression.fromExpression(DruidExpression.numberLiteral(direction > 0 ? 1
: -1)),
DruidExpression.fromExpression(DruidExpression.stringLiteral(plannerContext.getTimeZone().getID()))
)
);
}
```
It would be better to do the concatenation right there in the case where
`expression` is a literal. I think we could do it by looking at rightRexNode
directly, using something like `rightRexNode.isA(SqlKind.LITERAL)` and then
`RexLiteral.value(rightRexNode)`.
Btw, if it's _not_ a literal we still need to do the `concat` stuff.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
