Re: [Tarantool-patches] [PATCH v5 6/8] lua, datetime: time intervals support

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Mon, Aug 16, 2021 at 02:59:40AM +0300, Timur Safin via Tarantool-patches wrote:
> diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua
> index 4d946f194..5fd0565ac 100644
> --- a/src/lua/datetime.lua
> +++ b/src/lua/datetime.lua
> @@ -62,8 +75,23 @@ local DT_EPOCH_1970_OFFSET = 719163
>  local datetime_t = ffi.typeof('struct datetime')
>  local interval_t = ffi.typeof('struct datetime_interval')
>  
> +ffi.cdef [[
> +    struct interval_months {
> +        int m;
> +    };
> +
> +    struct interval_years {
> +        int y;
> +    };
> +]]
> +local interval_months_t = ffi.typeof('struct interval_months')
> +local interval_years_t = ffi.typeof('struct interval_years')

I disagree that this is required. interval_years and interval_months are
very vague notions - they depend on the date they are applied to.
Besides, supporting them complicates the code. I think that a time
interval must be exact. At the same time, I agree it may be convenient
to add a few years or months to a date. Why not add datetime methods
(add_years, add_months) instead of introducing two new kinds of
intervals?

> +
>  local function is_interval(o)
> -    return type(o) == 'cdata' and ffi.istype(interval_t, o)
> +    return type(o) == 'cdata' and
> +           (ffi.istype(interval_t, o) or
> +            ffi.istype(interval_months_t, o) or
> +            ffi.istype(interval_years_t, o))
>  end
>  
>  local function is_datetime(o)
> @@ -72,7 +100,10 @@ end
>  
>  local function is_date_interval(o)
>      return type(o) == 'cdata' and
> -           (ffi.istype(interval_t, o) or ffi.istype(datetime_t, o))
> +           (ffi.istype(datetime_t, o) or
> +            ffi.istype(interval_t, o) or
> +            ffi.istype(interval_months_t, o) or
> +            ffi.istype(interval_years_t, o))
>  end
>  
>  local function interval_new()
> @@ -80,6 +111,13 @@ local function interval_new()
>      return interval
>  end
>  
> +local function check_number(n, message)
> +    if type(n) ~= 'number' then
> +        return error(("%s: expected number, but received %s"):
> +                     format(message, n), 2)
> +    end
> +end
> +
>  local function check_date(o, message)
>      if not is_datetime(o) then
>          return error(("%s: expected datetime, but received %s"):
> @@ -87,6 +125,20 @@ local function check_date(o, message)
>      end
>  end
>  
> +local function check_date_interval(o, message)
> +    if not is_datetime(o) and not is_interval(o) then
> +        return error(("%s: expected datetime or interval, but received %s"):
> +                     format(message, o), 2)
> +    end
> +end
> +
> +local function check_interval(o, message)
> +    if not is_interval(o) then
> +        return error(("%s: expected interval, but received %s"):
> +                     format(message, o), 2)
> +    end
> +end
> +
>  local function check_str(s, message)
>      if not type(s) == 'string' then
>          return error(("%s: expected string, but received %s"):
> @@ -102,6 +154,77 @@ local function check_range(v, range, txt)
>      end
>  end
>  
> +local function interval_years_new(y)
> +    check_number(y, "years(number)")
> +    local o = ffi.new(interval_years_t)
> +    o.y = y
> +    return o
> +end
> +
> +local function interval_months_new(m)
> +    check_number(m, "months(number)")
> +    local o = ffi.new(interval_months_t)
> +    o.m = m
> +    return o
> +end
> +
> +local function interval_weeks_new(w)
> +    check_number(w, "weeks(number)")
> +    local o = ffi.new(interval_t)
> +    o.secs = w * SECS_PER_DAY * 7
> +    return o
> +end
> +
> +local function interval_days_new(d)
> +    check_number(d, "days(number)")
> +    local o = ffi.new(interval_t)
> +    o.secs = d * SECS_PER_DAY
> +    return o
> +end
> +
> +local function interval_hours_new(h)
> +    check_number(h, "hours(number)")
> +    local o = ffi.new(interval_t)
> +    o.secs = h * 60 * 60
> +    return o
> +end
> +
> +local function interval_minutes_new(m)
> +    check_number(m, "minutes(number)")
> +    local o = ffi.new(interval_t)
> +    o.secs = m * 60
> +    return o
> +end
> +
> +local function interval_seconds_new(s)
> +    check_number(s, "seconds(number)")
> +    local o = ffi.new(interval_t)
> +    o.nsec = s % 1 * 1e9
> +    o.secs = s - (s % 1)
> +    return o
> +end

These functions should have been added by the patch that introduced the
datetime library to Lua, because without them the library doesn't seem
to be complete. Doing this in a separate patch doesn't ease review - in
fact it only confuses me as a reviewer, because now I have to jump
between two patches to see the whole picture. In general, please try to
split a patch set in such a way that each patch may be applied before
the rest without breaking anything, introducing dead code or a
half-working feature.

IMO the series should be split as follows:

 1. Add datetime library to Lua + unit tests + Lua tests.
 2. Add datetime serialization to msgpack/yaml/json + Lua tests.
 3. Add datetime indexing support + Lua tests.