Time zone related be function #1598

HangyuanLiu · 2019-08-07T09:46:22Z

Time zone related be function
#1583

morningman · 2019-08-07T10:20:14Z

fe/src/main/java/org/apache/doris/qe/Coordinator.java

-        this.queryGlobals.setNow_string(DATE_FORMAT.format(new Date()));
+        this.queryGlobals.setNow_string(String.valueOf(new Date().getTime()));
+        if (context.getSessionVariable().getTimeZone().equals("CST")) {
+            this.queryGlobals.setTime_zone("Asia/Shanghai");                      


I think you should add "Asia/Shanghai" to some definition class

morningman · 2019-08-07T11:13:31Z

be/src/runtime/timestamp_value.h

+
+    void to_datetime_val(doris_udf::DateTimeVal *tv) const {
+        boost::posix_time::ptime p = boost::posix_time::from_time_t(val / 1000);
+        int _year = p.date().year();


Your code style is not compatible with Doris C++ code style.
private member should starts with _.
local variables should not start with _

imay · 2019-08-07T11:36:04Z

be/src/runtime/timestamp_value.h

+        val = timestamp;
+    }
+
+    bool from_date_time_value(const DateTimeValue& tv, std::string timezone) {


You should add some comments for these functions

imay · 2019-08-07T11:45:05Z

be/src/runtime/timestamp_value.h

+    static boost::local_time::tz_database _s_tz_database;
+};
+
+class TimestampValue {


I think we just implement a constructor with (timestamp and timezone) in DataTimeValue is enough. And we can reuse code of DateTimeValue

…to time-zone-be-function Conflicts: be/src/exprs/timestamp_functions.cpp be/test/exprs/timestamp_functions_test.cpp fe/src/main/java/org/apache/doris/qe/Coordinator.java

imay · 2019-08-11T03:38:02Z

be/src/testutil/function_utils.cpp

@@ -34,6 +34,15 @@ FunctionUtils::FunctionUtils() {
    _fn_ctx = FunctionContextImpl::create_context(
        _state, _memory_pool, return_type, arg_types, 0, false);
 }
+FunctionUtils::FunctionUtils(RuntimeState* state) {       
+        _state = state;


4 spaces indent

imay · 2019-08-11T03:38:43Z

be/src/runtime/runtime_state.h

+    int64_t timestamp() const {
+        return _timestamp;
+    }
+    const std::string timezone() const {


Suggested change

const std::string timezone() const {

const std::string& timezone() const {

be/src/runtime/datetime_value.cpp

imay · 2019-08-11T03:44:14Z

be/src/runtime/datetime_value.h

@@ -435,7 +436,8 @@ class DateTimeValue {
    }

    int64_t second_diff(const DateTimeValue& rhs) const {
-        return unix_timestamp() - rhs.unix_timestamp();
+        return unix_timestamp(TimezoneDatabase::default_time_zone) 
+            - rhs.unix_timestamp(TimezoneDatabase::default_time_zone);


For second_diff which is a literal compare, I think we should not introduce timezone. Which can bring some cost which isn't needed. Such as search if timezone exists.

And I think we can introduce another seconds such as seconds from 0000-01-01 00:00:00, which has no relation with time zone.

imay · 2019-08-11T03:52:32Z

be/src/exprs/timestamp_functions.cpp

@@ -470,7 +372,8 @@ DoubleVal TimestampFunctions::time_diff(
    }
    const DateTimeValue& ts_value1 = DateTimeValue::from_datetime_val(ts_val1);
    const DateTimeValue& ts_value2 = DateTimeValue::from_datetime_val(ts_val2);
-    int64_t timediff = ts_value1.unix_timestamp() - ts_value2.unix_timestamp();
+    int64_t timediff = ts_value1.unix_timestamp(ctx->impl()->state()->timezone()) 
+        - ts_value2.unix_timestamp(ctx->impl()->state()->timezone());


use another method to do this?

imay · 2019-08-11T03:57:41Z

be/src/exprs/timezone_db.h

+    ~TimezoneDatabase();
+
+    static void init() {
+        TimezoneDatabase();


what this mean?

I think what you need is not this TimezoneDatabase constructor.
You can move TimezoneDatabase() to this init().

I think there is no need to have object of this class. we only just need some static functions

imay · 2019-08-12T07:05:35Z

be/src/runtime/datetime_value.h

@@ -332,9 +333,9 @@ class DateTimeValue {
    // Add interval 
    bool date_add_interval(const TimeInterval& interval, TimeUnit unit);

-    int unix_timestamp() const;
+    int64_t unix_timestamp(const std::string& timezone) const;


add some comments for these two functions

imay · 2019-08-12T07:09:31Z

be/src/exprs/timestamp_functions.cpp

-        }
+DateTimeVal TimestampFunctions::now(FunctionContext* context) {
+    DateTimeValue dtv;
+    dtv.from_unixtime(context->impl()->state()->timestamp() / 1000,


check this function's return?

imay · 2019-08-12T07:09:45Z

be/src/exprs/timestamp_functions.cpp

-        }
+DoubleVal TimestampFunctions::curtime(FunctionContext* context) {
+    DateTimeValue dtv;
+    dtv.from_unixtime(context->impl()->state()->timestamp() / 1000,


check return value?

imay · 2019-08-12T07:11:01Z

be/src/runtime/datetime_value.cpp

-int DateTimeValue::unix_timestamp() const {
-    int64_t days = daynr() - calc_daynr(1970, 1, 1);
-    if (days < 0) {
+int64_t DateTimeValue::unix_timestamp(const std::string& timezone) const {


I think you should return a bool value if timezone is not valid

imay · 2019-08-12T08:31:10Z

be/src/runtime/datetime_value.h

+
+    //unix_timestamp is called with a timezone argument,
+    //it returns seconds of the value of date literal since '1970-01-01 00:00:00' UTC
+    bool unix_timestamp(int64_t& timestamp, const std::string& timezone) const;


Suggested change

bool unix_timestamp(int64_t& timestamp, const std::string& timezone) const;

bool to_unix_timestamp(int64_t* timestamp, const std::string& timezone) const;

imay

LGTM

morningman

LGTM

Support time zone function in be

79401d5

morningman requested changes Aug 7, 2019

View reviewed changes

imay requested changes Aug 7, 2019

View reviewed changes

Merge branch 'master' of https://github.com/apache/incubator-doris in…

25f7938

…to time-zone-be-function Conflicts: be/src/exprs/timestamp_functions.cpp be/test/exprs/timestamp_functions_test.cpp fe/src/main/java/org/apache/doris/qe/Coordinator.java

HangyuanLiu force-pushed the time-zone-be-function branch 2 times, most recently from 5b72023 to 460c2d5 Compare August 9, 2019 09:09

add default time zone

68357f9

HangyuanLiu force-pushed the time-zone-be-function branch from 460c2d5 to 68357f9 Compare August 9, 2019 09:13

add time zone doc

c6fcd5d

imay requested changes Aug 11, 2019

View reviewed changes

imay reviewed Aug 12, 2019

View reviewed changes

HangyuanLiu force-pushed the time-zone-be-function branch 2 times, most recently from 8b74260 to 9a22e98 Compare August 12, 2019 08:19

imay reviewed Aug 12, 2019

View reviewed changes

fix timediff

fc06993

HangyuanLiu force-pushed the time-zone-be-function branch from 9a22e98 to fc06993 Compare August 12, 2019 08:40

imay approved these changes Aug 12, 2019

View reviewed changes

morningman approved these changes Aug 12, 2019

View reviewed changes

morningman merged commit 69af50a into apache:master Aug 12, 2019

imay mentioned this pull request Sep 26, 2019

Release Notes 0.11.0 #1891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Time zone related be function #1598

Time zone related be function #1598

HangyuanLiu commented Aug 7, 2019

morningman Aug 7, 2019

morningman Aug 7, 2019

imay Aug 7, 2019

imay Aug 7, 2019

imay Aug 11, 2019

imay Aug 11, 2019

imay Aug 11, 2019

imay Aug 11, 2019

imay Aug 11, 2019

imay Aug 12, 2019

imay Aug 12, 2019

imay Aug 12, 2019

imay Aug 12, 2019

imay Aug 12, 2019

HangyuanLiu Aug 12, 2019

imay left a comment

morningman left a comment

	const std::string timezone() const {
	const std::string& timezone() const {

	bool unix_timestamp(int64_t& timestamp, const std::string& timezone) const;
	bool to_unix_timestamp(int64_t* timestamp, const std::string& timezone) const;

Time zone related be function #1598

Time zone related be function #1598

Conversation

HangyuanLiu commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imay left a comment

Choose a reason for hiding this comment

morningman left a comment

Choose a reason for hiding this comment