Re: [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime

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

16.08.2021 02:59, Timur Safin via Tarantool-patches пишет:
> * created a new Tarantool built-in module `datetime`;
> * register cdef types for this module;
> * export some `dt_*` functions from `c-dt` library;
>
> * implemented `ctime`, `asctime` and `strftime` as
>    ffi wrappers in C kernel code:
>
> We used to use heavy ffi-related Lua code, but it appears
> to be quite fragile, if we run this code on different OS
> targets. To make this Lua code less platfrom dependent we
> have moved all platform specifics to the C level.
> Specifically we want to avoid dependence on `struct tm {}`
> layout and GLIBC-specific `strftime` behaviour (which
> differs to BSD/Mac LIBC behaviour).
>
> * introduced simple datetime tests
>
> Created app-tap test for new builtin module `datetime.lua`,
> where we specifically check:
>
> - cases of simple datetime string formatting using:
>    - asctime (gmt time);
>    - ctime (local TZ time);
>    - strftime (using given format).
>
> - verify that we parse expected, and fail with unexpected:
>    - for that, in the datetime.lua, we have extended api of `parse_date()`,
>      `parse_time()`, and `parse_time_zone()` so they return
>      not only parsed object, but also a length of parsed substring;
>    - which allows us to parse even _partially_ valid strings like
>      "20121224 Foo bar".
>
> - check calculated attributes to date object, e.g.:
>    - timestamp, seconds, microseconds, minute, or hours
>    - to_utc(), and to_tz() allow to switch timezone of a
>      datetime object. It's not changing much - only timezone
>      but that impacts textual representation of a date.
>
> Part of #5941

Please, add a docbot request to the commit message.
Here it should say that you introduce lua datetime module
and describe shortly what the module does.

> ---
>   cmake/BuildCDT.cmake                          |   2 +
>   extra/exports                                 |  26 +
>   src/CMakeLists.txt                            |   2 +
>   src/lib/core/CMakeLists.txt                   |   1 +
>   src/lib/core/datetime.c                       |  96 ++++
>   src/lib/core/datetime.h                       |  95 ++++
>   src/lua/datetime.lua                          | 500 ++++++++++++++++++
>   src/lua/init.c                                |   4 +-
>   src/lua/utils.c                               |  27 +
>   src/lua/utils.h                               |  12 +
>   test/app-tap/datetime.test.lua                | 206 ++++++++
>   .../gh-5632-6050-6259-gc-buf-reuse.test.lua   |  74 ++-
>   12 files changed, 1043 insertions(+), 2 deletions(-)
>   create mode 100644 src/lib/core/datetime.c
>   create mode 100644 src/lib/core/datetime.h
>   create mode 100644 src/lua/datetime.lua
>   create mode 100755 test/app-tap/datetime.test.lua
>
> diff --git a/cmake/BuildCDT.cmake b/cmake/BuildCDT.cmake
> index 343fb1b99..80b26c64a 100644
> --- a/cmake/BuildCDT.cmake
> +++ b/cmake/BuildCDT.cmake
> @@ -5,4 +5,6 @@ macro(libccdt_build)
>       file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/)
>       add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/c-dt
>                        ${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/build/)
> +    set_target_properties(cdt PROPERTIES COMPILE_FLAGS "-DDT_NAMESPACE=tnt_")
> +    add_definitions("-DDT_NAMESPACE=tnt_")
>   endmacro()


This change belongs to the previous commit, doesn't it?


