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 0E3B16EC40; Tue, 10 Aug 2021 15:20:55 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0E3B16EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628598055; bh=meVZuemBRLkDBQ/gW6cnB5dd+Kh3MVbC0J0eNhmNMM4=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=UL0zZl9nyGH10e9GEvREu+bv6UpBdOKOFy8gLL/LuvQsrmiWRPr7SDn88zzc9Ro4e 5syDF0m5eCO36L6HWM8mA6ai2lbnJzf6Jio70MurSTc2epFCt+apkH4QrPVZkxXiQ9 VpwajU17KNjWx/SgLg9krnt2hHjqzjYVdrzdvqfY= Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 14D106EC40 for ; Tue, 10 Aug 2021 15:20:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 14D106EC40 Received: by smtp36.i.mail.ru with esmtpa (envelope-from ) id 1mDQkW-0005pU-Lj; Tue, 10 Aug 2021 15:20:53 +0300 To: Safin Timur , olegrok@tarantool.org References: <449cb4c7-c738-ed07-8517-61b663ad9f79@tarantool.org> <0931c9db-bd8e-0687-2c96-73fac1aeff7e@tarantool.org> <18971894-830a-6bbd-a87b-83cd6abe5f34@tarantool.org> <3349aff9-a95c-cab5-4610-ca1089c1e762@tarantool.org> Message-ID: <8e779f32-f4e0-f3ac-45c3-c976f32bfd75@tarantool.org> Date: Tue, 10 Aug 2021 15:20:51 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <3349aff9-a95c-cab5-4610-ca1089c1e762@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9736CF3E71F18CE0C3E1D5927724F4AAA182A05F5380850408B4FC0F2478D3665FA4E4B73C9E3B11931E2FCA39BC135825A040BF4C3AF32C0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A4C4638C9DDF45FCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371B0187663A04449C8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85B2AF6D242247159BFEEA81144784718117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18BDFBBEFFF4125B51D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EEB1593CA6EC85F86D28765F5520A300B2D8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE30CABCCA60F52D7EB6136E347CC761E07C4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637BBEA499411984DA1EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B50541C2D0C0994EB233F220942C854A5FA3 X-C1DE0DAB: 0D63561A33F958A5B35E07779F25DF63201DF5AAB55F1CF8BC49FB149B8CA219D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753177526CD55AFC11410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34505665BFD4C707055ABD7049A597A1C6B7688B10F0469305B26937F6C59F1633B7778DE98F2F86821D7E09C32AA3244CBA3AE761A163843A47CE8642F43945EE1E098CBE561D634383B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojGhQhWEp1aB8Uxdl75WomRw== X-Mailru-Sender: 6C3E74F07C41AE94D32402E5012278FA6B0607A032D9615178165634EF3A756602C5CE79F7D8EA6107784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the fixes! On 08.08.2021 19:35, Safin Timur wrote: > Much respect that you've found some time for review, even while being on vacation! Thanks! > > On 08.08.2021 14:26, Vladislav Shpilevoy wrote: >>>>> +local datetime_index_handlers = { >>>>> +    unixtime = function(self) >>>>> +        return self.secs >>>>> +    end, >>>>> + >>>>> +    timestamp = function(self) >>>>> +        return tonumber(self.secs) + self.nsec / 1e9 >>>> >>>> 11. If you are saying the Lua number value range is enough, >>>> why do you store them as 64bit integers in 'self' and >>>> convert to a number only for the external API? >>> >>> May be I misunderstood your sentence, but let me elaborate here. >>> I need seconds and nanoseconds kept separately for their efficient and precise handling, and for passing to c-dt. >>> >>> If we would keep 32-bit integers in seconds then we would be able to handle only dates upto 2038 year, thus we need 64-bit seconds for extended range. >>> >>> OTOH, not whole 64-bit range is practical, and required for expected in real-life datetime range values. It's not a problem that Lua number and int64_t have very different range for precise integer values. Lua number could handle integer values upto 9e15, which is corresponding to ... >> >> I know all that. The question is why do you keep storing cdata 64 bits numbers >> inside of the datetime objects, if you convert them all to Lua numbers before >> return? You could store self.secs as just number. And the other fields too. Lua >> number is double, it does not loose precision for integers < 2^53, which should >> be enough for the ranges you want to support. Correct? > > I keep using 64-bit because the primary code operating with fields is on C-side, and we need Lua number only on rare cases when user asked for composed attribute date.timestamp. Whenever we deal with seconds within arthmetic operations or transfer to c-dt, we need integer C type, larger than 32-bit. It's good fit for int64_t. But it is slower. Notably slower last time I benched, when I also thought integers should be faster than doubles. But cdata 64 bit integers are slower than plain Lua numbers. Perhaps because they involve too much work with metatables for everything. Besides, doubles are larger than 32 bits - they are 53 bits until they start loosing int precision. And it is just enough for you, isn't it? >>>> 19. It can ignore some fields (here it ignores 'year'): >>>> >>>> tarantool> d1 = datetime({secs = 100, year = 1}) >>>> --- >>>> ... >>>> >>>> tarantool> d2 = datetime({secs = 100, year = 2}) >>>> --- >>>> ... >>>> >>>> tarantool> d1 == d2 >>>> --- >>>> - true >>>> ... >>>> >>> >>> Yes, and that's by design. .secs defines 'easy_way' of initializing object via secs/nsec/offset, while .year selects 'ymd' mode. And `easy_way` has precedence. It looks reasonable for me, please, suggest your idea how to handle it even better without much redesign? >> >> You could for instance, by looking at a first key you see in the dictionary, >> decide how will you parse the entire dict. If you see first 'secs', you parse >> only secs, nsecs and offset. Everything else raises an error. If you see >> first 'year', you raise an error for the other non-related keys. >> >> Ignorance of some keys does not look acceptable. >> >> And by the way, does it mean I can't specify a date using year, month, day, >> hours, and secs? Why isn't it valid? > > Here is the tricky part, agreed that this may be confusing. The "easy mode" operates on secs, nsec, offset, the human-readable mode operates on year, month, day, hour, minute, **second**, and tz. > > There is second in human-readable mode, it's rather named differently (i.e. complete word, non-plural). > > The more important question though - how to implement reasonably diagnosing of such mix-in of raw attributes, with human-radable attributes? > > - I tend to believe that we should **not accept them both** in the single function, but rather accept .secs/.nsec/.offset via different entries, like datetime_new_raw vs datetime_new > > ``` > date = require('datetime') > T = date.new{year = 1970, month = 1, day = 1, minute = 0, seconds = 15, tz = 180} > T2 = date.new_raw{secs = 0, nsec = 0, offset = 180} -- will be the same as new(0, 0, 180) > ``` > > In this case we physically prevent mixing of different modes with different allowed attributes in the builder object. > > What do you think? I agree that these should not be allowed to mix. But the names above look strange. They both seem raw. Non-raw is a string. Intuitively. datetime.new_date() datetime.new_stamp() ? Or still you could have one new() and inside of it allow to set all the fields. Like I asked above and like you also showed in your example: T = date.new{year = 1970, month = 1, day = 1, minute = 0, seconds = 15, tz = 180} Because I still don't understand, why can't I specify an exact date with year, day, month, and seconds inside of the day. What if I want the precision does to nsecs? That is, the max precision: year, month, day, nsecs. >>>> 20. Serialization does not seem to be working in the console >>>> despite you tried to implement it above: >>>> >>>> tarantool> d = datetime({secs = 100}) >>>> --- >>>> ... >>>> >>>> tarantool> d >>>> --- >>>> - 'cdata: 0x0103ad9c50' >>>> ... >>> >>> Yup, redefining of __serialize is not enough for cdata object by whatever reason (I didn't look deep). But after the next patch(es) it's start to stringize properly. >> >> But don't you have __serialize implementation in this commit already? >> (I don't remember and didn't see the latest code yet.) > > Yes, I did have __serialize there, but that was working strangely and only started to behave properly (well, more or less) once I've added __tostring in the following commit. I'm not very well informed about expected console behaviour (regarding those 2 meta-methods, at least), so took that for granted. Probably, there is some bug, may be it's expected behaviour for cdata based data structures? Does not matter much. I am not against serialization done in a separate commit. I only don't like when it is added half-way in the second commit and starts working at some point later. I propose to make it atomic. Either the serialization is added here and starts working here and is tested here, or all is done in a separate commit. >>>> 22. Did you really test this function? >>>> >>>> tarantool> dt = require('datetime') >>>> --- >>>> ... >>>> >>>> tarantool> d1 = dt() >>>> --- >>>> ... >>>> >>>> tarantool> d2 - 1 >>>> --- >>>> - error: 'builtin/datetime.lua:487: bad argument #1 to ''typeof'' (C type expected, >>>>       got number)' >>>> ... >>> >>> All kinds of arguments checking implemented in the separate patch about interval arithmetic operations, please find a moment to look into them. >> >> Please, do not introduce broken functions. Make the correct ones right away and >> add tests. The new code does not need to introduce full functionality if it >> can be done in an atomic way, but it must not be visibly broken at least. Here >> it is. > > Ok, here is how it's looking in patchset: in 1 commit we introduce arithmetic support, but not handle well all corner cases (especially there is no support for proper year or month arithmetics). Special types for year, and months date intervals have been added in the next commit, where we handle all corner cases with bogus data. > > We could: > - either squash interval support to the original module; > - or remove arithmetic altogether from original patch creating the module datetime.lua. > > > What is the recommended way? I would leave the intervals in their own commit, in which I would also add the arithmetics. Working correctly right away. >>>>> + >>>>> +local function datetime_from(o) >>>>> +    if o == nil or type(o) == 'table' then >>>>> +        return datetime_new(o) >>>>> +    elseif type(o) == 'string' then >>>>> +        return parse(o) >>>>> +    end >>>>> +end >>>>> + >>>>> +local function local_now() >>>>> +    local p_tv = ffi.new('struct timeval [1]') >>>>> +    local rc = builtin.gettimeofday(p_tv, nil) >>>> >>>> 24. Why can't you use fiber time? It should be enough in >>>> nearly all usecases and is much faster. For real-real time >>>> talking directly to the kernel we have 'clock' Lua module >>>> already. You could check if results of clock functions can >>>> be passed into your module to create 'real' timestamps not >>>> depending on the eventloop. >>> >>> Fiber time, which actually return cached ev_rt_now, looks imprecise and depends on event loop circumstances, though eventually it calls the same clock_gettime(). So fiber-time is looking like a cached timestamp value from [hopefully, non-distant] past. >>> >>> I'd rather prefer to have more realtime values, and clock_realtime*/clock_monotonic* look relevant, they call clock_gettime() for their purposes, which in turn calls the same gettimeofday(). So in this case we call same api, but with 2 extra call layers above. >> >> By using gettimeofday you drag a lot of cdef garbage including the function >> itself and what it returns. Please, don't. Anyway, the standard says struct >> timeval can have more members than the usually used seconds and microseconds: >> >>     https://pubs.opengroup.org/onlinepubs/7908799/xsh/systime.h.html >> >>     The header defines the timeval structure that includes >>     **at least** the following members >> >>> IMHO, not worth extra complexity if we could just simply call gettimeofday(). >> >> Now you added a lot of extra complexity exactly because of this function. > > Once again, this code is in C now, there is no any non-portable timeval definition of ffi to gettimeofday in Lua - everything is in C, which extra complexity you are talking about? I do not understand. Sorry, I didn't see the latest version at the time I wrote this comment.