Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "'Oleg Babin'" <olegrok@tarantool.org>, <v.shpilevoy@tarantool.org>
Cc: <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH resend v2 09/11] lua, datetime: time intervals support
Date: Sat, 31 Jul 2021 01:58:52 +0300
Message-ID: <045901d78596$784a68d0$68df3a70$@tarantool.org> (raw)
In-Reply-To: <ff90fb89-1c85-de43-ecb8-b72068127000@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


  reply	other threads:[~2021-07-30 22:58 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-28 10:34 [Tarantool-patches] [PATCH resend v2 00/11] Initial datetime support Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 01/11] build: add Christian Hansen c-dt to the build Timur Safin via Tarantool-patches
2021-07-29 23:40   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-31  9:22     ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 02/11] lua: built-in module datetime Timur Safin via Tarantool-patches
2021-07-29 18:55   ` Oleg Babin via Tarantool-patches
2021-07-30 19:00     ` Timur Safin via Tarantool-patches
2021-07-31  6:29       ` Oleg Babin via Tarantool-patches
2021-07-31 16:51         ` Timur Safin via Tarantool-patches
2021-07-29 23:36   ` Vladislav Shpilevoy via Tarantool-patches
2021-07-30 15:39     ` Timur Safin via Tarantool-patches
2021-08-01 17:01       ` Vladislav Shpilevoy via Tarantool-patches
2021-08-01 20:23         ` Timur Safin via Tarantool-patches
2021-08-04 23:57           ` Vladislav Shpilevoy via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 03/11] lua, datetime: datetime tests Timur Safin via Tarantool-patches
2021-07-29 18:55   ` Oleg Babin via Tarantool-patches
2021-07-30 20:45     ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 04/11] lua, datetime: display datetime Timur Safin via Tarantool-patches
2021-07-29 18:55   ` Oleg Babin via Tarantool-patches
2021-07-30 21:48     ` Timur Safin via Tarantool-patches
2021-07-31  6:29       ` Oleg Babin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 05/11] box, datetime: add messagepack support for datetime Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 06/11] box, datetime: datetime comparison for indices Timur Safin via Tarantool-patches
2021-07-29 18:56   ` Oleg Babin via Tarantool-patches
2021-07-30 22:18     ` Timur Safin via Tarantool-patches
2021-07-31  6:30       ` Oleg Babin via Tarantool-patches
2021-07-31  9:31         ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 07/11] lua, datetime: proper datetime encoding Timur Safin via Tarantool-patches
2021-07-29 18:57   ` Oleg Babin via Tarantool-patches
2021-07-30 22:20     ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 08/11] lua, datetime: calculated attributes for datetimes Timur Safin via Tarantool-patches
2021-07-29 18:57   ` Oleg Babin via Tarantool-patches
2021-07-30 22:30     ` Timur Safin via Tarantool-patches
2021-07-31  6:31       ` Oleg Babin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 09/11] lua, datetime: time intervals support Timur Safin via Tarantool-patches
2021-07-29 18:58   ` Oleg Babin via Tarantool-patches
2021-07-30 22:58     ` Timur Safin via Tarantool-patches [this message]
2021-07-31  6:31       ` Oleg Babin via Tarantool-patches
2021-07-31  9:20         ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 10/11] lua, datetime: unixtime, timestamp setters in datetime.lua Timur Safin via Tarantool-patches
2021-07-29 18:58   ` Oleg Babin via Tarantool-patches
2021-07-30 23:11     ` Timur Safin via Tarantool-patches
2021-07-31  6:31       ` Oleg Babin via Tarantool-patches
2021-07-31  9:54         ` Timur Safin via Tarantool-patches
2021-07-28 10:34 ` [Tarantool-patches] [PATCH resend v2 11/11] datetime: changelog for datetime module Timur Safin via Tarantool-patches
2021-07-29 18:55 ` [Tarantool-patches] [PATCH resend v2 00/11] Initial datetime support Oleg Babin 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='045901d78596$784a68d0$68df3a70$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=tsafin@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git