> diff --git a/extra/exports b/extra/exports
> index 9eaba1282..80eb92abd 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -148,8 +148,34 @@ csv_feed
>   csv_iterator_create
>   csv_next
>   csv_setopt
> +datetime_asctime
> +datetime_ctime
> +datetime_now
> +datetime_strftime
> +decimal_unpack
>   decimal_from_string
>   decimal_unpack
> +tnt_dt_dow
> +tnt_dt_from_rdn
> +tnt_dt_from_struct_tm
> +tnt_dt_from_yd
> +tnt_dt_from_ymd
> +tnt_dt_from_yqd
> +tnt_dt_from_ywd
> +tnt_dt_parse_iso_date
> +tnt_dt_parse_iso_time_basic
> +tnt_dt_parse_iso_time_extended
> +tnt_dt_parse_iso_time
> +tnt_dt_parse_iso_zone_basic
> +tnt_dt_parse_iso_zone_extended
> +tnt_dt_parse_iso_zone_lenient
> +tnt_dt_parse_iso_zone
> +tnt_dt_rdn
> +tnt_dt_to_struct_tm
> +tnt_dt_to_yd
> +tnt_dt_to_ymd
> +tnt_dt_to_yqd
> +tnt_dt_to_ywd
>   error_ref
>   error_set_prev
>   error_unref
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 97b0cb326..4473ff1da 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -51,6 +51,8 @@ lua_source(lua_sources ../third_party/luafun/fun.lua)
>   lua_source(lua_sources lua/httpc.lua)
>   lua_source(lua_sources lua/iconv.lua)
>   lua_source(lua_sources lua/swim.lua)
> +lua_source(lua_sources lua/datetime.lua)
> +
>   # LuaJIT jit.* library
>   lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bc.lua)
>   lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bcsave.lua)
> diff --git a/src/lib/core/CMakeLists.txt b/src/lib/core/CMakeLists.txt
> index 2cd4d0b4f..8bc776b82 100644
> --- a/src/lib/core/CMakeLists.txt
> +++ b/src/lib/core/CMakeLists.txt
> @@ -30,6 +30,7 @@ set(core_sources
>       decimal.c
>       mp_decimal.c
>       cord_buf.c
> +    datetime.c
>   )
>   
>   if (TARGET_OS_NETBSD)
> diff --git a/src/lib/core/datetime.c b/src/lib/core/datetime.c
> new file mode 100644
> index 000000000..c48295a6f
> --- /dev/null
> +++ b/src/lib/core/datetime.c
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +

As far as I know, we switched to the following license format recently:

/*
  * SPDX-License-Identifier: BSD-2-Clause
  *
  * Copyright 2010-2021, Tarantool AUTHORS, please see AUTHORS file.
  */

See, for example:

./src/box/module_cache.c: * SPDX-License-Identifier: BSD-2-Clause
./src/box/lua/lib.c: * SPDX-License-Identifier: BSD-2-Clause
./src/box/lua/lib.h: * SPDX-License-Identifier: BSD-2-Clause
./src/box/module_cache.h: * SPDX-License-Identifier: BSD-2-Clause
./src/lib/core/cord_buf.c: * SPDX-License-Identifier: BSD-2-Clause
./src/lib/core/crash.c: * SPDX-License-Identifier: BSD-2-Clause
./src/lib/core/cord_buf.h: * SPDX-License-Identifier: BSD-2-Clause
./src/lib/core/crash.h: * SPDX-License-Identifier: BSD-2-Clause


<stripped>


> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> new file mode 100644
> index 000000000..1a8d7e34f
> --- /dev/null
> +++ b/src/lib/core/datetime.h
> @@ -0,0 +1,95 @@
> +#pragma once
> +/*
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +

Same about the license.

And I'd move the "#pragma once" below the license comment.
Otherwise it's easily lost. Up to you.

> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdio.h>

AFAICS you don't need stdio included here.

> +#include "c-dt/dt.h"
> +
> +#if defined(__cplusplus)
> +extern "C"
> +{
> +#endif /* defined(__cplusplus) */
> +
> +#ifndef SECS_PER_DAY
> +#define SECS_PER_DAY          86400
> +#define DT_EPOCH_1970_OFFSET  719163

Please, add a short comment on what this is.
I had to spend some time googling to understand.

So, please mention that this is measured in days from 01-01-0001.

> +#endif
> +
> +/**
> + * Full datetime structure representing moments
> + * since Unix Epoch (1970-01-01).
> + * Time is kept normalized to UTC, time-zone offset
> + * is informative only.
> + */
> +struct datetime {
> +	/** seconds since epoch */
> +	double secs;
> +	/** nanoseconds if any */
> +	int32_t nsec;


As discussed, let's make nsec a uint32_t, since
nsec part is always positive.


> +	/** offset in minutes from UTC */
> +	int32_t offset;
> +};
> +
> +/**
> + * Date/time interval structure
> + */
> +struct datetime_interval {
> +	/** relative seconds delta */
> +	double secs;
> +	/** nanoseconds delta */
> +	int32_t nsec;
> +};
> +


Please start comments with a capital letter and end them with a dot.


