Re: [Tarantool-patches] [PATCH resend v2 09/11] lua, datetime: time intervals support

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi there!

: From: Oleg Babin <olegrok@tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH resend v2 09/11] lua, datetime: time
: intervals support
: 
: Thanks for your patch.
: 
: I wrote several comments below.
: 
: 
: However it makes me think that such approach will work quite slow since
: 
: all functions is implemented in Lua, all arithmetic is in Lua.

I'm not that pessimistic here :) [Given the fact that I'd remove
Out of a loops all handy handlers table creation] But we need
to measure it first. 
: 
: 
...
: > @@ -158,58 +169,146 @@ local DT_EPOCH_1970_OFFSET = 719163LL
: >
: >   local datetime_t = ffi.typeof('struct datetime_t')
: >   local interval_t = ffi.typeof('struct datetime_interval_t')
: > +ffi.cdef [[
: > +    struct t_interval_months {
: > +        int m;
: > +    };
: > +
: > +    struct t_interval_years {
: > +        int y;
: > +    };
: > +]]
: > +local interval_months_t = ffi.typeof('struct t_interval_months')
: > +local interval_years_t = ffi.typeof('struct t_interval_years')
: > +
: > +local function is_interval(o)
: > +    return ffi.istype(interval_t, o) or
: > +           ffi.istype(interval_months_t, o) or
: > +           ffi.istype(interval_years_t, o)
: > +end
: > +
: 
: It will throw for non-cdata values:
: 
: tarantool> ffi.istype(interval_t, o)
: ---
: - error: 'bad argument #1 to ''?'' (C type expected, got nil)'
: ...
: 
: tarantool> ffi.istype(interval_t, 123)
: ---
: - error: 'bad argument #1 to ''?'' (C type expected, got nil)'
: ...

Yes, thanks for pin-pointing it! I've modified it this way to 
avoid such errors:
------------------------------------------------
@@ -182,15 +181,19 @@ local interval_months_t = ffi.typeof('struct t_interval_months')
 local interval_years_t = ffi.typeof('struct t_interval_years')
 
 local function is_interval(o)
-    return ffi.istype(interval_t, o) or
-           ffi.istype(interval_months_t, o) or
-           ffi.istype(interval_years_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)
-    return ffi.istype(o, datetime_t)
+    return type(o) == 'cdata' and ffi.istype(o, datetime_t)
 end
 
+local function is_date_interval(o)
+    return is_datetime(o) or is_interval(o)
+end
 
 local function interval_new()
     local interval = ffi.new(interval_t)
------------------------------------------------

