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

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

On Mon, Aug 16, 2021 at 02:59:36AM +0300, Timur Safin via Tarantool-patches wrote:
> 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_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.
> + */
> +
> +#include <string.h>
> +#include <time.h>
> +
> +#include "trivia/util.h"
> +#include "datetime.h"
> +
> +static int
> +local_dt(int64_t secs)
> +{
> +	return dt_from_rdn((int)(secs / SECS_PER_DAY) + DT_EPOCH_1970_OFFSET);
> +}

I don't understand what this function does. Please add a comment.

> +
> +static struct tm *
> +datetime_to_tm(const struct datetime *date)
> +{
> +	static struct tm tm;
> +
> +	memset(&tm, 0, sizeof(tm));
> +	int64_t secs = date->secs;
> +	dt_to_struct_tm(local_dt(secs), &tm);
> +
> +	int seconds_of_day = (int64_t)date->secs % SECS_PER_DAY;
> +	tm.tm_hour = (seconds_of_day / 3600) % 24;
> +	tm.tm_min = (seconds_of_day / 60) % 60;
> +	tm.tm_sec = seconds_of_day % 60;

To make the code easier for understanding, please define and use
constants here and everywhere else in this patch: HOURS_PER_DAY,
MINUTES_PER_HOUR, NSECS_PER_USEC, etc.

> +
> +	return &tm;
> +}
> +
> +void
> +datetime_now(struct datetime *now)
> +{
> +	struct timeval tv;
> +	gettimeofday(&tv, NULL);
> +	now->secs = tv.tv_sec;
> +	now->nsec = tv.tv_usec * 1000;
> +
> +	time_t now_seconds;
> +	time(&now_seconds);

Can't you use tv.tv_sec here?

> +	struct tm tm;
> +	localtime_r(&now_seconds, &tm);
> +	now->offset = tm.tm_gmtoff / 60;
> +}
> +
> +char *
> +datetime_asctime(const struct datetime *date, char *buf)
> +{
> +	struct tm *p_tm = datetime_to_tm(date);
> +	return asctime_r(p_tm, buf);
> +}
> +
> +char *
> +datetime_ctime(const struct datetime *date, char *buf)
> +{
> +	time_t time = date->secs;
> +	return ctime_r(&time, buf);
> +}
> +
> +size_t
> +datetime_strftime(const struct datetime *date, const char *fmt, char *buf,
> +		  uint32_t len)
> +{
> +	struct tm *p_tm = datetime_to_tm(date);
> +	return strftime(buf, len, fmt, p_tm);
> +}
> 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.
> + */
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#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

I don't understand what this constant stores. Please add a comment.

> +#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;

Please add a comment explaining why you use 'double' instead of
an integer type.

> +	/** nanoseconds if any */
> +	int32_t nsec;

Why 'nsec', but 'secs'? This is inconsistent. Should be 'nsec' and 'sec'
or 'nsecs' and 'secs'.

> +	/** offset in minutes from UTC */
> +	int32_t offset;

Why do you use int32_t instead of int for these two members?

Same comments for the datetime_interval struct.

> +};
> +
> +/**
> + * Date/time interval structure
> + */
> +struct datetime_interval {
> +	/** relative seconds delta */
> +	double secs;
> +	/** nanoseconds delta */
> +	int32_t nsec;
> +};
> +
> +/**
> + * 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);

Extra space after '*'.

Please add comments to all these functions, like you did for
datetime_asctime.

> +
> +#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.
> +
> +    */
> +    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);

The line is too long. Please ensure that all lines are <= 80 characters
long.

> +
> +    // datetime.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);

Extra space after '*'.

> +
> +]]
> +
> +local builtin = ffi.C
> +local math_modf = math.modf
> +
> +local SECS_PER_DAY     = 86400
> +
> +-- c-dt/dt_config.h
> +
> +-- Unix, January 1, 1970, Thursday
> +local DT_EPOCH_1970_OFFSET = 719163
> +
> +
> +local datetime_t = ffi.typeof('struct datetime')
> +local interval_t = ffi.typeof('struct datetime_interval')
> +
> +local function is_interval(o)
> +    return type(o) == 'cdata' and ffi.istype(interval_t, o)

The check for 'cdata' is redundant. ffi.istype alone should be enough
AFAIK.

