Re: [Tarantool-patches] [RFC PATCH 00/13] Initial datetime support

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On 16.07.2021 02:03, Timur Safin wrote:
> Thanks, Oleg, for your fast and valuable feedback!
> [ You are the fastest hand on the wild west today! ]
>
>
> : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On
> : Subject: Re: [Tarantool-patches] [RFC PATCH 00/13] Initial datetime support
> :
<...>
> :
> : 6. Does this module allows to manipulate with timezones and locales?
> :
> : Icu-date that we currently use allows it
> : (https://github.com/tarantool/icu-date).
>
> Not, at the moment. And I do like how it's handled in icu-date. And
> surprisingly that was exactly original plan - to use icu database
> for translation of timezone names into their offsets, with memorization
> of results (exactly as it's done in zone_id_cache in icu-date.lua).
>
> Locales is the strongest feature in icu-date, and should be reused
> properly while handling named-timezones. Will return here
>
> :
> : Also try to use tests from icu-date module. I think it could be useful.
> : The best option is to allow drop-in but I'm not sure that it's possible.
>
> Drop-in is hardly possible (because icu-date api looks too much mouthful
> with all their forms of a kind date:get(icu_date.fields.MILLISECOND) or
> date:get_attribute(attributes.MINIMAL_DAYS_IN_FIRST_WEEK). But tests might
> be of help. (And I assume you meant tap tests, not busted tests). And while
> we are here - why there are 2 kinds of tests? (And not only busted used because
> it's much more convenient? :) )

Because it's impossible to install "busted" via tarantoolctl, because 
nobody tests is it compatible with Tarantool.

Tarantool has built-in tap module that is used and also there is a 
luatest that seems to be ok for many cases.

(But luatest was introduced a bit later than module was improted so we 
use tap for test).

>
> : 7. Is it expected?
> :
> : ```
> :
> : tarantool> datetime.now() - datetime.now()
> : ---
> : - +18823 days, 16 hours, 29 minutes, 44.178836107254 seconds
> : ...
> :
> : ```
> :
> : I believe it should be 0.
>
> Not exactly 0, but very small fraction of second. Thanks for reporting - that
> Was artifact of last minute rename/refactoring. Here is the patch for this
> Problem and similar in +
>
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> index d28f931f6..57ef80403 100644
> --- a/src/lua/datetime.lua
> +++ b/src/lua/datetime.lua
> @@ -558,6 +558,17 @@ local function date_first(lhs, rhs)
>       end
>   end
>   
> +local function _normalize_nsec(secs, nsec)
> +    if nsec < 0 then
> +        secs = secs - 1
> +        nsec = nsec + NANOS_PER_SEC
> +    elseif nsec >= NANOS_PER_SEC then
> +        secs = secs + 1
> +        nsec = nsec - NANOS_PER_SEC
> +    end
> +    return secs, nsec
> +end
> +
>   local function datetime_sub(lhs, rhs)
>       check_date(lhs) -- make sure left is date
>       local d, s = lhs, rhs
> @@ -565,18 +576,15 @@ local function datetime_sub(lhs, rhs)
>       -- 1. left is date, right is date or delta
>       if (ffi.typeof(s) == datetime_t) or (ffi.typeof(s) == duration_t) then
>           local o
> +
>           -- if they are both dates then result is delta
>           if ffi.typeof(s) == datetime_t then
>               o = duration_new()
>           else
>               o = datetime_new()
>           end
> -        o.secs = d.secs - s.secs
> -        o.nsec = d.nsec - s.nsec
> -        if o.nsec < 0 then
> -            o.secs = d.secs - 1
> -            o.nsec = d.nsec + NANOS_PER_SEC
> -        end
> +        o.secs, o.nsec = _normalize_nsec(d.secs - s.secs, d.nsec - s.nsec)
> +
>           return o
>   
>       -- 2. left is date, right is duration in months
> @@ -602,12 +610,8 @@ local function datetime_add(lhs, rhs)
>       if (ffi.typeof(s) == datetime_t) or (ffi.typeof(s) == duration_t) then
>           local o = datetime_new()
>   
> -        o.secs = d.secs + s.secs
> -        o.nsec = d.nsec + s.nsec
> -        if o.nsec >= NANOS_PER_SEC then
> -            o.secs = d.secs + 1
> -            o.nsec = d.nsec - NANOS_PER_SEC
> -        end
> +        o.secs, o.nsec = _normalize_nsec(d.secs + s.secs, d.nsec + s.nsec)
> +
>           return o
>   
>       -- 2. left is date, right is duration in months
>
> :
> :
> : 8. Could you provide a helper that allows to convert unix time to datetime?
>
> Will introduce something, yes, we need one.

Thanks!

>
> :
> : I tried this way but it seems an interval instead of datetime.
> :
> : ```
> :
> : tarantool> datetime.seconds(datetime.now().seconds)
> : ---
> : - +18823 days, 16 hours, 34 minutes, 10.283248901367 seconds
> : ...
> :
> : ```
> :
> :
> : 9. Lua output doesn't support datetime
> :
> : ```
> :
> : tarantool> \set output yaml
> : ---
> : - true
> : ...
> :
> : tarantool> datetime.now()
> : ---
> : - 2021-07-15T19:46:15.797333+03:00
> : ...
> :
> : tarantool> \set output lua
> : true;
> : tarantool> datetime.now()
> : nil;
> :
> : ```
>
> Interesting, didn't know of that. Do you have any hints where it's
> Implemented for, say, decimal, or uuid?

https://github.com/tarantool/tarantool/blob/master/src/box/lua/serialize_lua.c

>
> :
> :
> : 10. You can export also the number of months/days in week as constansts.
> :
> : Probably it could be useful to make something like:
> :
> : ```
> : datetime_object:set('month', datetime.months.JANUARY)
> :
> : ```
>
> Hmm, hmm. Need to discuss with others. Didn't see so much details exported
> in python or perl datetime modules. How frequently you needed such
> functionality from icu-date?

I can't anser this question. Because we just provide some interfaces for 
our customers that is actually

wrappers over icu-date.

As I can see there are several function that uses it. There is one of them:

```

-- Returns the start of the current UTC date (zero hours, minutes, 
seconds, etc.)
-- The result is in nanoseconds
local function curr_date_nsec()
     local date = get_icu_date_instance()

     date:set_millis(date:now())
     local year = date:get(icu_date.fields.YEAR)
     local month = date:get(icu_date.fields.MONTH)
     local day_of_month = date:get(icu_date.fields.DAY_OF_MONTH)

     date:set_millis(0)
     date:set(icu_date.fields.YEAR, year)
     date:set(icu_date.fields.MONTH, month)
     date:set(icu_date.fields.DAY_OF_MONTH, day_of_month)

     return date:get_millis() * NSEC_IN_MILLISEC
end

```

However here we need to just strip datetime to just date.

Also it seems that getters are used in more intensive way:

```

local function nsec_to_day_of_week(nsec)
     checks("number|cdata")
     local millis = to_milliseconds(nsec)
     local date = get_icu_date_instance()
     date:set_millis(millis)
     return date:get(icu_date.fields.DAY_OF_WEEK)
end

local function parse_unix_time(nsec)
     checks("number|cdata")
     local millis = to_milliseconds(nsec)
     local date = get_icu_date_instance()
     date:set_millis(millis)
     return {
         year = date:get(icu_date.fields.YEAR),
         month = date:get(icu_date.fields.MONTH) + 1, -- starts with 0
         day = date:get(icu_date.fields.DAY_OF_MONTH),
     }
end

```

>
> :
> :
> : It was brief feedback as from (I hope) future customer of this module.
> :
> : If I've found another interface issues I'll give you feedback in next
> : e-mails.
> :
> : I didn't see code itself if you need it I can review this patchset in
> : more detailed way.
>
> That would be interesting to hear, thanks in advance!
>
> Timur
>
I think I'll do it on the next iteration of this patch. Thanks for your 
work on it.