From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E09DA6EC40; Tue, 17 Aug 2021 21:52:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E09DA6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629226351; bh=Hw4VEOqVAcHPGJXabsdwdVKr7kshZ4EEzX9VwHa4rnA=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=C0PIHClU72IPYZufq9QQDn6h30mpkrKk7aWdNZ2/Etps237laI78Pulxq4nSTgPcc U55NRg/lSO1mwKN6ijWsUYG/RaJ4v+QWFEkD3J2ARVaoaDs52XIIQ3yTgPIbisrfUN DIOeZqgrYDzvKvVV4WRZ3sauyQeh+gYEemkTF/n8= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9E63B6EC40 for ; Tue, 17 Aug 2021 21:52:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9E63B6EC40 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1mG4CI-0003a0-D3; Tue, 17 Aug 2021 21:52:26 +0300 Date: Tue, 17 Aug 2021 21:52:25 +0300 To: Timur Safin via Tarantool-patches Message-ID: <20210817185225.qypvxdifvpvr7od2@esperanza> References: <58bf0cbee84cccbe619c225aafa7d471a4c920ca.1629071531.git.tsafin@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <58bf0cbee84cccbe619c225aafa7d471a4c920ca.1629071531.git.tsafin@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD91BCCB18F2C129F87F36E61E9E4584E9D182A05F53808504062A00C0CE4771D743209DB969C23A905F502014B2AA3AA8CB976BDA3A557648D X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE789066434B85BF7C7EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D3A9DC970DD6E2F6EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F228637969575C783D3CE30F3E92C8E063CC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947C53A357CFAD2FB7E1BA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B678BAF5CC69B36DDD76E601842F6C81A1F004C906525384307823802FF610243DF43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C468D16C903838CAB43847C11F186F3C59DAA53EE0834AAEE X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79D0B18DC6AC13D9A1C8482AA70A1EAD1C0 X-C1DE0DAB: 0D63561A33F958A525CF600DF18CF9AEA6AF2D6AA89EEA9DF78354F571630B05D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7567C209D01CC1E34B410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34DFB7A809FB537087BC434FE50D55FE288F5D7AB596D5D234B5C2C804C2FF6770450B76D0A07C9A331D7E09C32AA3244CC955EA23B04C7BF3C5F3C0EB0FCB09D26C2483212766842283B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28tqYYdT3cjDC6w== X-Mailru-Sender: EAC3DA4C40F0802FC8DB1BBF04B7D9F25CE2C5F2F48BE1D92B4E7ECC3686C41BE97D29598A8FEE33F0798C444D2DF93A342CD0BA774DB6A9B195EA2AC6D0D44E902A426E4A0C259E9437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 6/8] lua, datetime: time intervals support X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Cc: v.shpilevoy@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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.