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

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

Hello Igor,

Please see some responses (but better see the next iteration of 
patchset to be sent later)

: From: Igor Munkin <imun@tarantool.org>
: Subject: Re: [RFC PATCH 00/13] Initial datetime support
: 
: Timur,
: 
: Thanks for the series! I wonder, why you decided to reinvent several
: approaches being already used for quite similar Tarantool types: decimal
: and uuid. Frankly speaking, I expect you introduce datetime the very
: *very* similar way. I didn't make the precise review for this series
: (this is waste of time), so please consider my major comments below.
: 
: Let's start with top 3 comments to the eye catching problems I've found
: while looking at the patches.
: 
: 1. Tests have to be committed in the same scope the corresponding
:    changes in Tarantool are introduced.
: 2. Docbot request is required for the changes you're introducing: this
:    is the place where you can leave the description you've added below.
: 3. Please consider approaches used for decimal and uuid: almost every
:    Oleg's comment came from the bugs fixed for these types.
: 
: As I write above, this is not a precise review, so I don't even start
: nitpicking the patches and simply address you to this document[1] and
: our dev guide[2]. Please, check whether your next series respects these
: guidelines.
: 
: On 15.07.21, Timur Safin wrote:
: > Here is the preliminary RFC version of a patchset, which is implementing
: > datetime lua support in box, their messagepack, yaml, json serialization
: > support, and a set of c and lua tests.
: >
: > * It's heavily influenced by Sci-Lua lua-time module implementation
: >   https://github.com/stepelu/lua-time
: >   e.g. you could find very similar approach for handling of operations
: >   with year and months periods (which should be handled differently than
: >   usual seconds, days periods).
: >
: > * But internally it actually uses Christina Hanson' c-dt module
: >   https://github.com/chansen/c-dt
: >   (though it has been modified slightly for cleaner integration
: >   into cmake build process)
: 
: BTW, submodule points to the very strange repo: please use either
: vanilla one, or create a fork into Tarantool repositories.

At the moment it points to my own fork, because I do not have 
Permissions, and unable to create new repo in our organization. 
I'll ask Sasha Turenko for a help in moving this repo.

: 
: >
: > This is preliminary version of patchset, with unoptimal patches divisions
: > (like you could see patches with rename refactorings), but, due to an
: amount
: > of code in patchset, and limited time have, I send patchset as RFC today,
: > expecting to receive initial feedback for various parts of a series.
: 
: Well, I guess I can stop here for the initial feedback.
: 
: I have no idea, why you decided to introduce a separate tiny module
: src/lua/datetime.c just for initializing several constants and a
: oneliner for pushing datetime GCcdata on Lua stack. This module is
: excess. Please consider the way this is done for uuid and move
: everything to src/lua/utils.c that encloses all illegitimate tricks with
: Lua C API.

I'll look there. 

