You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/07/26 02:52:50 UTC
[GitHub] [incubator-doris] WingsGo opened a new pull request #4187: [TimeZone]Set UTC to defult timezone
WingsGo opened a new pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187
## Proposed changes
TimeUtils cannot identify timezone "UTC", but in some OS system, we will get default timezone value `UTC`.It will cause some DDL failed with msg "Unknown or incorrect time zone: UTC", and some UT will also failed cause by this.So This PR is to set UTC mapping to default_timezone `UTC+08:00` to avoid situation like this. Relate ISSUE: #3950
## Types of changes
What types of changes does your code introduce to Doris?
_Put an `x` in the boxes that apply_
- [ ] Bugfix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation Update (if none of the other choices apply)
## Checklist
_Put an `x` in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code._
- [x] I have create an issue on [Doris's issues](https://github.com/apache/incubator-doris/issues), and have described the bug/feature there in detail
- [x] Commit messages in my PR start with the related issues ID, like "#4071 Add pull request template to doris project"
- [x] Compiling and unit tests pass locally with my changes
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] If this change need a document change, I have updated the document
- [x] Any dependent changes have been merged
## Further comments
If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] WingsGo commented on pull request #4187: [TimeZone]Get default timezone from session variable && add UTC timezone alias to UTC +00:00
Posted by GitBox <gi...@apache.org>.
WingsGo commented on pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#issuecomment-668023065
Hi, @morningman, I unify the timezone storage value from ZoneId.getId() in order to get a unify timezone string, and I add simple time_zone_test in BE to ensure they are supported, the purpose of the change is to support timezone format like "+08:00" or "counrty/city" or some specified timezone alias, such as "CST", "PRC" and "UTC" etc, I think it will cover most of situations, If there is some other case, we only need add alias in FE and check it worked in BE.
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] WingsGo commented on pull request #4187: [TimeZone]Get default timezone from session variable && add UTC timezone alias to UTC +00:00
Posted by GitBox <gi...@apache.org>.
WingsGo commented on pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#issuecomment-673870345
Hi, @morningman Can you take some time to review it, Thx~~
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #4187: [TimeZone]Get default timezone from session variable && add UTC timezone alias to UTC +00:00
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#discussion_r464085625
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -32,6 +32,7 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.apache.parquet.Strings;
Review comment:
Wrong import
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] WingsGo commented on a change in pull request #4187: [TimeZone]Set UTC to defult timezone
Posted by GitBox <gi...@apache.org>.
WingsGo commented on a change in pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#discussion_r460532621
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -256,7 +256,7 @@ public static String checkTimeZoneValidAndStandardize(String value) throws DdlEx
Matcher matcher = TIMEZONE_OFFSET_FORMAT_REG.matcher(value);
// it supports offset and region timezone type, "CST" use here is compatibility purposes.
boolean match = matcher.matches();
- if (!value.contains("/") && !value.equals("CST") && !match) {
+ if (!value.contains("/") && !value.equals("CST") && !value.equals("UTC") && !match) {
Review comment:
The root case of the problem such as `PRC` Timezone is that TimeUtils check is only check by regex we given, maybe we should also check the timezone exists in `ZoneId.getAvailableZoneIds`?
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #4187: [TimeZone]Set UTC to defult timezone
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#discussion_r460519840
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -256,7 +256,7 @@ public static String checkTimeZoneValidAndStandardize(String value) throws DdlEx
Matcher matcher = TIMEZONE_OFFSET_FORMAT_REG.matcher(value);
// it supports offset and region timezone type, "CST" use here is compatibility purposes.
boolean match = matcher.matches();
- if (!value.contains("/") && !value.equals("CST") && !match) {
+ if (!value.contains("/") && !value.equals("CST") && !value.equals("UTC") && !match) {
Review comment:
With more and more time zone added, I think it is better to refactor this judgment to make it easy to maintain.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -55,7 +55,7 @@
// set CST to +08:00 instead of America/Chicago
public static final ImmutableMap<String, String> timeZoneAliasMap = ImmutableMap.of(
- "CST", DEFAULT_TIME_ZONE, "PRC", DEFAULT_TIME_ZONE);
+ "CST", DEFAULT_TIME_ZONE, "PRC", DEFAULT_TIME_ZONE, "UTC", DEFAULT_TIME_ZONE);
Review comment:
I think `DEFAULT_TIME_ZONE` means `UTC+8` ? Not equals to `UTC`?
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] WingsGo commented on pull request #4187: [TimeZone]Get default timezone from session variable && add UTC timezone alias to UTC +00:00
Posted by GitBox <gi...@apache.org>.
WingsGo commented on pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#issuecomment-667617412
Hi, @morningman @HangyuanLiu I rename `TimeUtils.getTimeZone` to `TimeUtils.getSessionTimeZone` and use the function to get default timezone instead get default time zone from system && add alias `UTC` to `Africa/Abidjan`.We need to support set system time zone in fe.conf and add releated doc, maybe in next PR.
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] WingsGo commented on a change in pull request #4187: [TimeZone]Get default timezone from session variable && add UTC timezone alias to UTC +00:00
Posted by GitBox <gi...@apache.org>.
WingsGo commented on a change in pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#discussion_r464411034
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -32,6 +32,7 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import org.apache.parquet.Strings;
Review comment:
done
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] HangyuanLiu commented on a change in pull request #4187: [TimeZone]Set UTC to defult timezone
Posted by GitBox <gi...@apache.org>.
HangyuanLiu commented on a change in pull request #4187:
URL: https://github.com/apache/incubator-doris/pull/4187#discussion_r461542445
##########
File path: fe/fe-core/src/main/java/org/apache/doris/common/util/TimeUtils.java
##########
@@ -256,7 +256,7 @@ public static String checkTimeZoneValidAndStandardize(String value) throws DdlEx
Matcher matcher = TIMEZONE_OFFSET_FORMAT_REG.matcher(value);
// it supports offset and region timezone type, "CST" use here is compatibility purposes.
boolean match = matcher.matches();
- if (!value.contains("/") && !value.equals("CST") && !match) {
+ if (!value.contains("/") && !value.equals("CST") && !value.equals("UTC") && !match) {
Review comment:
Yes. the time zone represented by this time zone abbreviation may be uncertain.
Another reason is the timezone string may transfer to be, the library of timezone in be may not support short time zone.
So may be you should test UTC can run correctly in be time function ,such as from_unixtime
----------------------------------------------------------------
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@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org