Allow zero period for TIMESTAMPADD to help make Tableau operable

[john-bodley]

Description

Tableau provides an option to choose a time period representing the current month,

which results in auto-generated SQL of the form,

 TIMESTAMPADD(MONTH,0,...)

which results in Druid throwing an error per here of the form,

Unknown exception (java.lang.IllegalArgumentException): zero period is not acceptable in PeriodGranularity!

Granted adding a zero period is atypical, but I was wondering why this should be an error (the check was introduced in #3644). For reference MySQL et al. support having a zero period,

mysql> SELECT TIMESTAMPADD(MONTH,0,'2020-10-24');
+------------------------------------+
| TIMESTAMPADD(MONTH,2,'2020-10-24') |
+------------------------------------+
| 2020-10-24                         | 
+------------------------------------+
1 row in set (0.00 sec)

Motivation

Please provide the following for the desired feature or change:

• The TIMESTAMPADD should allow for an zero interval to ensure applications like Tableau are functional.

[FrankChen021]

I made some investigation about this problem, and I think it should be a bug.

It's interesting that TIMESTAMPADD works fine with DAY, MINUTE when the 2nd parameter is zero, while MONTH, YEAR fail.

Druid's internal implementation converts calling of TIMESTAMPADD to TIME_SHIFT, and its parameters are also converted to a type of Period which is accepted by the latter function. And the code dealing with Period, which checks period should not be zero mainly for query granularity, are shared across the whole project.

While for the cases of DAY or MINUTE, Druid converts it to an integer value in milliseconds, which does not involve a Period object. This is also why the existing UT test case don't help to expose this bug.

@gianm what do you think ?

[gianm]

@gianm what do you think ?

I'm not sure if it should be considered a bug or a feature, but either way, it'd be good to allow this. It looks like the restriction to disallow a zero-size granularity (from #3644) was done to prevent infinite iterables. Maybe we could move the error to the iterable method instead of the constructor.

[FrankChen021]

@gianm

I think it's better to keep the check of zero period in the ctor because it helps prevent invalid user configurations during the deserialization phase of these configurations.

For this problem, maybe we need an internal ctor to bypass the check and delay the check to getIterable method

[gianm]

It's a little dangerous to have the check in the @JsonCreator constructor but to skip it in an internal constructor, because if that object ever "escapes" out to the wire, it will fail to deserialize.

Maybe instead we should special-case zero in TimestampShiftExprMacro. What do you think?

[FrankChen021]

Maybe instead we should special-case zero in TimestampShiftExprMacro. What do you think?

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.

BTW, I also notice that TIMESTAMPADD is converted to native query expression as

"virtualColumns": [
    {
      "type": "expression",
      "name": "v0",
      "expression": "timestamp_shift(\"__time\",concat('P', 1, 'M'),1,'UTC')",
      "outputType": "LONG"
    },

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 ?

[gianm]

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.

[FrankChen021]

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.

I see, thanks for the explanation.

I'll open a PR later both for the zero period problem and the slight improvement here.

[gianm]

I'll open a PR later both for the zero period problem and the slight improvement here.

That is great, please link them here.

[FrankChen021]

@gianm @john-bodley This issue can be closed since the related PR has been merged into master branch.

[jihoonson]

Fixed by #10550. Thanks @FrankChen021.