You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/10/25 00:40:18 UTC
[GitHub] [druid] john-bodley opened a new issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
john-bodley opened a new issue #10530:
URL: https://github.com/apache/druid/issues/10530
### Description
Tableau provides an option to choose a time period representing the current month,
<img width="565" alt="Screen Shot 2020-10-24 at 5 25 41 PM" src="https://user-images.githubusercontent.com/4567245/97096253-46590080-161e-11eb-93c0-055438950486.png">
which results in auto-generated SQL of the form,
```sql
TIMESTAMPADD(MONTH,0,...)
```
which results in Druid throwing an error per [here](https://github.com/apache/druid/blob/c6caae9a24ed0363380d2a6b1b12f5f8af9bd425/core/src/main/java/org/apache/druid/java/util/common/granularity/PeriodGranularity.java#L63) 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, i.e., I don't see anything in the code which would present the operation from succeeding had the check not been present. 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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-718375579
> 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 ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
jihoonson commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-741527863
Fixed by https://github.com/apache/druid/pull/10550. Thanks @FrankChen021.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-719132187
>
> 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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 edited a comment on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 edited a comment on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-716472662
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](https://github.com/apache/druid/blob/f3a2903218573f5d336b082b1c9b8a60a19e8c54/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java#L11393) don't help to expose this bug.
@gianm what do you think ?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
gianm commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-716691106
> @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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-717153394
@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
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
gianm commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-717308915
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?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-716472662
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 don't involve a `Period` object. This is also why the existing [UT test case](https://github.com/apache/druid/blob/f3a2903218573f5d336b082b1c9b8a60a19e8c54/sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java#L11393) don't help to expose this bug.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson closed issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
jihoonson closed issue #10530:
URL: https://github.com/apache/druid/issues/10530
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] FrankChen021 commented on issue #10530: Allow zero period for TIMESTAMPADD to help make Tableau operable
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on issue #10530:
URL: https://github.com/apache/druid/issues/10530#issuecomment-733595479
@gianm @john-bodley This issue can be closed since the related PR has been merged into master branch.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org