-
Notifications
You must be signed in to change notification settings - Fork 13k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-13112][table-planner-blink] Support LocalZonedTimestampType in blink #9036
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 1f7eaf1 (Tue Aug 06 15:40:05 UTC 2019) 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:
|
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/TableConfig.java
Show resolved
Hide resolved
...-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/api/TableConfig.scala
Outdated
Show resolved
Hide resolved
...ink-table-runtime-blink/src/main/java/org/apache/flink/table/api/ExecutionConfigOptions.java
Outdated
Show resolved
Hide resolved
de3f751
to
5685400
Compare
*/ | ||
private TimeZone timeZone = TimeZone.getTimeZone("UTC"); | ||
private ZoneId zoneId = ZoneId.systemDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this break the old behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never be used in release-1.8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another concern is it may cause unstable tests if systemDefault
is based on some system level settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, tests should pass always, if the test not set zone id, the assert should not expect some time zone dependents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any bad experience with time zone set to "UTC" by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TimestampWithLocalTimeZone will be same to TimestampWithoutZone if we set to "UTC" by default.
I take a look to hive, spark, they all are default system time zone by default. Oracle maybe same.
I think user want to default system time zone when use TimestampWithLocalTimeZone at the most of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change to getLocalTimeZone
84e511a
to
a684ed1
Compare
1abd1e5
to
cf2afbf
Compare
*/ | ||
private TimeZone timeZone = TimeZone.getTimeZone("UTC"); | ||
private ZoneId localZoneId = ZoneId.systemDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @twalthr, we changed TimeZone
to LocalZoneId
to better fit with TIMESTAMP WITH LOCAL TIME ZONE
LGTM, @twalthr , do you have further comments? |
/** | ||
* Adds a reusable Time ZoneId to the member area of the generated class. | ||
*/ | ||
def addReusableTimeZoneID(): String = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Never used. Remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think it should be used, we now use old time zone, we should use zone id instead.
9310b11
to
1f7eaf1
Compare
travis passed: https://travis-ci.org/JingsongLi/flink/builds/557282177 |
Merging this.. |
What is the purpose of the change
Now we just support TimestampType, it is without time zone.
We need support LocalZonedTimestampType, and define time zone in config.
Now, for the functions of
TIMESTAMP_WITH_LOCAL_TIME_ZONE
:1.cast: only support cast to and cast from
Char/VarChar/Date/Time/Timestamp
. (Limitation of calcite)2.Support calcite functions: EXTRACT, FLOOR, CEIL, UNIX_TIMESTAMP, DATE_FORMAT.
3.Not support functions of blink extension.
Verifying this change
ut
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation