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
Set timezone variable in fe #1587
Conversation
@@ -211,6 +244,12 @@ public static void setVar(SessionVariable sessionVariable, SetVar setVar) throws | |||
} | |||
// Check variable attribute and setVar | |||
checkUpdate(setVar, ctx.getFlag()); | |||
// Check variable time_zone value is valid | |||
if (setVar.getVariable().toLowerCase().equals("time_zone")) { | |||
if (!setVar.getValue().getStringValue().equalsIgnoreCase("SYSTEM")) { |
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.
we don't support "system" variable because fe system time zone is different
@@ -203,6 +209,33 @@ private static void checkUpdate(SetVar setVar, int flag) throws DdlException { | |||
} | |||
} | |||
|
|||
// Check if the time zone_value is valid | |||
private static void checkTimeZoneValid(SetVar setVar) throws DdlException { |
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 think is a common function that can be used by others.
we'd better to put this function another place.
if (!value.contains("/") && !value.equals("CST") && !m.matches()) { | ||
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, setVar.getValue().getStringValue()); | ||
} | ||
if (m.matches()) { |
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.
Can you add some comments for this function to let people known what it want to do?
@@ -67,6 +68,8 @@ | |||
public static final int READ_ONLY = 8; | |||
// Variables with this flag can not be seen with `SHOW VARIABLES` statement. | |||
public static final int INVISIBLE = 16; | |||
// set CST to +08:00 instead of America/Chicago | |||
public static final ImmutableMap<String, String> timeZoneAliasMap = ImmutableMap.of("CST", "Asia/Shanghai"); |
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 think it's better to put it in TimeUtils
public static void checkTimeZoneValid(String value) throws DdlException { | ||
try { | ||
// match offset type, such as +08:00, -07:00 | ||
Pattern p = Pattern.compile("^[+-]{1}\\d{2}\\:\\d{2}$"); |
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 think you can make this static to compile it only one time
if (!value.contains("/") && !value.equals("CST") && !m.matches()) { | ||
ErrorReport.reportDdlException(ErrorCode.ERR_UNKNOWN_TIME_ZONE, value); | ||
} | ||
if (m.matches()) { |
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.
you can make it better to avoid calling matches two times?
This is part of support multiple time zone(Issue #1583 ), it just set the timezone variable in fe.
something to notice:
#1583