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 A4BBD6EC55; Fri, 16 Jul 2021 02:03:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A4BBD6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626390209; bh=xm3OK0ioeIwK0hjaZpNpwV3vcaxYwsl3eeEMtFrIkeM=; h=To:Cc:References:In-Reply-To:Date:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=UTLD5Avci0WMad3+rRCho8KgVKo7rVA1XQMp6/JqOuwMZGlzirZy2L8IkSorQr/df LghK1c9XO20qlsDfW3OPMk+P8FtZxWDNrmHvisO7EVz0aVnL6DzF3jjtjRjsIm+Y7+ VdaFOyF6cSus2rLgODFwOvW7EH23SmoUx0hOy5AE= Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 604836EC55 for ; Fri, 16 Jul 2021 02:03:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 604836EC55 Received: by smtp51.i.mail.ru with esmtpa (envelope-from ) id 1m4AO6-0003gg-Iv; Fri, 16 Jul 2021 02:03:27 +0300 To: "'Oleg Babin'" Cc: "TML" References: <5deb9cc2-3a74-9968-0fcc-aa0a1f851fb9@tarantool.org> In-Reply-To: <5deb9cc2-3a74-9968-0fcc-aa0a1f851fb9@tarantool.org> Date: Fri, 16 Jul 2021 02:03:26 +0300 Message-ID: <129501d779cd$9e83bfd0$db8b3f70$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQKFdMLi9L7xndItUtF479+moA8ElwIUIJqiqdftxfA= Content-Language: ru X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C320FB367CC9CBEE80CFFDA1774AEA76E0182A05F53808504072C750792D9B2932242C048190A83DD328F668636C1AC15B5ACD3BDCB2556345 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AD2F2D6F6013FF7FC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7A179494B5629353BEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F21C49E2390F79F168AD496AF3379B10CFCC7F00164DA146DAFE8445B8C89999728AA50765F7900637F6B57BC7E64490618DEB871D839B7333395957E7521B51C2DFABB839C843B9C08941B15DA834481F8AA50765F7900637F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CECD24B97DE0E565D6136E347CC761E074AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3B471455E47B21337BA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE732FCE54C4D9A645443847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5F7F1E4565A84C6784891A1455F602A199365D03389F06201D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3438212F20E1E78D8E2ACC7D7E285A1A6CDB9F4C6734B90720DF66E899C570B5BD19BA4CE251BF28D31D7E09C32AA3244C5A3C4F4932718D245FD2DBD6A3B36EDE60759606DA2E136A729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojSyb42jm8PHK5/PIt4Dvq/Q== X-Mailru-Sender: B5B6A6EBBD94DAD8DA84A184D75F19DBC4D8C17BB12CDB81C1637E68C40C509E482062AC006132DF1EC9E4A2C82A33BC8C24925A86E657CE0C70AEE3C9A96FBAB3D7EE8ED63280BE112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [RFC PATCH 00/13] Initial datetime 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: Timur Safin via Tarantool-patches Reply-To: Timur Safin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks, Oleg, for your fast and valuable feedback!=20 [ You are the fastest hand on the wild west today! ] : From: Tarantool-patches = On : Subject: Re: [Tarantool-patches] [RFC PATCH 00/13] Initial datetime = support :=20 : Thanks for your patch. :=20 :=20 : I'd want to use native datetime data type in one project=20 : that should be released soon. So I'll give you some feedback because = I'm : really interested in this patch. I probably won't perform deep review = and I'll : just give some feedback about an interface. Very good! At this stage of feature development feedback to api created = is=20 the most valuable. : 1. I see you implemented "datetime" module as ffi wrapper over c-dt. :=20 : Why did you choose FFI and could you show that FFI is better than Lua = C API? :=20 : I know there are such benchmark for decimal and they show that FFI was : slower than Lua C API : = (https://github.com/tarantool/tarantool/issues/692#issuecomment-505043929= ). I didn't see these results, that was prior my Tarantool times. Very = interesting! I've asked to LuaJIT woodoo masters for their feedback (it will be very=20 unfortunate to see the necessity to rework whole C interface from such=20 convenient FFI mechanism). But, well, let us check performance here. : 2. Could you please implement a hepler `is_datetime`? Uuid and decimal : have is_* methods. Yes, will do so. :=20 :=20 : 3. Modue throws unhandled error: :=20 : ``` :=20 : tarantool> datetime.now() =3D=3D newproxy() : --- : - error: 'builtin/datetime.lua:232: attempt to index local ''rhs'' (a : userdata value)' : ... :=20 : ``` :=20 : I think you can see how works uuid module : (https://github.com/tarantool/tarantool/blob/master/src/lua/uuid.lua) :=20 : to fix such errors. Yes, careful processing of invalid arguments now is the missing point at the moment. [Will is is_datetime() wherever it's reasonable.] :=20 : 4. Parse function has quite strange output format :=20 : ``` :=20 : tarantool> datetime.parse_date('2016-12-18') : --- : - 2016-12-18T00:00Z : - 10 : ... :=20 : tarantool> datetime.parse_date('2016-13-18') : --- : - null : - 0 : ... :=20 : ``` :=20 :=20 : Do we really need the second value in case of success? :=20 : Do we really need 0 as the second return value in case of failue? : Probably we should extend error message somehow. Good question! I could explain why it behave so strangely - underlying c-dt dt_parse_iso_date(), dt_parse_iso_time() and = dt_parse_iso_timezone() could deal with incorrect suffices, but return the length of properly parsed part of string, thus I return this length as 2nd value in = returned tuple. You could see how it's used in the test/app-tap/datetime.test.lua = cases named "Parse iso date - valid strings" and "Parse iso date -=20 invalid strings". I agreed that it looks strangely, and we may rework=20 a way we signal parse errors here. :=20 :=20 : 5. More strange errors, please improve the validation. :=20 : ``` :=20 : tarantool> datetime.days() : --- : - error: 'builtin/datetime.lua:203: attempt to perform arithmetic on : local ''d'' (a : nil value)' : ... :=20 : tarantool> datetime.ctime() : --- : - error: 'builtin/datetime.lua:550: bad argument #1 to ''typeof'' (C : type expected, : got nil)' : ... :=20 : ``` Yes, will be handled similarly to the cases above (where is_datetime()=20 Would be used for invalid arguments handling). :=20 :=20 : Also I see some assertions in code (e.g. `assert(v >=3D 0 and v < = 61)`). : This yields to :=20 : "assertion failed" message that is not completely clear. Agreed that many assertions there are not user-friendly and ask for some massaging. :=20 : 6. Does this module allows to manipulate with timezones and locales? :=20 : Icu-date that we currently use allows it : (https://github.com/tarantool/icu-date). Not, at the moment. And I do like how it's handled in icu-date. And=20 surprisingly that was exactly original plan - to use icu database=20 for translation of timezone names into their offsets, with memorization of results (exactly as it's done in zone_id_cache in icu-date.lua). Locales is the strongest feature in icu-date, and should be reused=20 properly while handling named-timezones. Will return here :=20 : Also try to use tests from icu-date module. I think it could be = useful. : The best option is to allow drop-in but I'm not sure that it's = possible. Drop-in is hardly possible (because icu-date api looks too much mouthful with all their forms of a kind date:get(icu_date.fields.MILLISECOND) or date:get_attribute(attributes.MINIMAL_DAYS_IN_FIRST_WEEK). But tests = might be of help. (And I assume you meant tap tests, not busted tests). And = while we are here - why there are 2 kinds of tests? (And not only busted used = because it's much more convenient? :) ) : 7. Is it expected? :=20 : ``` :=20 : tarantool> datetime.now() - datetime.now() : --- : - +18823 days, 16 hours, 29 minutes, 44.178836107254 seconds : ... :=20 : ``` :=20 : I believe it should be 0. Not exactly 0, but very small fraction of second. Thanks for reporting - = that Was artifact of last minute rename/refactoring. Here is the patch for = this=20 Problem and similar in + diff --git a/src/lua/datetime.lua b/src/lua/datetime.lua index d28f931f6..57ef80403 100644 --- a/src/lua/datetime.lua +++ b/src/lua/datetime.lua @@ -558,6 +558,17 @@ local function date_first(lhs, rhs) end end =20 +local function _normalize_nsec(secs, nsec) + if nsec < 0 then + secs =3D secs - 1 + nsec =3D nsec + NANOS_PER_SEC + elseif nsec >=3D NANOS_PER_SEC then + secs =3D secs + 1 + nsec =3D nsec - NANOS_PER_SEC + end + return secs, nsec +end + local function datetime_sub(lhs, rhs) check_date(lhs) -- make sure left is date local d, s =3D lhs, rhs @@ -565,18 +576,15 @@ local function datetime_sub(lhs, rhs) -- 1. left is date, right is date or delta if (ffi.typeof(s) =3D=3D datetime_t) or (ffi.typeof(s) =3D=3D = duration_t) then local o + -- if they are both dates then result is delta if ffi.typeof(s) =3D=3D datetime_t then o =3D duration_new() else o =3D datetime_new() end - o.secs =3D d.secs - s.secs - o.nsec =3D d.nsec - s.nsec - if o.nsec < 0 then - o.secs =3D d.secs - 1 - o.nsec =3D d.nsec + NANOS_PER_SEC - end + o.secs, o.nsec =3D _normalize_nsec(d.secs - s.secs, d.nsec - = s.nsec) + return o =20 -- 2. left is date, right is duration in months @@ -602,12 +610,8 @@ local function datetime_add(lhs, rhs) if (ffi.typeof(s) =3D=3D datetime_t) or (ffi.typeof(s) =3D=3D = duration_t) then local o =3D datetime_new() =20 - o.secs =3D d.secs + s.secs - o.nsec =3D d.nsec + s.nsec - if o.nsec >=3D NANOS_PER_SEC then - o.secs =3D d.secs + 1 - o.nsec =3D d.nsec - NANOS_PER_SEC - end + o.secs, o.nsec =3D _normalize_nsec(d.secs + s.secs, d.nsec + = s.nsec) + return o =20 -- 2. left is date, right is duration in months :=20 :=20 : 8. Could you provide a helper that allows to convert unix time to = datetime? Will introduce something, yes, we need one.=20 :=20 : I tried this way but it seems an interval instead of datetime. :=20 : ``` :=20 : tarantool> datetime.seconds(datetime.now().seconds) : --- : - +18823 days, 16 hours, 34 minutes, 10.283248901367 seconds : ... :=20 : ``` :=20 :=20 : 9. Lua output doesn't support datetime :=20 : ``` :=20 : tarantool> \set output yaml : --- : - true : ... :=20 : tarantool> datetime.now() : --- : - 2021-07-15T19:46:15.797333+03:00 : ... :=20 : tarantool> \set output lua : true; : tarantool> datetime.now() : nil; :=20 : ``` Interesting, didn't know of that. Do you have any hints where it's=20 Implemented for, say, decimal, or uuid? :=20 :=20 : 10. You can export also the number of months/days in week as = constansts. :=20 : Probably it could be useful to make something like: :=20 : ``` : datetime_object:set('month', datetime.months.JANUARY) :=20 : ``` Hmm, hmm. Need to discuss with others. Didn't see so much details = exported in python or perl datetime modules. How frequently you needed such=20 functionality from icu-date? :=20 :=20 : It was brief feedback as from (I hope) future customer of this module. :=20 : If I've found another interface issues I'll give you feedback in next : e-mails. :=20 : I didn't see code itself if you need it I can review this patchset in : more detailed way. That would be interesting to hear, thanks in advance! Timur