[FLINK-22861][table-planner] Fix return value deduction of TIMESTAMPADD function #16511
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 6ae6d51 (Thu Sep 23 18:08:49 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
There was a problem hiding this comment.
Thanks @tsreaper for the contribution, I left several comments.
And I found some CI tests failure, you may need to have a look
| val ddl = | ||
| s""" | ||
| |CREATE TABLE MyTable ( | ||
| | a TIMESTAMP WITH LOCAL TIME ZONE |
There was a problem hiding this comment.
nit: we can use a TIMESTAMP_LTZ
| case MILLISECOND: | ||
| type = | ||
| typeFactory.createSqlType( | ||
| SqlTypeName.TIMESTAMP, | ||
| Math.max(MILLISECOND_PRECISION, datetimeType.getPrecision())); | ||
| break; | ||
| case MICROSECOND: | ||
| type = | ||
| typeFactory.createSqlType( | ||
| SqlTypeName.TIMESTAMP, | ||
| Math.max(MICROSECOND_PRECISION, datetimeType.getPrecision())); | ||
| break; |
There was a problem hiding this comment.
The timestampAdd function supports the case that a timestamp_ltz argument add interval MILLISECOND or MILLISECOND , and in this case the deduced type should be timestamp_ltz?
There was a problem hiding this comment.
Nice catch. However this change cannot be tested now as SQL code generation does not support fractions of seconds.
| * | ||
| * <p>Returns modified datetime. | ||
| * | ||
| * <p>This class was copied over from Calcite to fix the return type deduction issue on timestamp |
There was a problem hiding this comment.
Could we create a calcite issue and link it here? we can remove this once calcite community fix it.
| TimeUnit timeUnit, | ||
| RelDataType intervalType, | ||
| RelDataType datetimeType) { | ||
| // CHANGED: this method is changed to deduce return type on timestamp with local time zone |
There was a problem hiding this comment.
We can use this template as we discussed with kurt offline
// BEGIN FLINK MODIFICATION
// Reason: …
// (Optional) Should be removed after CALCITE-XXXX is fixed
…
// END FLINK MODIFICATION
|
Thanks @leonardBang for the review. The CI failure is caused by the changes in the result type of |
| "2016-06-15 00:00:01.000000") | ||
| "2016-06-15 00:00:01") |
There was a problem hiding this comment.
I'm not sure this makes sense, I remember the default precision of TIMESTAMP/TIMESTAMP_LTZ is 6 in Flink
There was a problem hiding this comment.
OK, so for time / timestamp / timestamp_ltz + hour / minute / second we should leave the original precision unchanged, for other data types + hour / minute / second we should use the default timestamp precision.
|
@tsreaper Can you create a PR for 1.13? |
…DD function This closes apache#16511
What is the purpose of the change
Currently
TIMESTAMPADD(MINUTE, 10, CURRENT_TIMESTAMP)will be deduced astimestamptype. However this should betimestamp with local time zonetype. With this incorrect type, code generation will refuse to work.This PR fixes this issue by copying and modifying the corresponding classes from Calcite.
Brief change log
Verifying this change
This change added tests and can be verified by running the added tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation