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

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

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.


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


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


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.


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.


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)'
...

```


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.


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

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.


7. Is it expected?

```

tarantool> datetime.now() - datetime.now()
---
- +18823 days, 16 hours, 29 minutes, 44.178836107254 seconds
...

```

I believe it should be 0.


8. Could you provide a helper that allows to convert unix time to datetime?

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;

```


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)

```


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.


  On 15.07.2021 11:18, Timur Safin via Tarantool-patches 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)
>
> 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.
>
> 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.
>
> 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.
>
> 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)
>
> 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:
>
> ```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
> -------------------------------------
>
> 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
>
> `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   |
>
> `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"`
>
> <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
> ---------------
>
> 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
>
>
>
> Timur Safin (13):
>    build: add Christian Hansen c-dt to the build
>    lua: built-in module datetime
>    test: datetime test
>    test: datetime string formatting
>    box: add messagepack support for datetime
>    lua: positive/negative cases in datetime test
>    lua: asctime and strfime fixed
>    box, lua: renamed t_datetime_tz structure to datetime_t
>    lua: calculated attributes for date
>    lua: tostring formatization in datetime.lua
>    test: allow relaxed date format without tz
>    lua: initial time duration support
>    lua: complete time duration support
>
>   .gitmodules                       |   3 +
>   CMakeLists.txt                    |   8 +
>   cmake/BuildCDT.cmake              |   6 +
>   src/CMakeLists.txt                |   4 +
>   src/box/field_def.c               |  34 +-
>   src/box/field_def.h               |   1 +
>   src/box/msgpack.c                 |   7 +-
>   src/box/tuple_compare.cc          |  24 +
>   src/exports.h                     |  26 +
>   src/lib/core/CMakeLists.txt       |   4 +-
>   src/lib/core/datetime.h           |  96 ++++
>   src/lib/core/mp_datetime.c        | 232 ++++++++
>   src/lib/core/mp_extension_types.h |   1 +
>   src/lib/mpstream/mpstream.c       |  11 +
>   src/lib/mpstream/mpstream.h       |   4 +
>   src/lua/datetime.c                |  70 +++
>   src/lua/datetime.h                |  53 ++
>   src/lua/datetime.lua              | 868 ++++++++++++++++++++++++++++++
>   src/lua/init.c                    |   6 +-
>   src/lua/msgpack.c                 |  12 +
>   src/lua/msgpackffi.lua            |   8 +
>   src/lua/serializer.c              |   4 +
>   src/lua/serializer.h              |   2 +
>   src/lua/utils.c                   |   1 -
>   test/app-tap/datetime.test.lua    | 266 +++++++++
>   test/unit/CMakeLists.txt          |   2 +
>   test/unit/datetime.c              | 220 ++++++++
>   test/unit/datetime.result         | 358 ++++++++++++
>   third_party/c-dt                  |   1 +
>   third_party/lua-cjson/lua_cjson.c |   8 +
>   third_party/lua-yaml/lyaml.cc     |   6 +-
>   31 files changed, 2326 insertions(+), 20 deletions(-)
>   create mode 100644 cmake/BuildCDT.cmake
>   create mode 100644 src/lib/core/datetime.h
>   create mode 100644 src/lib/core/mp_datetime.c
>   create mode 100644 src/lua/datetime.c
>   create mode 100644 src/lua/datetime.h
>   create mode 100644 src/lua/datetime.lua
>   create mode 100755 test/app-tap/datetime.test.lua
>   create mode 100644 test/unit/datetime.c
>   create mode 100644 test/unit/datetime.result
>   create mode 160000 third_party/c-dt
>