> +/**
> + * Convert datetime to string using default asctime format
> + * "Sun Sep 16 01:03:52 1973\n\0"
> + * Wrapper around reenterable asctime_r() version of POSIX function
> + * @param date source datetime value
> + * @sa datetime_ctime
> + */
> +char *
> +datetime_asctime(const struct datetime *date, char *buf);
> +
> +char *
> +datetime_ctime(const struct datetime *date, char *buf);
> +
> +size_t
> +datetime_strftime(const struct datetime *date, const char *fmt, char *buf,
> +		  uint32_t len);
> +
> +void
> +datetime_now(struct datetime * now);
> +
> +#if defined(__cplusplus)
> +} /* extern "C" */
> +#endif /* defined(__cplusplus) */
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> new file mode 100644
> index 000000000..ce579828f
> --- /dev/null
> +++ b/src/lua/datetime.lua
> @@ -0,0 +1,500 @@
> +local ffi = require('ffi')
> +
> +ffi.cdef [[
> +
> +    /*
> +    `c-dt` library functions handles properly both positive and negative `dt`
> +    values, where `dt` is a number of dates since Rata Die date (0001-01-01).
> +
> +    For better compactness of our typical data in MessagePack stream we shift
> +    root of our time to the Unix Epoch date (1970-01-01), thus our 0 is
> +    actually dt = 719163.
> +
> +    So here is a simple formula how convert our epoch-based seconds to dt values
> +        dt = (secs / 86400) + 719163
> +    Where 719163 is an offset of Unix Epoch (1970-01-01) since Rata Die
> +    (0001-01-01) in dates.
> +
> +    */


I'd move the comments outside the ffi.cdef block. This way they'd get
proper highlighting, and it would be harder to mess something up
by accidentally deleting the "*/"


> +    typedef int dt_t;
> +
> +    // dt_core.h
> +    dt_t     tnt_dt_from_rdn     (int n);
> +    dt_t     tnt_dt_from_ymd     (int y, int m, int d);
> +
> +    int      tnt_dt_rdn          (dt_t dt);
> +
> +    // dt_parse_iso.h
> +    size_t tnt_dt_parse_iso_date          (const char *str, size_t len, dt_t *dt);
> +    size_t tnt_dt_parse_iso_time          (const char *str, size_t len, int *sod, int *nsec);
> +    size_t tnt_dt_parse_iso_zone_lenient  (const char *str, size_t len, int *offset);
> +
> +    // datetime.c


Also you may split the definitions into multiple ffi.cdef[[]] blocks
if you want to add some per-definition comments.


> +    int
> +    datetime_to_string(const struct datetime * date, char *buf, uint32_t len);
> +
> +    char *
> +    datetime_asctime(const struct datetime *date, char *buf);
> +
> +    char *
> +    datetime_ctime(const struct datetime *date, char *buf);
> +
> +    size_t
> +    datetime_strftime(const struct datetime *date, const char *fmt, char *buf,
> +                      uint32_t len);
> +
> +    void
> +    datetime_now(struct datetime * now);
> +
> +]]


<stripped>


