Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Safin Timur <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime
Date: Wed, 18 Aug 2021 11:56:16 +0300	[thread overview]
Message-ID: <2aeef940-50f3-574b-4e15-8bf2159aa688@tarantool.org> (raw)
In-Reply-To: <85040d30-2a0c-9ba1-5df9-46429ac4a756@tarantool.org>



18.08.2021 02:30, Safin Timur пишет:
> On 17.08.2021 15:15, Serge Petrenko wrote:
>>
>>
>>> - 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.
>
> I'm planning to cheat here - to not put detailed description to docbot 
> request part here, but rather to refer to the externally available 
> documentation I've written in 
> https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043988
>
> Like:
>
> @TarantoolBot document
> Title: Introduced new built-in `datetime` module
>
>
> `datetime` module has been introduced, which allows to parse ISO-8601 
> literals representing timestamps of various formats, and then 
> manipulate with date objects.
>
> Please refer to 
> https://github.com/tarantool/tarantool/discussions/6244 for more 
> detailed description of module API.

That's fine, I guess.

>
>>
>>> ---
>>>   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?
>
> I considered that, but decided that for better observability purposes 
> I want to keep this rename of symbols via `tnt_` prefix closer to the 
> code where we exports those functions (please see `tnt_dt_dow` and 
> others below in the /extra/exports).
>
> [But taking into account that we gonna squash these commits I don't 
> care that much now]
>
>>
>>
>>> 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
>
> These guys renamed here
> |
> V
>>> +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.
>>   */
>
> Much shorter - I like it. Updated.
>
>>
>> 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.
>
> Updated
>
>>
>> And I'd move the "#pragma once" below the license comment.
>> Otherwise it's easily lost. Up to you.
>
> On one side I agreed that it might left unrecognizable for untrained 
> eye. But on other side - the sooner compiler/preprocessor will see, 
> the better :)
>
> [And there is already well established convention to put it at the 
> first line.]
>
> So I've left it on the 1st line.
>
>>
>>> +#include <stdint.h>
>>> +#include <stdbool.h>
>>> +#include <stdio.h>
>>
>> AFAICS you don't need stdio included here.
>
> Indeed!
>
>>
>>> +#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.
>
> I've written some explanation about these magic numbers. Now it's 
> verboser a bit:
> --------------------------------------------------------
> /**
>  * We count dates since so called "Rata Die" date
>  * January 1, 0001, Monday (as Day 1).
>  * But datetime structure keeps seconds since
>  * Unix "Epoch" date:
>  * Unix, January 1, 1970, Thursday
>  *
>  * The difference between Epoch (1970-01-01)
>  * and Rata Die (0001-01-01) is 719163 days.
>  */
>
> #ifndef SECS_PER_DAY
> #define SECS_PER_DAY          86400
> #define DT_EPOCH_1970_OFFSET  719163
> #endif
> --------------------------------------------------------
>
> also, for the MP-related patch, I've added this comment, and defines 
> (might be used in asserts):
>
> --------------------------------------------------------
> /**
>  * c-dt library uses int as type for dt value, which
>  * represents the number of days since Rata Die date.
>  * This implies limits to the number of seconds we
>  * could safely store in our structures and then safely
>  * pass to c-dt functions.
>  *
>  * So supported ranges will be
>  * - for seconds [-185604722870400 .. 185480451417600]
>  * - for dates   [-5879610-06-22T00:00Z .. 5879611-07-11T00:00Z]
>  */
> #define MAX_DT_DAY_VALUE (int64_t)INT_MAX
> #define MIN_DT_DAY_VALUE (int64_t)INT_MIN
> #define SECS_EPOCH_1970_OFFSET     \
>     ((int64_t)DT_EPOCH_1970_OFFSET * SECS_PER_DAY)
> #define MAX_EPOCH_SECS_VALUE    \
>     (MAX_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET)
> #define MIN_EPOCH_SECS_VALUE    \
>     (MIN_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET)
> --------------------------------------------------------
>
>>
>>> +#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.
>
> Changed.
>
>>
>>
>>> +    /** 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.
>
> Done.
>
>>
>>
>>> +/**
>>> + * 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 "*/"
>
> Extracted.
>
>>
>>
>>> +    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.
>>
>
> Have split it into several. Like that
> ------------------------------------
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> index ce579828f..7601421b1 100644
> --- a/src/lua/datetime.lua
> +++ b/src/lua/datetime.lua
> @@ -1,8 +1,6 @@
>  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).
>
> @@ -14,22 +12,27 @@ ffi.cdef [[
>          dt = (secs / 86400) + 719163
>      Where 719163 is an offset of Unix Epoch (1970-01-01) since Rata Die
>      (0001-01-01) in dates.
> +]]
>
> -    */
> +-- dt_core.h definitions
> +ffi.cdef [[
>      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
> +-- dt_parse_iso.h definitions
> +ffi.cdef [[
>      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
> +-- Tarantool functions - datetime.c
> +ffi.cdef [[
>      int
>      datetime_to_string(const struct datetime * date, char *buf, 
> uint32_t len);
>
> @@ -45,7 +48,6 @@ ffi.cdef [[
>
>      void
>      datetime_now(struct datetime * now);
> -
>  ]]
>
>  local builtin = ffi.C
> ------------------------------------
>
>>
>>> +    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>
>>
>>
>>
>
> Here is (was) incremental patch. [Now it's slightly changed, with 
> MP-related defines, but you got the point]:
> ------------------------------------------------------------------
> diff --git a/src/lib/core/datetime.c b/src/lib/core/datetime.c
> index c48295a6f..719a4cd47 100644
> --- a/src/lib/core/datetime.c
> +++ b/src/lib/core/datetime.c
> @@ -1,32 +1,7 @@
>  /*
> - * 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.
> + * SPDX-License-Identifier: BSD-2-Clause
>   *
> - * 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.
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>   */
>
>  #include <string.h>
> diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h
> index 1a8d7e34f..88774110c 100644
> --- a/src/lib/core/datetime.h
> +++ b/src/lib/core/datetime.h
> @@ -1,38 +1,12 @@
>  #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.
> + * SPDX-License-Identifier: BSD-2-Clause
>   *
> - * 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.
> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file.
>   */
>
>  #include <stdint.h>
>  #include <stdbool.h>
> -#include <stdio.h>
>  #include "c-dt/dt.h"
>
>  #if defined(__cplusplus)
> @@ -40,23 +14,34 @@ extern "C"
>  {
>  #endif /* defined(__cplusplus) */
>
> +/**
> + * We count dates since so called "Rata Die" date
> + * January 1, 0001, Monday (as Day 1).
> + * But datetime structure keeps seconds since
> + * Unix "Epoch" date:
> + * Unix, January 1, 1970, Thursday
> + *
> + * The difference between Epoch (1970-01-01)
> + * and Rata Die (0001-01-01) is 719163 days.
> + */
> +
>  #ifndef SECS_PER_DAY
>  #define SECS_PER_DAY          86400
>  #define DT_EPOCH_1970_OFFSET  719163
>  #endif
>
>  /**
> - * Full datetime structure representing moments
> - * since Unix Epoch (1970-01-01).
> - * Time is kept normalized to UTC, time-zone offset
> + * datetime structure keeps number of seconds since
> + * Unix Epoch.
> + * Time is normalized by UTC, so time-zone offset
>   * is informative only.
>   */
>  struct datetime {
> -       /** seconds since epoch */
> +       /** Seconds since Epoch. */
>         double secs;
> -       /** nanoseconds if any */
> -       int32_t nsec;
> -       /** offset in minutes from UTC */
> +       /** Nanoseconds, if any. */
> +       uint32_t nsec;
> +       /** Offset in minutes from UTC. */
>         int32_t offset;
>  };
>
> @@ -64,10 +49,10 @@ struct datetime {
>   * Date/time interval structure
>   */
>  struct datetime_interval {
> -       /** relative seconds delta */
> +       /** Relative seconds delta. */
>         double secs;
> -       /** nanoseconds delta */
> -       int32_t nsec;
> +       /** Nanoseconds delta, if any. */
> +       uint32_t nsec;
>  };
>
>  /**
> ------------------------------------------------------------------
>
> Thanks,
> Timur


Thanks for the changes!

Looks good so far. I'll take a look at the whole series again
once you push the updates to Vladimir's comments

-- 
Serge Petrenko


  reply	other threads:[~2021-08-18  8:56 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-15 23:59 [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Timur Safin via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build Timur Safin via Tarantool-patches
2021-08-17 12:15   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:24     ` Safin Timur via Tarantool-patches
2021-08-18  8:56       ` Serge Petrenko via Tarantool-patches
2021-08-17 15:50   ` Vladimir Davydov via Tarantool-patches
2021-08-18 10:04     ` Safin Timur via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime Timur Safin via Tarantool-patches
2021-08-17 12:15   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:30     ` Safin Timur via Tarantool-patches
2021-08-18  8:56       ` Serge Petrenko via Tarantool-patches [this message]
2021-08-17 16:52   ` Vladimir Davydov via Tarantool-patches
2021-08-17 19:16     ` Vladimir Davydov via Tarantool-patches
2021-08-18 13:38       ` Safin Timur via Tarantool-patches
2021-08-18 10:03     ` Safin Timur via Tarantool-patches
2021-08-18 10:06       ` Safin Timur via Tarantool-patches
2021-08-18 11:45       ` Vladimir Davydov via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 3/8] lua, datetime: display datetime Timur Safin via Tarantool-patches
2021-08-17 12:15   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:32     ` Safin Timur via Tarantool-patches
2021-08-17 17:06   ` Vladimir Davydov via Tarantool-patches
2021-08-18 14:10     ` Safin Timur via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for datetime Timur Safin via Tarantool-patches
2021-08-16  0:20   ` Safin Timur via Tarantool-patches
2021-08-17 12:15     ` Serge Petrenko via Tarantool-patches
2021-08-17 12:16   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:42     ` Safin Timur via Tarantool-patches
2021-08-18  9:01       ` Serge Petrenko via Tarantool-patches
2021-08-17 18:36   ` Vladimir Davydov via Tarantool-patches
2021-08-18 14:27     ` Safin Timur via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 5/8] box, datetime: datetime comparison for indices Timur Safin via Tarantool-patches
2021-08-17 12:16   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:43     ` Safin Timur via Tarantool-patches
2021-08-18  9:03       ` Serge Petrenko via Tarantool-patches
2021-08-17 19:05   ` Vladimir Davydov via Tarantool-patches
2021-08-18 17:18     ` Safin Timur via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 6/8] lua, datetime: time intervals support Timur Safin via Tarantool-patches
2021-08-17 12:16   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:44     ` Safin Timur via Tarantool-patches
2021-08-17 18:52   ` Vladimir Davydov via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 7/8] datetime: perf test for datetime parser Timur Safin via Tarantool-patches
2021-08-17 19:13   ` Vladimir Davydov via Tarantool-patches
2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 8/8] datetime: changelog for datetime module Timur Safin via Tarantool-patches
2021-08-17 12:16   ` Serge Petrenko via Tarantool-patches
2021-08-17 23:44     ` Safin Timur via Tarantool-patches
2021-08-18  9:04       ` Serge Petrenko via Tarantool-patches
2021-08-17 12:15 ` [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Serge Petrenko via Tarantool-patches
     [not found] ` <20210818082222.mofgheciutpipelz@esperanza>
2021-08-18  8:25   ` Vladimir Davydov via Tarantool-patches
2021-08-18 13:24     ` Safin Timur via Tarantool-patches
2021-08-18 14:22       ` Vladimir Davydov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2aeef940-50f3-574b-4e15-8bf2159aa688@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=tsafin@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox