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

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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
: 
: Thanks for your patch.
: 
: 
: I'd want to use native datetime data type in one project 
: that should be released soon. So I'll give you some feedback because I'm
: really interested in this patch. I probably won't perform deep review and I'll
: just give some feedback about an interface.

Very good! At this stage of feature development feedback to api created is 
the most valuable.

: 1. I see you implemented "datetime" module as ffi wrapper over c-dt.
: 
: Why did you choose FFI and could you show that FFI is better than Lua C API?
: 
: I know there are such benchmark for decimal and they show that FFI was
: slower than Lua C API
: (https://github.com/tarantool/tarantool/issues/692#issuecomment-505043929).

I didn't see these results, that was prior my Tarantool times. Very interesting!
I've asked to LuaJIT woodoo masters for their feedback (it will be very 
unfortunate to see the necessity to rework whole C interface from such 
convenient FFI mechanism). But, well, let us check performance here.

: 2. Could you please implement a hepler `is_datetime`? Uuid and decimal
: have is_* methods.

Yes, will do so.

: 
: 
: 3. Modue throws unhandled error:
: 
: ```
: 
: tarantool> datetime.now() == newproxy()
: ---
: - error: 'builtin/datetime.lua:232: attempt to index local ''rhs'' (a
: userdata value)'
: ...
: 
: ```
: 
: I think you can see how works uuid module
: (https://github.com/tarantool/tarantool/blob/master/src/lua/uuid.lua)
: 
: to fix such errors.

Yes, careful processing of invalid arguments now is the missing point
at the moment. [Will is is_datetime() wherever it's reasonable.]

: 
: 4. Parse function has quite strange output format
: 
: ```
: 
: tarantool> datetime.parse_date('2016-12-18')
: ---
: - 2016-12-18T00:00Z
: - 10
: ...
: 
: tarantool> datetime.parse_date('2016-13-18')
: ---
: - null
: - 0
: ...
: 
: ```
: 
: 
: Do we really need the second value in case of success?
: 
: Do we really need 0 as the second return value in case of failue?
: Probably we should extend error message somehow.

Good question! I could explain why it behave so strangely - underlying
c-dt dt_parse_iso_date(), dt_parse_iso_time() and dt_parse_iso_timezone()
could deal with incorrect suffices, but return the length of properly
parsed part of string, thus I return this length as 2nd value in returned
tuple. You could see how it's used in the test/app-tap/datetime.test.lua 
cases named "Parse iso date - valid strings" and "Parse iso date - 
invalid strings". I agreed that it looks strangely, and we may rework 
a way we signal parse errors here.

: 
: 
: 5. More strange errors, please improve the validation.
: 
: ```
: 
: tarantool> datetime.days()
: ---
: - error: 'builtin/datetime.lua:203: attempt to perform arithmetic on
: local ''d'' (a
:      nil value)'
: ...
: 
: tarantool> datetime.ctime()
: ---
: - error: 'builtin/datetime.lua:550: bad argument #1 to ''typeof'' (C
: type expected,
:      got nil)'
: ...
: 
: ```

Yes, will be handled similarly to the cases above (where is_datetime() 
Would be used for invalid arguments handling).

: 
: 
: Also I see some assertions in code (e.g. `assert(v >= 0 and v < 61)`).
: This yields to
: 
: "assertion failed" message that is not completely clear.

Agreed that many assertions there are not user-friendly and ask
for some massaging.

: 
: 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? :) )

: 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. 

: 
: 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?

: 
: 
: 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?

: 
: 
: 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