> +end
> +
> +local function is_datetime(o)
> +    return type(o) == 'cdata' and ffi.istype(datetime_t, o)
> +end
> +
> +local function is_date_interval(o)
> +    return type(o) == 'cdata' and
> +           (ffi.istype(interval_t, o) or ffi.istype(datetime_t, o))
> +end
> +
> +local function interval_new()
> +    local interval = ffi.new(interval_t)
> +    return interval
> +end
> +
> +local function check_date(o, message)
> +    if not is_datetime(o) then
> +        return error(("%s: expected datetime, 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"):
> +                     format(message, s), 2)
> +    end
> +end
> +
> +local function check_range(v, range, txt)
> +    assert(#range == 2)
> +    if v < range[1] or v > range[2] then
> +        error(('value %d of %s is out of allowed range [%d, %d]'):
> +              format(v, txt, range[1], range[2]), 4)
> +    end
> +end
> +
> +local function datetime_cmp(lhs, rhs)
> +    if not is_date_interval(lhs) or
> +       not is_date_interval(rhs) then
> +       return nil

Bad indentation.

> +    end
> +    local sdiff = lhs.secs - rhs.secs
> +    return sdiff ~= 0 and sdiff or (lhs.nsec - rhs.nsec)
> +end
> +
> +local function datetime_eq(lhs, rhs)
> +    local rc = datetime_cmp(lhs, rhs)
> +    return rc ~= nil and rc == 0
> +end
> +
> +local function datetime_lt(lhs, rhs)
> +    local rc = datetime_cmp(lhs, rhs)
> +    return rc == nil and error('incompatible types for comparison', 2) or
> +           rc < 0
> +end
> +
> +local function datetime_le(lhs, rhs)
> +    local rc = datetime_cmp(lhs, rhs)
> +    return rc == nil and error('incompatible types for comparison', 2) or
> +           rc <= 0
> +end
> +
> +local function datetime_serialize(self)
> +    return { secs = self.secs, nsec = self.nsec, offset = self.offset }
> +end
> +
> +local function interval_serialize(self)
> +    return { secs = self.secs, nsec = self.nsec }
> +end
> +
> +local function datetime_new_raw(secs, nsec, offset)
> +    local dt_obj = ffi.new(datetime_t)
> +    dt_obj.secs = secs
> +    dt_obj.nsec = nsec
> +    dt_obj.offset = offset
> +    return dt_obj
> +end
> +
> +local function datetime_new_dt(dt, secs, frac, offset)

What's 'frac'? Please either add a comment or give it a better name.

> +    local epochV = dt ~= nil and (builtin.tnt_dt_rdn(dt) - DT_EPOCH_1970_OFFSET) *
> +                   SECS_PER_DAY or 0

AFAIK we don't use camel-case for naming local variables in Lua code.

> +    local secsV = secs ~= nil and secs or 0

This is equivalent to 'secs = secs or 0'

> +    local fracV = frac ~= nil and frac or 0
> +    local ofsV = offset ~= nil and offset or 0
> +    return datetime_new_raw(epochV + secsV - ofsV * 60, fracV, ofsV)
> +end
> +
> +-- create datetime given attribute values from obj
> +-- in the "easy mode", providing builder with
> +-- .secs, .nsec, .offset
> +local function datetime_new_obj(obj, ...)
> +    if obj == nil or type(obj) ~= 'table' then

type(obj) ~= 'table' implies 'obj' == nil

> +        return datetime_new_raw(obj, ...)
> +    end
> +    local secs = 0
> +    local nsec = 0
> +    local offset = 0
> +
> +    for key, value in pairs(obj) do
> +        if key == 'secs' then
> +            secs = value
> +        elseif key == 'nsec' then
> +            nsec = value
> +        elseif key == 'offset' then
> +            offset = value
> +        else
> +            error(('unknown attribute %s'):format(key), 2)
> +        end

Hmm, do we really need this check? IMO this is less clear and probably
slower than accessing the table directly:

local secs = obj.secs
local nsecs = obj.nsecs
...

I never saw we do anything like this in Lua code.

Same comment for datetime_new and other places where you use pairs()
like this.

> +    end
> +
> +    return datetime_new_raw(secs, nsec, offset)
> +end
> +
> +-- create datetime given attribute values from obj

Bad comment - what's 'obj'?

> +local function datetime_new(obj)
> +    if obj == nil or type(obj) ~= 'table' then
> +        return datetime_new_raw(0, 0, 0)
> +    end
> +    local y = 0
> +    local M = 0
> +    local d = 0
> +    local ymd = false
> +
> +    local h = 0
> +    local m = 0
> +    local s = 0
> +    local frac = 0
> +    local hms = false
> +    local offset = 0
> +
> +    local dt = 0
> +
> +    for key, value in pairs(obj) do
> +        if key == 'year' then
> +            check_range(value, {1, 9999}, key)
> +            y = value
> +            ymd = true
> +        elseif key == 'month' then
> +            check_range(value, {1, 12}, key)
> +            M = value
> +            ymd = true
> +        elseif key == 'day' then
> +            check_range(value, {1, 31}, key)
> +            d = value
> +            ymd = true
> +        elseif key == 'hour' then
> +            check_range(value, {0, 23}, key)
> +            h = value
> +            hms = true
> +        elseif key == 'min' or key == 'minute' then
> +            check_range(value, {0, 59}, key)
> +            m = value
> +            hms = true
> +        elseif key == 'sec' or key == 'second' then

I don't think we should support both 'sec'/'min' and 'second'/'minute'
here. Since you want to be consistent with os.date() output, I would
leave 'sec'/'min' only.

> +            check_range(value, {0, 60}, key)
> +            s, frac = math_modf(value)
> +            frac = frac * 1e9 -- convert fraction to nanoseconds

So 'frac' actually stores nanoseconds. Please rename accordingly.

> +            hms = true
> +        elseif key == 'tz' then
> +        -- tz offset in minutes

Bad indentation.

> +            check_range(value, {0, 720}, key)
> +            offset = value
> +        elseif key == 'isdst' or key == 'wday' or key =='yday' then -- luacheck: ignore 542

Missing space between '==' and 'yday'.

> +            -- ignore unused os.date attributes
> +        else
> +            error(('unknown attribute %s'):format(key), 2)
> +        end
> +    end
> +
> +    -- .year, .month, .day
> +    if ymd then
> +        dt = builtin.tnt_dt_from_ymd(y, M, d)
> +    end
> +
> +    -- .hour, .minute, .second
> +    local secs = 0
> +    if hms then
> +        secs = h * 3600 + m * 60 + s
> +    end
> +
> +    return datetime_new_dt(dt, secs, frac, offset)
> +end
> +
> +--[[
> +    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
> +]]

Please mention in the comment what this function returns.
Same for other 'parse' functions.

> +
> +local function parse_date(str)
> +    check_str("datetime.parse_date()")

check_str(str, ...)

Here and everywhere else.

> +    local dt = ffi.new('dt_t[1]')
> +    local len = builtin.tnt_dt_parse_iso_date(str, #str, dt)
> +    return len > 0 and datetime_new_dt(dt[0]) or nil, tonumber(len)

Is this tonumber() really necessary?

> +end
> +
> +--[[
> +    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.
> +]]
> +local function parse_time(str)
> +    check_str("datetime.parse_time()")
> +    local sp = ffi.new('int[1]')
> +    local fp = ffi.new('int[1]')
> +    local len = builtin.tnt_dt_parse_iso_time(str, #str, sp, fp)
> +    return len > 0 and datetime_new_dt(nil, sp[0], fp[0]) or nil,
> +           tonumber(len)
> +end
> +
> +--[[
> +    Basic    Extended
> +    Z        N/A
> +    +hh      N/A
> +    -hh      N/A
> +    +hhmm    +hh:mm
> +    -hhmm    -hh:mm
> +]]
> +local function parse_zone(str)
> +    check_str("datetime.parse_zone()")
> +    local offset = ffi.new('int[1]')
> +    local len = builtin.tnt_dt_parse_iso_zone_lenient(str, #str, offset)
> +    return len > 0 and datetime_new_dt(nil, nil, nil, offset[0]) or nil,
> +           tonumber(len)
> +end
> +

Extra new line.

> +
> +--[[
> +    aggregated parse functions
> +    assumes to deal with date T time time_zone
> +    at once
> +
> +    date [T] time [ ] time_zone
> +]]
> +local function parse(str)
> +    check_str("datetime.parse()")
> +    local dt = ffi.new('dt_t[1]')
> +    local len = #str
> +    local n = builtin.tnt_dt_parse_iso_date(str, len, dt)
> +    local dt_ = dt[0]
> +    if n == 0 or len == n then
> +        return datetime_new_dt(dt_)
> +    end
> +
> +    str = str:sub(tonumber(n) + 1)
> +
> +    local ch = str:sub(1,1)

Missing space after ','.

> +    if ch:match('[Tt ]') == nil then
> +        return datetime_new_dt(dt_)
> +    end
> +
> +    str = str:sub(2)
> +    len = #str
> +
> +    local sp = ffi.new('int[1]')
> +    local fp = ffi.new('int[1]')
> +    local n = builtin.tnt_dt_parse_iso_time(str, len, sp, fp)
> +    if n == 0 then
> +        return datetime_new_dt(dt_)
> +    end
> +    local sp_ = sp[0]
> +    local fp_ = fp[0]
> +    if len == n then
> +        return datetime_new_dt(dt_, sp_, fp_)
> +    end
> +
> +    str = str:sub(tonumber(n) + 1)
> +
> +    if str:sub(1,1) == ' ' then

Missing space after ','.

> +        str = str:sub(2)
> +    end
> +
> +    len = #str
> +
> +    local offset = ffi.new('int[1]')
> +    n = builtin.tnt_dt_parse_iso_zone_lenient(str, len, offset)
> +    if n == 0 then
> +        return datetime_new_dt(dt_, sp_, fp_)
> +    end
> +    return datetime_new_dt(dt_, sp_, fp_, offset[0])
> +end
> +
> +local function datetime_from(o)

Please add a comment explaining what this function does.

> +    if o == nil or type(o) == 'table' then
> +        return datetime_new(o)
> +    elseif type(o) == 'string' then
> +        return parse(o)
> +    end
> +end
> +
> +local function local_now()

Please add a comment explaining what this function does.

> +    local d = datetime_new_raw(0, 0, 0)
> +    builtin.datetime_now(d)
> +    return d
> +end
> +
> +-- Change the time-zone to the provided target_offset
> +-- Time `.secs`/`.nsec` are always UTC normalized, we need only to
> +-- reattribute object with different `.offset`

It doesn't change the time-zone of the given object. It creates a new
object with the new timezone. Please fix the comment to avoid confusion.

> +local function datetime_to_tz(self, tgt_ofs)
> +    if self.offset == tgt_ofs then
> +        return self
> +    end
> +    if type(tgt_ofs) == 'string' then
> +        local obj = parse_zone(tgt_ofs)
> +        if obj == nil then
> +            error(('%s: invalid time-zone format %s'):format(self, tgt_ofs), 2)
> +        else
> +            tgt_ofs = obj.offset

target_offset. Please don't use confusing abbreviations.

> +        end
> +    end
> +    return datetime_new_raw(self.secs, self.nsec, tgt_ofs)
> +end
> +
> +local function datetime_index(self, key)
> +    if key == 'epoch' or key == 'unixtime' then
> +        return self.secs
> +    elseif key == 'ts' or key == 'timestamp' then
> +        return self.secs + self.nsec / 1e9
> +    elseif key == 'ns' or key == 'nanoseconds' then
> +        return self.secs * 1e9 + self.nsec
> +    elseif key == 'us' or key == 'microseconds' then
> +        return self.secs * 1e6 + self.nsec / 1e3
> +    elseif key == 'ms' or key == 'milliseconds' then
> +        return self.secs * 1e3 + self.nsec / 1e6
> +    elseif key == 's' or key == 'seconds' then
> +        return self.secs + self.nsec / 1e9
> +    elseif key == 'm' or key == 'min' or key == 'minutes' then
> +        return (self.secs + self.nsec / 1e9) / 60
> +    elseif key == 'hr' or key == 'hours' then
> +        return (self.secs + self.nsec / 1e9) / (60 * 60)
> +    elseif key == 'd' or key == 'days' then
> +        return (self.secs + self.nsec / 1e9) / (24 * 60 * 60)

 1. There's so many ways to get the same information, for example m,
    min, minutes. There should be exactly one way.

 2. I'd expect datetime.hour return the number of hours in the current
    day, like os.date(), but it returns the number of hours since the
    Unix epoch instead. This is confusing and useless.

 3. Please document the behavior somewhere in the comments to the code.

 4. These if-else's are inefficient AFAIU.

> +    elseif key == 'to_utc' then
> +        return function(self)
> +            return datetime_to_tz(self, 0)
> +        end
> +    elseif key == 'to_tz' then
> +        return function(self, offset)
> +            return datetime_to_tz(self, offset)
> +        end
> +    else
> +        error(('unknown attribute %s'):format(key), 2)
> +    end
> +end
> +
> +local function datetime_newindex(self, key, value)
> +    if key == 'epoch' or key == 'unixtime' then
> +        self.secs = value
> +        self.nsec, self.offset = 0, 0
> +    elseif key == 'ts' or key == 'timestamp' then
> +        local secs, frac = math_modf(value)
> +        self.secs = secs
> +        self.nsec = frac * 1e9
> +        self.offset = 0

Do we really want the datetime object to be mutable? If so, allowing to
set its value only to the time since the unix epoch doesn't look
particularly user-friendly.

> +    else
> +        error(('assigning to unknown attribute %s'):format(key), 2)
> +    end
> +end
> +
> +-- sizeof("Wed Jun 30 21:49:08 1993\n")
> +local buf_len = 26

What if year > 9999?

> +
> +local function asctime(o)
> +    check_date(o, "datetime:asctime()")
> +    local buf = ffi.new('char[?]', buf_len)

I think it would be more efficient to define the buffer in a global
variable - we don't yield in this code so this should be fine. Also,
please give the buffer an appropriate name that would say that it is
used for datetime formatting.

> +    return ffi.string(builtin.datetime_asctime(o, buf))
> +end
> +
> +local function ctime(o)
> +    check_date(o, "datetime:ctime()")
> +    local buf = ffi.new('char[?]', buf_len)
> +    return ffi.string(builtin.datetime_ctime(o, buf))
> +end
> +
> +local function strftime(fmt, o)
> +    check_date(o, "datetime.strftime()")
> +    local sz = 128
> +    local buff = ffi.new('char[?]', sz)
> +    builtin.datetime_strftime(o, fmt, buff, sz)
> +    return ffi.string(buff)
> +end
> +
> +local datetime_mt = {
> +    __serialize = datetime_serialize,
> +    __eq = datetime_eq,
> +    __lt = datetime_lt,
> +    __le = datetime_le,
> +    __index = datetime_index,
> +    __newindex = datetime_newindex,
> +}
> +
> +local interval_mt = {
> +    __serialize = interval_serialize,
> +    __eq = datetime_eq,
> +    __lt = datetime_lt,
> +    __le = datetime_le,
> +    __index = datetime_index,

Why does datetime_mt has __newindex while interval_mt doesn't?

> +}
> +
> +ffi.metatype(interval_t, interval_mt)
> +ffi.metatype(datetime_t, datetime_mt)
> +
> +return setmetatable(
> +    {
> +        new         = datetime_new,
> +        new_raw     = datetime_new_obj,

I'm not sure, we need to make the 'raw' function public.

> +        interval    = interval_new,

Why would anyone want to create a 0 interval?

> +
> +        parse       = parse,
> +        parse_date  = parse_date,

> +        parse_time  = parse_time,
> +        parse_zone  = parse_zone,

Creating a datetime object without a date sounds confusing.

> +
> +        now         = local_now,
> +        strftime    = strftime,
> +        asctime     = asctime,
> +        ctime       = ctime,
> +
> +        is_datetime = is_datetime,
> +        is_interval = is_interval,

I don't see any point in making these functions public.

> +    }, {
> +        __call = function(self, ...) return datetime_from(...) end
> +    }
> +)
> diff --git a/src/lua/init.c b/src/lua/init.c
> index f9738025d..127e935d7 100644
> --- a/src/lua/init.c
> +++ b/src/lua/init.c
> @@ -129,7 +129,8 @@ extern char strict_lua[],
>  	parse_lua[],
>  	process_lua[],
>  	humanize_lua[],
> -	memprof_lua[]
> +	memprof_lua[],
> +	datetime_lua[]
>  ;
>  
>  static const char *lua_modules[] = {
> @@ -184,6 +185,7 @@ static const char *lua_modules[] = {
>  	"memprof.process", process_lua,
>  	"memprof.humanize", humanize_lua,
>  	"memprof", memprof_lua,
> +	"datetime", datetime_lua,
>  	NULL
>  };
>  
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index c71cd4857..2c89326f3 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -48,6 +48,9 @@ static uint32_t CTID_STRUCT_IBUF_PTR;
>  uint32_t CTID_CHAR_PTR;
>  uint32_t CTID_CONST_CHAR_PTR;
>  uint32_t CTID_UUID;
> +uint32_t CTID_DATETIME = 0;
> +uint32_t CTID_INTERVAL = 0;

Should be called CTID_DATETIME_INTERVAL.  

Anyway, it's never used. Please remove.

> +
>  
>  void *
>  luaL_pushcdata(struct lua_State *L, uint32_t ctypeid)
> @@ -120,6 +123,12 @@ luaL_pushuuidstr(struct lua_State *L, const struct tt_uuid *uuid)
>  	lua_pushlstring(L, str, UUID_STR_LEN);
>  }
>  
> +struct datetime *
> +luaL_pushdatetime(struct lua_State *L)
> +{
> +	return luaL_pushcdata(L, CTID_DATETIME);
> +}
> +

This function isn't used in this patch. Please move it to patch 4.