: 
: >
: > Prior plan was to split series into one, which introduces datetime module
: api,
: > and another one with datetime messagepack serialization implementation.
: > But here they left as part of a same series. Sorry, for invonvenience -
: I'll
: > reshuffle order according to feedback provided.
: 
: [I personally don't know what kind of the feedback you need to reshuffle
: the series...]
: 
: >
: > The problem, which was stopping such simple division - that `datetime.lua`
: > module uses `datetime_to_string()` ffi function for stringization of a
: > datetime object. And this function has been introduced as part of
: > messagepack/yaml/json serialization support. Yes, intra-dependendencies
: > between parts looks suboptimal, and I'm very open to suggestions how to
: > divide it properly.
: 
: Again, vse uje ukradeno^W pridumano do nas: consider the way this is
: solved for uuid (within src/lib/uuid/tt_uuid.[ch]), since you've chosen
: FFI approach for this type (we'll return to this question later).

I've seen tt_uuid library, will consider extraction of datetime stringization
Helper somewhere similarly to tt_uuid. 

: 
: >
: > Module API
: > ==========
: >
: > To simplify review of a code in patchset below you could find preliminary
: > documentation of a datetime api module. Eventually we may put it
: > as .rst file in documentation repository (though, personally, I'd prefer
: > to have all modules api committed to the same repo as code, but I digress)
: 
: I guess, I'll leave general comments right here.
: 
: 1. <asctime>, <strftime>, <ctime> can be implemented via FFI by user,
:    and you simply provide a wrapped. Anyway, it's already implemented,
:    so OK.
: 2. IMHO, you test too many exotic formats and corner cases of creating
:    datetime object. I doubt somebody will use at least 10% of the
:    provided functionality (especially in beta).

Those are types of standard ISO-8601 date/time formats with few extensions.

: 3. Datetime and interval arithmetics -- this is what bother me a lot.
:    I believe every average Lua user can parse date into the table and
:    build the string from this table back. But nobody wants to mess with
:    all that hell with leap years arithmetics, leap seconds, etc.

Agreed that convenient interval arithmetic is the most important part
of datetime module. 

: 4. Furthermore, I don't understand the following behaviour:
:    | tarantool> d = require('datetime')
:    | ---
:    | ...
:    |
:    | tarantool> ft = require('ffi').typeof
:    | ---
:    | - +2 days, 0 hours, 0 minutes, 0 seconds
:    | ...
:    |
:    | tarantool> d.days(2) + d.days(1)
:    | ---
:    | - 1970-01-04T00:00Z
:    | ...
:    |
:    | tarantool> ft(d.days(2) + d.days(1))
:    | ---
:    | - ctype<struct datetime_t>
:    | ...
:    |
:    | tarantool> d.days(2) - d.days(1)
:    | ---
:    | - error: 'builtin/datetime.lua:554: date/time expected'
:    | ...
:    |
:    I believe it's quite natural to do arithmetics with intervals to
:    create a new one, isn't it?

At the moment there was limitation that at least 1 of 2 arguments
passed (either left or right) have to be full datetime object, and
only another one could be interval. (Though in __add we could
summarize 2 intervals, as you've shown above, but that was oversight
in original patchset, which later has been "fixed")

Now, while you are mentioning it, I tend to agree that it's ok to
summarize or subtract 2 intervals, and I'll relax this restriction. 

: 
: >
: > Datetime module
: > ===============
: >
: > This module allows to parse date/time stamps, manipulate with them, and
: > generate textual representation. Parsing of date constants will handle
: > all ISO-8601 formats, and deal with some extensions.
: >
: > For internal date representation it uses cdata structures of a form:
: 
: BTW, I agree w/ Oleg and see no motivation why FFI is chosen for
: implementation: decimal is implemented via Lua C API, uuid -- via FFI.
: Are there any proofs and benchmarks for datetime? Does it violate any
: limit mentioned here[1]?

Using ffi to call c-dt was an obvious, easiest way to call c-dt. And taking
into account that uuid is still using FFI I conclude there is no strict
policy here and ffi is acceptable. 

Did not understand "Does it violate any limit mentioned here[1]?" what are
you talking about here? Could you please elaborate?

: 
: >
: > ```c++
: > struct t_datetime {
: > 	int secs;
: > 	int nsecs;
: > 	int offset;
: > };
: > ```
: >
: > Where:
: >
: > -   secs is the (signed) number of seconds since epoch 1970-01-01T00:00Z;
: > -   nsecs is number of nanoseconds since beginning of day;
: > -   offset is the timezone offset (in minutes);
: >
: > `datetime()` ? create datetime object
: > -------------------------------------
: >
: 
: Both decimal and uuid modules provides <new> function, but you decided
: to name the datetime constructor <datetime>. I see no reason for such
: inconsistency.

Will add `new` as entry point.

But, WFIW, here we use the same approach as in uuid.lua where uuid() is
shortcut for uuid.new()

: 
: > Create date/time object using either string literals, or initialization
: > object. When given string literals it behaves like wrapper around
: > parse() method.
: >
: > When there is initialization object it could create date/time object
: > using these attributes below:
: >
: > | **Easy way** |
: |
: > |--------------|----------------------------------------------------------
: ---------------------------------------|
: > | secs         | Seconds since epoch
: |
: > | nsec         | Nanoseconds since midnight
: |
: > | offset       | Time-zone offset in minutes
: |
: > | **YMD part** |
: |
: > | year         | Year in range \[1..9999\]
: |
: > | month        | Month in range \[1..12\]
: |
: > | day          | Day in month in range \[1..31\]
: |
: > | **HMS part** |
: |
: > | hour         | Hour in range \[0..23\]
: |
: > | minute       | Minute in range \[0..59\]
: |
: > | second       | Seconds in range \[0..60\]. It allows to have fraction
: part in which case it goes to nsec field |
: > | tz           | Timezone offset (in minutes) for HMS part
: |
: >
: > Example:
: >
: > ```lua
: >
: > datetime = require `datetime`
: >
: > d = datetime{secs = 0, nsec = 0, offset = 0}
: > d = datetime(?1970-01-01?)
: > d = datetime{year = 1970, month = 1, day = 1}
: > ```
: >
: > `delta()` ? create time duration object
: > ---------------------------------------
: >
: > TBD
: 
: Why delta? Sci-Lua uses period, that is quite accurate term. SQL
: provides INTERVAL type. I guess you can choose something from this.

Yup, interval is much better name, will use it instead.

: 
: >
: > `parse()` ? parse full ISO-8601 string
: > --------------------------------------
: >
: > Parse full length date/time literal, which may be in ISO-8601 format of
: > any of extended formats supported by `parse_date()`, `parse_time()` or
: > `parse_timezone()`
: >
: > It deals with date/time string in format
: >
: > `date ([Tt ] time ([ ] time_zone)? )?`
: >
: > Where time or `time_zone` parts may be omitted.
: >
: > Example:
: >
: > ```lua
: >
: > datetime = require `datetime`
: >
: > d = datetime.parse(`1970-01-01`)
: > d = datetime.parse(`1970-01-01T00:00:00Z`)
: > d = datetime.parse(`1970-01-01T02:00:00+02:00`)
: > ```
: >
: > `parse_date()` ? parse ISO-8601 date literal
: > --------------------------------------------
: >
: > Parse date string literal, return partial date object which has
: > precision of up-to date period of time.
: >
: > A number of standard ISO-8601 formats supported, plus there are some
: > relaxed formats which are of frequently use:
: >
: > | Basic    | Extended   |                          |
: > |----------|------------|--------------------------|
: > | 20121224 | 2012-12-24 | Calendar date (ISO 8601) |
: > | 2012359  | 2012-359   | Ordinal date (ISO 8601)  |
: > | 2012W521 | 2012-W52-1 | Week date (ISO 8601)     |
: > | 2012Q485 | 2012-Q4-85 | Quarter date             |
: >
: > `parse_time()` ? parse ISO-8601 time literal
: > --------------------------------------------
: >
: > Parse time string literal, return partial date/time object, which
: > defines time period inside of single date.
: >
: > A number of standard ISO-8601 formats supported, plus there are some
: > relaxed formats which are of frequently use:
: >
: > | Basic             | Extended            |
: > |-------------------|---------------------|
: > | T12               | N/A                 |
: > | T1230             | T12:30              |
: > | T123045           | T12:30:45           |
: > | T123045.123456789 | T12:30:45.123456789 |
: > | T123045,123456789 | T12:30:45,123456789 |
: >
: > The time designator T may be omitted.
: >
: > `parse_zone()` ? parse ISO-8601 time zone
: > -----------------------------------------
: >
: > Parse time-zone string literal, return partial date/time object, which
: > defines timezone offset in minutes sing GMT.
: >
: > A number of standard ISO-8601 formats supported, plus there are some
: > relaxed formats which are of frequently use:
: >
: > | Basic | Extended |
: > |-------|----------|
: > | Z     | N/A      |
: > | ?hh   | N/A      |
: > | ?hhmm | ?hh:mm   |
: 
: AFAIU, <parse_{date,time,zone}> are the parts of <parse> routine, right?
: Then why do you provide a separate interfaces for these purposes?

Because there are some cases when you need to parse parts of full
date-time literal, like parsing only time part. I kept them public,
similarly to how it was done in the original c-dt. I see not much
point to hide them if we already have such building blocks for external
consumption.

: 
: >
: > `tostring()` ? convert datetime object to string
: > ------------------------------------------------
: >
: > Return string representation (probably compact if there are some parts
: > missing) of a date-time objects passed
: >
: > `now()` ? return current date/time
: > ----------------------------------
: >
: > `now()` returns local date and time object. It will use local time-zone
: > and nanosecond precision.
: >
: > `strftime()` ? convert date object to string using format
: > ---------------------------------------------------------
: >
: > `strftime()` is the FFI wrapper around strftime() function in LIBC. It
: > supports all the same flags which supports strftime() from host OS.
: >
: > See
: > <https://pubs.opengroup.org/onlinepubs/000095399/functions/strftime.html>
: > for more details.
: >
: > `asctime()` ? convert date object to string using asctime predefined
: format
: > --------------------------------------------------------------------------
: -
: >
: > `asctime()` is the FFI wrapper over `asctime_r()` from a host libc.
: asctime
: > returns string in the form `"Sun Sep 16 01:03:52 1973\\n\\0"`
: >
: > <https://pubs.opengroup.org/onlinepubs/009695399/functions/asctime.html>
: >
: > `ctime()` ? convert local time to string using ctime() predefined format
: > ------------------------------------------------------------------------
: >
: > `ctime()` is the FFI wrapper over `ctime_r()` in the host libc. ctime
: > returns string in the form `"Sun Sep 16 01:03:52 1973\\n\\0"`
: 
: This is not quite right, considering your code, so here is the question:
: do you mean ctime(3) or ctime_r(3).

ctime_r is _reentrant_ version of ctime, which behave identically. I'll
use more careful wording then, to not confuse reader with 2 separate names.

: 
: >
: > <https://pubs.opengroup.org/onlinepubs/009695399/functions/ctime.html>
: >
: > The difference of `ctime()` and `asctime()` is that former is returning
: > local time zone formatted, while the latter will deal with GMT.
: >
: > Examples:
: >
: > ```
: > tarantool> date = require 'datetime'
: > ---
: > ...
: > tarantool> T = date.now()
: > ---
: > ...
: > tarantool> T
: > ---
: > - 2021-07-14T01:36:48.554105+03:00
: > ...
: > tarantool> date.asctime(T)
: > ---
: > - 'Tue Jul 13 22:36:48 2021
: > '
: > ...
: > tarantool> date.ctime(T)
: > ---
: > - 'Wed Jul 14 01:36:48 2021
: > '
: > ...
: > ```
: >
: > Date attribute accessors
: > ------------------------
: >
: > |                |
: |
: > |----------------|--------------------------------------------------------
: ---------|
: > | `timestamp`    | Calculate timestamp with seconds and nanoseconds parts
: combined |
: > | `nanoseconds`  | Number of nanoseconds in time part
: |
: > | `microseconds` | Number of microseconds in time part
: |
: > | `milliseconds` | Number of milliseconds in time part
: |
: > | `seconds`      | Alias to timestamp
: |
: > | `minutes`      | Number of minutes in time part
: |
: > | `hours`        | Number of hours in time part
: |
: > | `days`         | Number of days in time part
: |
: >
: > ```
: > tarantool> d = date{year = 1970, month = 1, day = 1, hour = 0, minute =
: 10, second=10}
: > tarantool> d.secs
: > ---
: > - 610
: > ...
: > tarantool> d.nsec
: > ---
: > - 0
: > ...
: > tarantool> d.offset
: > ---
: > - 0
: > ...
: > tarantool> d.nanoseconds
: > ---
: > - 610000000000
: > ...
: > tarantool> d.milliseconds
: > ---
: > - 610000
: > ...
: > tarantool> d.hours
: > ---
: > - 0.16944444444444
: > ...
: > tarantool> d.minutes
: > ---
: > - 10.166666666667
: > ...
: > ```
: >
: > Date arithmetic
: > ---------------
: >
: 
: The most interesting part of the doc by the way.

Yeah, agreed. See it soon!


: 
: > TBD
: >
: >
: > https://github.com/tarantool/tarantool/issues/5941
: > https://github.com/tarantool/tarantool/issues/5946
: > https://github.com/tarantool/tarantool/tree/tsafin/gh-5941-datetime-V2
: >
: >
: >
...
: 
: [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure
: [2]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/
: [1]: http://luajit.org/ext_ffi_semantics.html#status
: 
: --
: Best regards,
: IM

Regards,
Timur