: > @@ -224,19 +323,123 @@ local function interval_serialize(self)
: >       return { secs = self.secs, nsec = self.nsec }
: >   end
: >
: > +local function local_rd(o)
: > +    return math.floor(tonumber(o.secs / SECS_PER_DAY)) +
: DT_EPOCH_1970_OFFSET
: > +end
: > +
: > +local function local_dt(o)
: > +    return cdt.dt_from_rdn(local_rd(o))
: > +end
: > +
: > +local function _normalize_nsec(secs, nsec)
: > +    if nsec < 0 then
: > +        secs = secs - 1
: > +        nsec = nsec + NANOS_PER_SEC
: > +    elseif nsec >= NANOS_PER_SEC then
: > +        secs = secs + 1
: > +        nsec = nsec - NANOS_PER_SEC
: > +    end
: > +    return secs, nsec
: > +end
: > +
: > +-- addition or subtraction from date/time of a given interval
: > +-- described via table direction should be +1 or -1
: > +local function interval_increment(self, o, direction)
: > +    assert(direction == -1 or direction == 1)
: > +    check_date(self, "interval_increment(date, object, -+1)")
: > +    assert(type(o) == 'table')
: > +
: > +    local ym_updated = false
: > +    local dhms_updated = false
: > +
: > +    local dt = local_dt(self)
: > +    local secs, nsec
: > +    secs, nsec = self.secs, self.nsec
: > +
: > +    for key, value in pairs(o) do
: > +        local handlers = {
: 
: The same as in one previous patch. It's too expensive to recreate table
: and functions for each simple
: 
: action and for each iteration loop.

I've removed creation of a table, which is invariant in the loop, to
outside of loop statement. Let me measure how it much would different
than series of iffs. 

---------------------------------------------------------
@@ -377,52 +382,52 @@ local function interval_increment(self, o, direction)
     local secs, nsec
     secs, nsec = self.secs, self.nsec
 
+    local handlers = {
+        years = function(k, v)
+            check_range(v, {0, 9999}, k)
+            dt = builtin.dt_add_years(dt, direction * v, builtin.DT_LIMIT)
+            ym_updated = true
+        end,
+
+        months = function(k, v)
+            check_range(v, {0, 12}, k)
+            dt = builtin.dt_add_months(dt, direction * v, builtin.DT_LIMIT)
+            ym_updated = true
+        end,
+
+        weeks = function(k, v)
+            check_range(v, {0, 52}, k)
+            secs = secs + direction * 7 * v * SECS_PER_DAY
+            dhms_updated = true
+            dhms_updated = true
+        end,
+
+        days = function(k, v)
+            check_range(v, {0, 31}, k)
+            secs = secs + direction * v * SECS_PER_DAY
+            dhms_updated = true
+        end,
+
+        hours = function(k, v)
+            check_range(v, {0, 23}, k)
+            secs = secs + direction * 60 * 60 * v
+            dhms_updated = true
+        end,
+
+        minutes = function(k, v)
+            check_range(v, {0, 59}, k)
+            secs = secs + direction * 60 * v
+        end,
+
+        seconds = function(k, v)
+            check_range(v, {0, 60}, k)
+            local s, frac = seconds_fraction(v)
+            secs = secs + direction * s
+            nsec = nsec + direction * frac * 1e9 -- convert fraction to nanoseconds
+            dhms_updated = true
+        end,
+    }
     for key, value in pairs(o) do
-        local handlers = {
-            years = function(v)
-                check_range(v, {0, 9999}, key)
-                dt = cdt.dt_add_years(dt, direction * v, cdt.DT_LIMIT)
-                ym_updated = true
-            end,
-
-            months = function(v)
-                check_range(v, {0, 12}, key)
-                dt = cdt.dt_add_months(dt, direction * v, cdt.DT_LIMIT)
-                ym_updated = true
-            end,
-
-            weeks = function(v)
-                check_range(v, {0, 52}, key)
-                secs = secs + direction * 7 * v * SECS_PER_DAY
-                dhms_updated = true
-            end,
-
-            days = function(v)
-                check_range(v, {0, 31}, key)
-                secs = secs + direction * v * SECS_PER_DAY
-                dhms_updated = true
-            end,
-
-            hours = function(v)
-                check_range(v, {0, 23}, key)
-                secs = secs + direction * 60 * 60 * v
-                dhms_updated = true
-            end,
-
-            minutes = function(v)
-                check_range(v, {0, 59}, key)
-                secs = secs + direction * 60 * v
-            end,
-
-            seconds = function(v)
-                check_range(v, {0, 60}, key)
-                local s, frac = seconds_fraction(v)
-                secs = secs + direction * s
-                nsec = nsec + direction * frac * 1e9 -- convert fraction to nanoseconds
-                dhms_updated = true
-            end,
-        }
-        handlers[key](value)
+        handlers[key](key, value)
     end
 
     secs, nsec = _normalize_nsec(secs, nsec)
---------------------------------------------------------

: 
: > +            years = function(v)
: > +                assert(v > 0 and v < 10000)
: > +                dt = cdt.dt_add_years(dt, direction * v, cdt.DT_LIMIT)
: > +                ym_updated = true
: > +            end,
: > +
: > +            months = function(v)
: > +                assert(v > 0 and v < 13 )
: > +                dt = cdt.dt_add_months(dt, direction * v, cdt.DT_LIMIT)
: > +                ym_updated = true
: > +            end,
: > +
: > +            weeks = function(v)
: > +                assert(v > 0 and v < 32)
: > +                secs = secs + direction * 7 * v * SECS_PER_DAY
: > +                dhms_updated = true
: > +            end,
: > +
: > +            days = function(v)
: > +                assert(v > 0 and v < 32)
: > +                secs = secs + direction * v * SECS_PER_DAY
: > +                dhms_updated = true
: > +            end,
: > +
: > +            hours = function(v)
: > +                assert(v >= 0 and v < 24)
: > +                secs = secs + direction * 60 * 60 * v
: > +                dhms_updated = true
: > +            end,
: > +
: > +            minutes = function(v)
: > +                assert(v >= 0 and v < 60)
: > +                secs = secs + direction * 60 * v
: > +            end,
: > +
: > +            seconds = function(v)
: > +                assert(v >= 0 and v < 61)
: > +                local s, frac
: > +                frac = v % 1
: > +                if frac > 0 then
: > +                    s = v - (v % 1)
: > +                else
: > +                    s = v
: > +                end
: > +                secs = secs + direction * s
: > +                nsec = nsec + direction * frac * 1e9 -- convert fraction
: to nanoseconds
: > +                dhms_updated = true
: > +            end,
: > +        }
: > +        handlers[key](value)
: > +    end
: > +
: > +    secs, nsec = _normalize_nsec(secs, nsec)
: > +
: > +    -- .days, .hours, .minutes, .seconds
: > +    if dhms_updated then
: > +        self.secs = secs
: > +        self.nsec = nsec
: > +    end
: > +
: > +    -- .years, .months updated
: > +    if ym_updated then
: > +        self.secs = (cdt.dt_rdn(dt) - DT_EPOCH_1970_OFFSET) *
: SECS_PER_DAY +
: > +                    secs % SECS_PER_DAY
: > +    end
: > +
: > +    return self
: > +end
: > +

: 
: tarantool> return require('datetime').now() + 1
: ---
: - error: '[string "return require(''datetime'').now() + 1"]:1: Usage:
: datetime:__add(interval)'
: ...
: 
: 
: Looks a bit confusing. User doesn't know about metamethods.

Partially agreed. Though I assume that users of this code will 
be educated enough. But in any case, what would be your suggested
error message here? 

Thanks,
Timur