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 4D4286EC55; Thu, 22 Jul 2021 15:54:44 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4D4286EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1626958484; bh=pwpwD1pnQ73DRf8RNpo4z9ves0VhtPRYYOjokGPdnzc=; h=To:References:In-Reply-To:Date:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=lTZwKZC7sRYVHXLGfBHH800KY+wY/4Y+MuI8al5PD4yIBLth/Lu2UVR7q1vkDtXRN ionceY5b7/fqa+UQWrmCznFCT/tJBTjvGDGo/xwRRD1LxPm24pUos/PWOAWz8dJCGj mZYFrFO7GvyQ8EI/P3mWgXX9JT6h8gtVYfmgHwuY= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 8E34C6EC55 for ; Thu, 22 Jul 2021 15:54:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 8E34C6EC55 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1m6YDo-0003Bm-7m; Thu, 22 Jul 2021 15:54:41 +0300 To: "'Igor Munkin'" References: <20210722100133.GL11494@tarantool.org> In-Reply-To: <20210722100133.GL11494@tarantool.org> Date: Thu, 22 Jul 2021 15:54:39 +0300 Message-ID: <281601d77ef8$bc0b3800$3421a800$@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+moA8ElwLj6nPyqdvf2DA= Content-Language: ru X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C386C8E0DDEE7E2465724AB895DD30A549182A05F538085040AB9A62004CC2A82C02570DC9779200B75BB914775020E4753DC02A838C44D856 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE792C68BF9CD4C0E9EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D70459430292EC88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D88F1F7A0EFDBF695FCBC6D8CE3A4A9DB7117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8BAA867293B0326636D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE7B96B19DC4093321EA93887B71B66F2BD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3CE135D2742255B35302FCEF25BFAB345C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637A7EFCB0EB5ACB161EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5FB99135D243DBF40B23738748119001760EF006EA8834764D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7501A9DF589746230F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D342B1F2AD168155B06F31DE4F0414BFC87C85114AD671E205E89ED5980F98B91769B8089838D72E4041D7E09C32AA3244C6CCEFC32BFE53F7685F7F26D5C550DDD3E8609A02908F27183B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojaAaPr+N/4d3JuZJzNOLQhw== X-Mailru-Sender: 9B1A9A1451179B430DCE2B318EA4156279946544F4B4C4338B7688DAF6B0A690D74BED5340B609DC5C2808D6142752370A8ED71B308007E3DC85537438B7E1A423D748DE48713E689437F6177E88F7363CDA0F3B3F5B9367 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 Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, Alexander Turenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hello Igor, Please see some responses (but better see the next iteration of=20 patchset to be sent later) : From: Igor Munkin : Subject: Re: [RFC PATCH 00/13] Initial datetime support :=20 : Timur, :=20 : Thanks for the series! I wonder, why you decided to reinvent several : approaches being already used for quite similar Tarantool types: = decimal : and uuid. Frankly speaking, I expect you introduce datetime the very : *very* similar way. I didn't make the precise review for this series : (this is waste of time), so please consider my major comments below. :=20 : Let's start with top 3 comments to the eye catching problems I've = found : while looking at the patches. :=20 : 1. Tests have to be committed in the same scope the corresponding : changes in Tarantool are introduced. : 2. Docbot request is required for the changes you're introducing: this : is the place where you can leave the description you've added = below. : 3. Please consider approaches used for decimal and uuid: almost every : Oleg's comment came from the bugs fixed for these types. :=20 : As I write above, this is not a precise review, so I don't even start : nitpicking the patches and simply address you to this document[1] and : our dev guide[2]. Please, check whether your next series respects = these : guidelines. :=20 : On 15.07.21, Timur Safin wrote: : > Here is the preliminary RFC version of a patchset, which is = implementing : > datetime lua support in box, their messagepack, yaml, json = serialization : > support, and a set of c and lua tests. : > : > * It's heavily influenced by Sci-Lua lua-time module implementation : > https://github.com/stepelu/lua-time : > e.g. you could find very similar approach for handling of = operations : > with year and months periods (which should be handled differently = than : > usual seconds, days periods). : > : > * But internally it actually uses Christina Hanson' c-dt module : > https://github.com/chansen/c-dt : > (though it has been modified slightly for cleaner integration : > into cmake build process) :=20 : BTW, submodule points to the very strange repo: please use either : vanilla one, or create a fork into Tarantool repositories. At the moment it points to my own fork, because I do not have=20 Permissions, and unable to create new repo in our organization.=20 I'll ask Sasha Turenko for a help in moving this repo. :=20 : > : > This is preliminary version of patchset, with unoptimal patches = divisions : > (like you could see patches with rename refactorings), but, due to = an : amount : > of code in patchset, and limited time have, I send patchset as RFC = today, : > expecting to receive initial feedback for various parts of a series. :=20 : Well, I guess I can stop here for the initial feedback. :=20 : I have no idea, why you decided to introduce a separate tiny module : src/lua/datetime.c just for initializing several constants and a : oneliner for pushing datetime GCcdata on Lua stack. This module is : excess. Please consider the way this is done for uuid and move : everything to src/lua/utils.c that encloses all illegitimate tricks = with : Lua C API. I'll look there.=20 :=20 : > : > Prior plan was to split series into one, which introduces datetime = module : api, : > and another one with datetime messagepack serialization = implementation. : > But here they left as part of a same series. Sorry, for = invonvenience - : I'll : > reshuffle order according to feedback provided. :=20 : [I personally don't know what kind of the feedback you need to = reshuffle : the series...] :=20 : > : > The problem, which was stopping such simple division - that = `datetime.lua` : > module uses `datetime_to_string()` ffi function for stringization of = a : > datetime object. And this function has been introduced as part of : > messagepack/yaml/json serialization support. Yes, = intra-dependendencies : > between parts looks suboptimal, and I'm very open to suggestions how = to : > divide it properly. :=20 : Again, vse uje ukradeno^W pridumano do nas: consider the way this is : solved for uuid (within src/lib/uuid/tt_uuid.[ch]), since you've = chosen : FFI approach for this type (we'll return to this question later). I've seen tt_uuid library, will consider extraction of datetime = stringization Helper somewhere similarly to tt_uuid.=20 :=20 : > : > Module API : > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D : > : > To simplify review of a code in patchset below you could find = preliminary : > documentation of a datetime api module. Eventually we may put it : > as .rst file in documentation repository (though, personally, I'd = prefer : > to have all modules api committed to the same repo as code, but I = digress) :=20 : I guess, I'll leave general comments right here. :=20 : 1. , , can be implemented via FFI by user, : and you simply provide a wrapped. Anyway, it's already implemented, : so OK. : 2. IMHO, you test too many exotic formats and corner cases of creating : datetime object. I doubt somebody will use at least 10% of the : provided functionality (especially in beta). Those are types of standard ISO-8601 date/time formats with few = extensions. : 3. Datetime and interval arithmetics -- this is what bother me a lot. : I believe every average Lua user can parse date into the table and : build the string from this table back. But nobody wants to mess = with : all that hell with leap years arithmetics, leap seconds, etc. Agreed that convenient interval arithmetic is the most important part of datetime module.=20 : 4. Furthermore, I don't understand the following behaviour: : | tarantool> d =3D require('datetime') : | --- : | ... : | : | tarantool> ft =3D require('ffi').typeof : | --- : | - +2 days, 0 hours, 0 minutes, 0 seconds : | ... : | : | tarantool> d.days(2) + d.days(1) : | --- : | - 1970-01-04T00:00Z : | ... : | : | tarantool> ft(d.days(2) + d.days(1)) : | --- : | - ctype : | ... : | : | tarantool> d.days(2) - d.days(1) : | --- : | - error: 'builtin/datetime.lua:554: date/time expected' : | ... : | : I believe it's quite natural to do arithmetics with intervals to : create a new one, isn't it? At the moment there was limitation that at least 1 of 2 arguments passed (either left or right) have to be full datetime object, and only another one could be interval. (Though in __add we could summarize 2 intervals, as you've shown above, but that was oversight in original patchset, which later has been "fixed") Now, while you are mentioning it, I tend to agree that it's ok to summarize or subtract 2 intervals, and I'll relax this restriction.=20 :=20 : > : > Datetime module : > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D : > : > This module allows to parse date/time stamps, manipulate with them, = and : > generate textual representation. Parsing of date constants will = handle : > all ISO-8601 formats, and deal with some extensions. : > : > For internal date representation it uses cdata structures of a form: :=20 : BTW, I agree w/ Oleg and see no motivation why FFI is chosen for : implementation: decimal is implemented via Lua C API, uuid -- via FFI. : Are there any proofs and benchmarks for datetime? Does it violate any : limit mentioned here[1]? Using ffi to call c-dt was an obvious, easiest way to call c-dt. And = taking into account that uuid is still using FFI I conclude there is no strict policy here and ffi is acceptable.=20 Did not understand "Does it violate any limit mentioned here[1]?" what = are you talking about here? Could you please elaborate? :=20 : > : > ```c++ : > struct t_datetime { : > int secs; : > int nsecs; : > int offset; : > }; : > ``` : > : > Where: : > : > - secs is the (signed) number of seconds since epoch = 1970-01-01T00:00Z; : > - nsecs is number of nanoseconds since beginning of day; : > - offset is the timezone offset (in minutes); : > : > `datetime()` ? create datetime object : > ------------------------------------- : > :=20 : Both decimal and uuid modules provides function, but you decided : to name the datetime constructor . I see no reason for such : inconsistency. Will add `new` as entry point. But, WFIW, here we use the same approach as in uuid.lua where uuid() is shortcut for uuid.new() :=20 : > Create date/time object using either string literals, or = initialization : > object. When given string literals it behaves like wrapper around : > parse() method. : > : > When there is initialization object it could create date/time object : > using these attributes below: : > : > | **Easy way** | : | : > = |--------------|---------------------------------------------------------= - : ---------------------------------------| : > | secs | Seconds since epoch : | : > | nsec | Nanoseconds since midnight : | : > | offset | Time-zone offset in minutes : | : > | **YMD part** | : | : > | year | Year in range \[1..9999\] : | : > | month | Month in range \[1..12\] : | : > | day | Day in month in range \[1..31\] : | : > | **HMS part** | : | : > | hour | Hour in range \[0..23\] : | : > | minute | Minute in range \[0..59\] : | : > | second | Seconds in range \[0..60\]. It allows to have = fraction : part in which case it goes to nsec field | : > | tz | Timezone offset (in minutes) for HMS part : | : > : > Example: : > : > ```lua : > : > datetime =3D require `datetime` : > : > d =3D datetime{secs =3D 0, nsec =3D 0, offset =3D 0} : > d =3D datetime(?1970-01-01?) : > d =3D datetime{year =3D 1970, month =3D 1, day =3D 1} : > ``` : > : > `delta()` ? create time duration object : > --------------------------------------- : > : > TBD :=20 : Why delta? Sci-Lua uses period, that is quite accurate term. SQL : provides INTERVAL type. I guess you can choose something from this. Yup, interval is much better name, will use it instead. :=20 : > : > `parse()` ? parse full ISO-8601 string : > -------------------------------------- : > : > Parse full length date/time literal, which may be in ISO-8601 format = of : > any of extended formats supported by `parse_date()`, `parse_time()` = or : > `parse_timezone()` : > : > It deals with date/time string in format : > : > `date ([Tt ] time ([ ] time_zone)? )?` : > : > Where time or `time_zone` parts may be omitted. : > : > Example: : > : > ```lua : > : > datetime =3D require `datetime` : > : > d =3D datetime.parse(`1970-01-01`) : > d =3D datetime.parse(`1970-01-01T00:00:00Z`) : > d =3D datetime.parse(`1970-01-01T02:00:00+02:00`) : > ``` : > : > `parse_date()` ? parse ISO-8601 date literal : > -------------------------------------------- : > : > Parse date string literal, return partial date object which has : > precision of up-to date period of time. : > : > A number of standard ISO-8601 formats supported, plus there are some : > relaxed formats which are of frequently use: : > : > | 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 | : > : > `parse_time()` ? parse ISO-8601 time literal : > -------------------------------------------- : > : > Parse time string literal, return partial date/time object, which : > defines time period inside of single date. : > : > A number of standard ISO-8601 formats supported, plus there are some : > relaxed formats which are of frequently use: : > : > | 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. : > : > `parse_zone()` ? parse ISO-8601 time zone : > ----------------------------------------- : > : > Parse time-zone string literal, return partial date/time object, = which : > defines timezone offset in minutes sing GMT. : > : > A number of standard ISO-8601 formats supported, plus there are some : > relaxed formats which are of frequently use: : > : > | Basic | Extended | : > |-------|----------| : > | Z | N/A | : > | ?hh | N/A | : > | ?hhmm | ?hh:mm | :=20 : AFAIU, are the parts of routine, = right? : Then why do you provide a separate interfaces for these purposes? Because there are some cases when you need to parse parts of full date-time literal, like parsing only time part. I kept them public, similarly to how it was done in the original c-dt. I see not much point to hide them if we already have such building blocks for external consumption. :=20 : > : > `tostring()` ? convert datetime object to string : > ------------------------------------------------ : > : > Return string representation (probably compact if there are some = parts : > missing) of a date-time objects passed : > : > `now()` ? return current date/time : > ---------------------------------- : > : > `now()` returns local date and time object. It will use local = time-zone : > and nanosecond precision. : > : > `strftime()` ? convert date object to string using format : > --------------------------------------------------------- : > : > `strftime()` is the FFI wrapper around strftime() function in LIBC. = It : > supports all the same flags which supports strftime() from host OS. : > : > See : > = = : > for more details. : > : > `asctime()` ? convert date object to string using asctime predefined : format : > = -------------------------------------------------------------------------= - : - : > : > `asctime()` is the FFI wrapper over `asctime_r()` from a host libc. : asctime : > returns string in the form `"Sun Sep 16 01:03:52 1973\\n\\0"` : > : > = : > : > `ctime()` ? convert local time to string using ctime() predefined = format : > = ------------------------------------------------------------------------ : > : > `ctime()` is the FFI wrapper over `ctime_r()` in the host libc. = ctime : > returns string in the form `"Sun Sep 16 01:03:52 1973\\n\\0"` :=20 : This is not quite right, considering your code, so here is the = question: : do you mean ctime(3) or ctime_r(3). ctime_r is _reentrant_ version of ctime, which behave identically. I'll use more careful wording then, to not confuse reader with 2 separate = names. :=20 : > : > = : > : > The difference of `ctime()` and `asctime()` is that former is = returning : > local time zone formatted, while the latter will deal with GMT. : > : > Examples: : > : > ``` : > tarantool> date =3D require 'datetime' : > --- : > ... : > tarantool> T =3D date.now() : > --- : > ... : > tarantool> T : > --- : > - 2021-07-14T01:36:48.554105+03:00 : > ... : > tarantool> date.asctime(T) : > --- : > - 'Tue Jul 13 22:36:48 2021 : > ' : > ... : > tarantool> date.ctime(T) : > --- : > - 'Wed Jul 14 01:36:48 2021 : > ' : > ... : > ``` : > : > Date attribute accessors : > ------------------------ : > : > | | : | : > = |----------------|-------------------------------------------------------= - : ---------| : > | `timestamp` | Calculate timestamp with seconds and nanoseconds = parts : combined | : > | `nanoseconds` | Number of nanoseconds in time part : | : > | `microseconds` | Number of microseconds in time part : | : > | `milliseconds` | Number of milliseconds in time part : | : > | `seconds` | Alias to timestamp : | : > | `minutes` | Number of minutes in time part : | : > | `hours` | Number of hours in time part : | : > | `days` | Number of days in time part : | : > : > ``` : > tarantool> d =3D date{year =3D 1970, month =3D 1, day =3D 1, hour = =3D 0, minute =3D : 10, second=3D10} : > tarantool> d.secs : > --- : > - 610 : > ... : > tarantool> d.nsec : > --- : > - 0 : > ... : > tarantool> d.offset : > --- : > - 0 : > ... : > tarantool> d.nanoseconds : > --- : > - 610000000000 : > ... : > tarantool> d.milliseconds : > --- : > - 610000 : > ... : > tarantool> d.hours : > --- : > - 0.16944444444444 : > ... : > tarantool> d.minutes : > --- : > - 10.166666666667 : > ... : > ``` : > : > Date arithmetic : > --------------- : > :=20 : The most interesting part of the doc by the way. Yeah, agreed. See it soon! :=20 : > TBD : > : > : > https://github.com/tarantool/tarantool/issues/5941 : > https://github.com/tarantool/tarantool/issues/5946 : > = https://github.com/tarantool/tarantool/tree/tsafin/gh-5941-datetime-V2 : > : > : > ... :=20 : [1]: https://github.com/tarantool/tarantool/wiki/Code-review-procedure : [2]: = https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/ : [1]: http://luajit.org/ext_ffi_semantics.html#status :=20 : -- : Best regards, : IM Regards, Timur