> diff --git a/test/app-tap/datetime.test.lua b/test/app-tap/datetime.test.lua
> new file mode 100755
> index 000000000..464d4bd49
> --- /dev/null
> +++ b/test/app-tap/datetime.test.lua
> @@ -0,0 +1,206 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local test = tap.test("errno")
> +local date = require('datetime')
> +local ffi = require('ffi')
> +
> +
> +test:plan(6)
> +
> +test:test("Simple tests for parser", function(test)
> +    test:plan(2)
> +    test:ok(date("1970-01-01T01:00:00Z") ==
> +            date {year=1970, month=1, day=1, hour=1, minute=0, second=0})
> +    test:ok(date("1970-01-01T02:00:00+02:00") ==
> +            date {year=1970, month=1, day=1, hour=2, minute=0, second=0, tz=120})
> +end)
> +
> +test:test("Multiple tests for parser (with nanoseconds)", function(test)
> +    test:plan(168)
> +    -- borrowed from p5-time-moments/t/180_from_string.t
> +    local tests =
> +    {
> +        { '1970-01-01T00:00:00Z',                       0,           0,    0 },
> +        { '1970-01-01T02:00:00+02:00',                  0,           0,  120 },
> +        { '1970-01-01T01:30:00+01:30',                  0,           0,   90 },
> +        { '1970-01-01T01:00:00+01:00',                  0,           0,   60 },
> +        { '1970-01-01T00:01:00+00:01',                  0,           0,    1 },
> +        { '1970-01-01T00:00:00+00:00',                  0,           0,    0 },
> +        { '1969-12-31T23:59:00-00:01',                  0,           0,   -1 },
> +        { '1969-12-31T23:00:00-01:00',                  0,           0,  -60 },
> +        { '1969-12-31T22:30:00-01:30',                  0,           0,  -90 },
> +        { '1969-12-31T22:00:00-02:00',                  0,           0, -120 },
> +        { '1970-01-01T00:00:00.123456789Z',             0,   123456789,    0 },
> +        { '1970-01-01T00:00:00.12345678Z',              0,   123456780,    0 },
> +        { '1970-01-01T00:00:00.1234567Z',               0,   123456700,    0 },
> +        { '1970-01-01T00:00:00.123456Z',                0,   123456000,    0 },
> +        { '1970-01-01T00:00:00.12345Z',                 0,   123450000,    0 },
> +        { '1970-01-01T00:00:00.1234Z',                  0,   123400000,    0 },
> +        { '1970-01-01T00:00:00.123Z',                   0,   123000000,    0 },
> +        { '1970-01-01T00:00:00.12Z',                    0,   120000000,    0 },
> +        { '1970-01-01T00:00:00.1Z',                     0,   100000000,    0 },
> +        { '1970-01-01T00:00:00.01Z',                    0,    10000000,    0 },
> +        { '1970-01-01T00:00:00.001Z',                   0,     1000000,    0 },
> +        { '1970-01-01T00:00:00.0001Z',                  0,      100000,    0 },
> +        { '1970-01-01T00:00:00.00001Z',                 0,       10000,    0 },
> +        { '1970-01-01T00:00:00.000001Z',                0,        1000,    0 },
> +        { '1970-01-01T00:00:00.0000001Z',               0,         100,    0 },
> +        { '1970-01-01T00:00:00.00000001Z',              0,          10,    0 },
> +        { '1970-01-01T00:00:00.000000001Z',             0,           1,    0 },
> +        { '1970-01-01T00:00:00.000000009Z',             0,           9,    0 },
> +        { '1970-01-01T00:00:00.00000009Z',              0,          90,    0 },
> +        { '1970-01-01T00:00:00.0000009Z',               0,         900,    0 },
> +        { '1970-01-01T00:00:00.000009Z',                0,        9000,    0 },
> +        { '1970-01-01T00:00:00.00009Z',                 0,       90000,    0 },
> +        { '1970-01-01T00:00:00.0009Z',                  0,      900000,    0 },
> +        { '1970-01-01T00:00:00.009Z',                   0,     9000000,    0 },
> +        { '1970-01-01T00:00:00.09Z',                    0,    90000000,    0 },
> +        { '1970-01-01T00:00:00.9Z',                     0,   900000000,    0 },
> +        { '1970-01-01T00:00:00.99Z',                    0,   990000000,    0 },
> +        { '1970-01-01T00:00:00.999Z',                   0,   999000000,    0 },
> +        { '1970-01-01T00:00:00.9999Z',                  0,   999900000,    0 },
> +        { '1970-01-01T00:00:00.99999Z',                 0,   999990000,    0 },
> +        { '1970-01-01T00:00:00.999999Z',                0,   999999000,    0 },
> +        { '1970-01-01T00:00:00.9999999Z',               0,   999999900,    0 },
> +        { '1970-01-01T00:00:00.99999999Z',              0,   999999990,    0 },
> +        { '1970-01-01T00:00:00.999999999Z',             0,   999999999,    0 },

Красивое :)

> +        { '1970-01-01T00:00:00.0Z',                     0,           0,    0 },
> +        { '1970-01-01T00:00:00.00Z',                    0,           0,    0 },
> +        { '1970-01-01T00:00:00.000Z',                   0,           0,    0 },
> +        { '1970-01-01T00:00:00.0000Z',                  0,           0,    0 },
> +        { '1970-01-01T00:00:00.00000Z',                 0,           0,    0 },
> +        { '1970-01-01T00:00:00.000000Z',                0,           0,    0 },
> +        { '1970-01-01T00:00:00.0000000Z',               0,           0,    0 },
> +        { '1970-01-01T00:00:00.00000000Z',              0,           0,    0 },
> +        { '1970-01-01T00:00:00.000000000Z',             0,           0,    0 },
> +        { '1973-11-29T21:33:09Z',               123456789,           0,    0 },
> +        { '2013-10-28T17:51:56Z',              1382982716,           0,    0 },
> +        { '9999-12-31T23:59:59Z',            253402300799,           0,    0 },
> +    }
> +    for _, value in ipairs(tests) do
> +        local str, epoch, nsec, offset
> +        str, epoch, nsec, offset = unpack(value)
> +        local dt = date(str)
> +        test:ok(dt.secs == epoch, ('%s: dt.secs == %d'):format(str, epoch))
> +        test:ok(dt.nsec == nsec, ('%s: dt.nsec == %d'):format(str, nsec))
> +        test:ok(dt.offset == offset, ('%s: dt.offset == %d'):format(str, offset))
> +    end
> +end)
> +
> +ffi.cdef [[
> +    void tzset(void);
> +]]
> +
>


<stripped>


>      
>      

-- 
Serge Petrenko