Re: [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
> * Version #5 changes:
>
>    - After performance evaluations it was decided to switch data type used
>      by `datetime.secs` from `int64_t` (which is boxed in LuaJIT) to
>      `double` (which is unboxed number).
>    - please see perf/datetime*.cc perf test as a benchmark we used for
>      comparison of 2 veriants of C implementation. Results are here -
>      https://gist.github.com/tsafin/618fbf847d258f6e7f5a75fdf9ea945b

Hi! Thanks for the patchset!

Generally looks ok. Most of my comments are style related,
except the ones covering patches 4, 5.

I like the amount of tests you introduce.

> * Version #4 changes:
>
> A lot.
>
>    - Got rid of closures usage here and there. Benchmarks revealed there is
>      20x speed difference of hash with closure handlers to the case when we
>      check keys using sequence of simple ifs.
>      And despite the fact that it looks ugler, we have updated all places which
>      used to use closures - so much performance impact does not worth it;
>
>    - c-dt library now is accompanied with corresponding unit-tests;
>
>    - due to a new (old) way of exporting public symbols, which is now used by
>      Tarantool build, we have modified c-dt sources to use approach similar
>      to that used by xxhash (or ICU) - "namespaces" for public symbols.
>      If there is DT_NAMESPACE macro (e.g. DT_NAMESPACE=tnt_) defined at the
>      moment of compilation we rename all relevant public symbols to
>      use that "namespace" as prefix
>   
> 	#define dt_from_rdn DT_NAME(DT_NAMESPACE, dt_from_rdn)
>
>      C sources continue to use original name (now as a macro), but in Lua ffi
>      we have to use renamed symbol - `tnt_dt_from_rdn`.
>
>    - We have separated code which implement MessagePack related funcionality
>      from generic (i.e. stringization) support for datetime types.
>      Stringization is in `src/core/datetime.c`, but MessagePack support is in
>      `src/core/mp_datetime.c`.
>
>    - `ctime` and `asctime` now use reentrant versions (i.e. `ctime_r` instead of
>      `ctime`, and `asctime_r` instead of `asctime`). This allows us to avoid
>      nasty problem with GC where it was possible to corrupt returned
>      static buffer regardless the fact that it was immediately passed
>      to `ffi.string`.
>      `test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua` updated to check our
>      new api `datetime.ctime`, and `datetime.asctime`.
>
>    - Introduced MessagePack unit-test which check C implementation (mp
>      encode/decode, mp_snprint, mp_fprint)
>
>    - Some special efforts have been taken to make sure we deal correctly
>      with datetime stamps which were before Unix Epoch (1970-01-01), i.e.
>      for negative stamps or timezones against epoch.
>
>    - And last, but not the least: in addition to usual getters like
>      `timestamp`, `nanoseconds`, `epoch`, we have introduced function
>      modifiers `to_utc()` or `to_tz(offset)` which allow to manipulate with
>      datetime timezone `.offset` field. Actually it does not do much now
>      (only reattributing object with different offset, but not changed
>      actual timestamp, which is UTC normalized). But different timestamp
>      modifies textual representation, which simplify debugging.
>
> * Version #3 changes:
>
>    - renamed `struct datetime_t` to `struct datetime`, and `struct
>      datetime_interval_t` to `struct datetime_interval`;
>    - significantly reworked arguments checks in module api entries;
>    - fixed datetime comparisons;
>    - changed hints calculation to take into account fractional part;
>    - provided more comments here and there;
>
> NB! There are MacOSX problems due to GLIBC specific code used (as Vlad
>      has already pointed out) - so additional patch, making it more
>      cross-compatible coming here soon...
>
> * Version #2 changes:
>
>    - fixed problem with overloaded '-' and '+' operations for datetime
>      arguments;
>    - fixed messagepack serialization problems;
>    - heavily documented MessagePack serialization schema in the code;
>    - introduced working implementation of datetime hints for storage engines;
>    - made interval related names be more consistent, renamed durations and period
>      to intervals, i.e. t_datetime_duration to datetime_interval_t,
>      duration_* to interval_*, period to interval;
>    - properly implemented all reasonable cases of datetime+interval
>      arithmetic;
>    - moved all initialization code to utils.c;
>    - renamed core/mp_datetime.c to core/datetime.c because it makes more
>      sense now;
>
> * Version #1 - initial RFC
>
> In brief
> --------
>
> This patchset implements datetime lua support in box, with serialization
> to messagepack, yaml, json and lua mode. Also it contains storage
> engines' indices implementation for datetime type introduced.
>
> * Current implementation is heavily influenced by Sci-Lua lua-time module
>    https://github.com/stepelu/lua-time  
>    e.g. you could find very similar approach for handling of operations
>    with year or month long intervals (which should be handled differently than
>    usual intervals of seconds, or days).
>
> * But internally we actually use Christian Hanson' c-dt module
>    https://github.com/chansen/c-dt  
>    (though it has been modified slightly for cleaner integration
>    into our cmake-based build process).
>
>
> Datetime Module API
> -------------------
>
> Small draft of documentation for our `datetime` module API has been
> extracted to the discussion topic there -
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043988
> We try to update documentation there once any relevant change introdcued
> to the implementaton.
>
> Messagepack serialization schema
> --------------------------------
>
> In short it looks like:
> - now we introduce new MP_EXT extension type #4;
> - we may save 1 required and 2 optional fields for datetime field;
>
> In all gory details it's explained in MessagePack serialization schema depicted here:
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043990
>
>
> https://github.com/tarantool/tarantool/issues/5941
> https://github.com/tarantool/tarantool/issues/5946
>
> https://github.com/tarantool/tarantool/tree/tsafin/gh-5941-datetime-v4
>
> Timur Safin (8):
>    build: add Christian Hansen c-dt to the build
>    lua: built-in module datetime
>    lua, datetime: display datetime
>    box, datetime: messagepack support for datetime
>    box, datetime: datetime comparison for indices
>    lua, datetime: time intervals support
>    datetime: perf test for datetime parser
>    datetime: changelog for datetime module
>
>   .gitmodules                                   |   3 +
>   CMakeLists.txt                                |   8 +
>   .../gh-5941-datetime-type-support.md          |   4 +
>   cmake/BuildCDT.cmake                          |  10 +
>   extra/exports                                 |  33 +
>   perf/CMakeLists.txt                           |   3 +
>   perf/datetime-common.h                        | 105 +++
>   perf/datetime-compare.cc                      | 213 +++++
>   perf/datetime-parser.cc                       | 105 +++
>   src/CMakeLists.txt                            |   5 +-
>   src/box/field_def.c                           |  53 +-
>   src/box/field_def.h                           |   4 +
>   src/box/lua/serialize_lua.c                   |   7 +-
>   src/box/memtx_space.c                         |   3 +-
>   src/box/msgpack.c                             |   7 +-
>   src/box/tuple_compare.cc                      |  77 ++
>   src/box/vinyl.c                               |   3 +-
>   src/lib/core/CMakeLists.txt                   |   5 +-
>   src/lib/core/datetime.c                       | 176 ++++
>   src/lib/core/datetime.h                       | 115 +++
>   src/lib/core/mp_datetime.c                    | 189 ++++
>   src/lib/core/mp_datetime.h                    |  89 ++
>   src/lib/core/mp_extension_types.h             |   1 +
>   src/lib/mpstream/mpstream.c                   |  11 +
>   src/lib/mpstream/mpstream.h                   |   4 +
>   src/lua/datetime.lua                          | 880 ++++++++++++++++++
>   src/lua/init.c                                |   4 +-
>   src/lua/msgpack.c                             |  12 +
>   src/lua/msgpackffi.lua                        |  18 +
>   src/lua/serializer.c                          |   4 +
>   src/lua/serializer.h                          |   2 +
>   src/lua/utils.c                               |  28 +-
>   src/lua/utils.h                               |  12 +
>   test/app-tap/datetime.test.lua                | 370 ++++++++
>   .../gh-5632-6050-6259-gc-buf-reuse.test.lua   |  74 +-
>   test/engine/datetime.result                   |  77 ++
>   test/engine/datetime.test.lua                 |  35 +
>   test/unit/CMakeLists.txt                      |   3 +-
>   test/unit/datetime.c                          | 381 ++++++++
>   test/unit/datetime.result                     | 471 ++++++++++
>   third_party/c-dt                              |   1 +
>   third_party/lua-cjson/lua_cjson.c             |   8 +
>   third_party/lua-yaml/lyaml.cc                 |   6 +-
>   43 files changed, 3591 insertions(+), 28 deletions(-)
>   create mode 100644 changelogs/unreleased/gh-5941-datetime-type-support.md
>   create mode 100644 cmake/BuildCDT.cmake
>   create mode 100644 perf/datetime-common.h
>   create mode 100644 perf/datetime-compare.cc
>   create mode 100644 perf/datetime-parser.cc
>   create mode 100644 src/lib/core/datetime.c
>   create mode 100644 src/lib/core/datetime.h
>   create mode 100644 src/lib/core/mp_datetime.c
>   create mode 100644 src/lib/core/mp_datetime.h
>   create mode 100644 src/lua/datetime.lua
>   create mode 100755 test/app-tap/datetime.test.lua
>   create mode 100644 test/engine/datetime.result
>   create mode 100644 test/engine/datetime.test.lua
>   create mode 100644 test/unit/datetime.c
>   create mode 100644 test/unit/datetime.result
>   create mode 160000 third_party/c-dt
>

-- 
Serge Petrenko