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 F174D6EC40; Tue, 17 Aug 2021 15:15:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F174D6EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629202510; bh=ojbKHeYQZOZQ58Rm02gVeb4GAuK1cswG2yOWGaFlvYk=; 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=qmDFuXv4JihOmNN5fmQzFkaMtZxy0vJOKxIMU0mOoIVJZZPjzImIDU0jgYkakijyL bJ7Ah57IHm/oLmwAvBWsNk07mvldZOVKW0QJGjUsvchmYFwWBvr/eHRUYr4wqGTP6C I6R/pBlIordaodneE63/RR3ipwUtIRXXSmVK2cEk= Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (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 6BA726EC40 for ; Tue, 17 Aug 2021 15:15:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6BA726EC40 Received: by smtp53.i.mail.ru with esmtpa (envelope-from ) id 1mFxzm-0003J0-6t; Tue, 17 Aug 2021 15:15:06 +0300 To: Timur Safin References: Message-ID: <65b51a68-cdc9-2333-09c3-56c9e6be2411@tarantool.org> Date: Tue, 17 Aug 2021 15:15:05 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9ECFD080E047A606F6525B29142351271182A05F538085040BCF49197A9AE514667364B358398F2128B102E4B2D9A503D0D9FC14D6E10B22C X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE70A10A23A3B64B805EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063725D748B084CAA27D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8C369E082E10AC99A354747DA4346C2F4117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B67ECBC18655D52CDF089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A539875893BE930736E085A6A68F140693FB2E9F49C0824414D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7567C209D01CC1E34B410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34B9F55CA4D2956E30C71193966235C00A363C003D305FF5C5AFA398023A0858681979A2C1C799B6FE1D7E09C32AA3244CC3083C5FBA29904B9C8CC9276A802636BBA718C7E6A9E04283B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28tr07Rjsdr5QyQ== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A446A6FDA86844F6A5F9BF3A5936BB70A3D889E23A9B5BA4347B424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 16.08.2021 02:59, Timur Safin via Tarantool-patches пишет: > * Version #5 changes: > > - After performance evaluations it was decided to switch data type used > by `datetime.secs` from `int64_t` (which is boxed in LuaJIT) to > `double` (which is unboxed number). > - please see perf/datetime*.cc perf test as a benchmark we used for > comparison of 2 veriants of C implementation. Results are here - > https://gist.github.com/tsafin/618fbf847d258f6e7f5a75fdf9ea945b Hi! Thanks for the patchset! Generally looks ok. Most of my comments are style related, except the ones covering patches 4, 5. I like the amount of tests you introduce. > * Version #4 changes: > > A lot. > > - Got rid of closures usage here and there. Benchmarks revealed there is > 20x speed difference of hash with closure handlers to the case when we > check keys using sequence of simple ifs. > And despite the fact that it looks ugler, we have updated all places which > used to use closures - so much performance impact does not worth it; > > - c-dt library now is accompanied with corresponding unit-tests; > > - due to a new (old) way of exporting public symbols, which is now used by > Tarantool build, we have modified c-dt sources to use approach similar > to that used by xxhash (or ICU) - "namespaces" for public symbols. > If there is DT_NAMESPACE macro (e.g. DT_NAMESPACE=tnt_) defined at the > moment of compilation we rename all relevant public symbols to > use that "namespace" as prefix > > #define dt_from_rdn DT_NAME(DT_NAMESPACE, dt_from_rdn) > > C sources continue to use original name (now as a macro), but in Lua ffi > we have to use renamed symbol - `tnt_dt_from_rdn`. > > - We have separated code which implement MessagePack related funcionality > from generic (i.e. stringization) support for datetime types. > Stringization is in `src/core/datetime.c`, but MessagePack support is in > `src/core/mp_datetime.c`. > > - `ctime` and `asctime` now use reentrant versions (i.e. `ctime_r` instead of > `ctime`, and `asctime_r` instead of `asctime`). This allows us to avoid > nasty problem with GC where it was possible to corrupt returned > static buffer regardless the fact that it was immediately passed > to `ffi.string`. > `test/app-tap/gh-5632-6050-6259-gc-buf-reuse.test.lua` updated to check our > new api `datetime.ctime`, and `datetime.asctime`. > > - Introduced MessagePack unit-test which check C implementation (mp > encode/decode, mp_snprint, mp_fprint) > > - Some special efforts have been taken to make sure we deal correctly > with datetime stamps which were before Unix Epoch (1970-01-01), i.e. > for negative stamps or timezones against epoch. > > - And last, but not the least: in addition to usual getters like > `timestamp`, `nanoseconds`, `epoch`, we have introduced function > modifiers `to_utc()` or `to_tz(offset)` which allow to manipulate with > datetime timezone `.offset` field. Actually it does not do much now > (only reattributing object with different offset, but not changed > actual timestamp, which is UTC normalized). But different timestamp > modifies textual representation, which simplify debugging. > > * Version #3 changes: > > - renamed `struct datetime_t` to `struct datetime`, and `struct > datetime_interval_t` to `struct datetime_interval`; > - significantly reworked arguments checks in module api entries; > - fixed datetime comparisons; > - changed hints calculation to take into account fractional part; > - provided more comments here and there; > > NB! There are MacOSX problems due to GLIBC specific code used (as Vlad > has already pointed out) - so additional patch, making it more > cross-compatible coming here soon... > > * Version #2 changes: > > - fixed problem with overloaded '-' and '+' operations for datetime > arguments; > - fixed messagepack serialization problems; > - heavily documented MessagePack serialization schema in the code; > - introduced working implementation of datetime hints for storage engines; > - made interval related names be more consistent, renamed durations and period > to intervals, i.e. t_datetime_duration to datetime_interval_t, > duration_* to interval_*, period to interval; > - properly implemented all reasonable cases of datetime+interval > arithmetic; > - moved all initialization code to utils.c; > - renamed core/mp_datetime.c to core/datetime.c because it makes more > sense now; > > * Version #1 - initial RFC > > In brief > -------- > > This patchset implements datetime lua support in box, with serialization > to messagepack, yaml, json and lua mode. Also it contains storage > engines' indices implementation for datetime type introduced. > > * Current implementation is heavily influenced by Sci-Lua lua-time module > https://github.com/stepelu/lua-time > e.g. you could find very similar approach for handling of operations > with year or month long intervals (which should be handled differently than > usual intervals of seconds, or days). > > * But internally we actually use Christian Hanson' c-dt module > https://github.com/chansen/c-dt > (though it has been modified slightly for cleaner integration > into our cmake-based build process). > > > Datetime Module API > ------------------- > > Small draft of documentation for our `datetime` module API has been > extracted to the discussion topic there - > https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043988 > We try to update documentation there once any relevant change introdcued > to the implementaton. > > Messagepack serialization schema > -------------------------------- > > In short it looks like: > - now we introduce new MP_EXT extension type #4; > - we may save 1 required and 2 optional fields for datetime field; > > In all gory details it's explained in MessagePack serialization schema depicted here: > https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043990 > > > https://github.com/tarantool/tarantool/issues/5941 > https://github.com/tarantool/tarantool/issues/5946 > > https://github.com/tarantool/tarantool/tree/tsafin/gh-5941-datetime-v4 > > Timur Safin (8): > build: add Christian Hansen c-dt to the build > lua: built-in module datetime > lua, datetime: display datetime > box, datetime: messagepack support for datetime > box, datetime: datetime comparison for indices > lua, datetime: time intervals support > datetime: perf test for datetime parser > datetime: changelog for datetime module > > .gitmodules | 3 + > CMakeLists.txt | 8 + > .../gh-5941-datetime-type-support.md | 4 + > cmake/BuildCDT.cmake | 10 + > extra/exports | 33 + > perf/CMakeLists.txt | 3 + > perf/datetime-common.h | 105 +++ > perf/datetime-compare.cc | 213 +++++ > perf/datetime-parser.cc | 105 +++ > src/CMakeLists.txt | 5 +- > src/box/field_def.c | 53 +- > src/box/field_def.h | 4 + > src/box/lua/serialize_lua.c | 7 +- > src/box/memtx_space.c | 3 +- > src/box/msgpack.c | 7 +- > src/box/tuple_compare.cc | 77 ++ > src/box/vinyl.c | 3 +- > src/lib/core/CMakeLists.txt | 5 +- > src/lib/core/datetime.c | 176 ++++ > src/lib/core/datetime.h | 115 +++ > src/lib/core/mp_datetime.c | 189 ++++ > src/lib/core/mp_datetime.h | 89 ++ > src/lib/core/mp_extension_types.h | 1 + > src/lib/mpstream/mpstream.c | 11 + > src/lib/mpstream/mpstream.h | 4 + > src/lua/datetime.lua | 880 ++++++++++++++++++ > src/lua/init.c | 4 +- > src/lua/msgpack.c | 12 + > src/lua/msgpackffi.lua | 18 + > src/lua/serializer.c | 4 + > src/lua/serializer.h | 2 + > src/lua/utils.c | 28 +- > src/lua/utils.h | 12 + > test/app-tap/datetime.test.lua | 370 ++++++++ > .../gh-5632-6050-6259-gc-buf-reuse.test.lua | 74 +- > test/engine/datetime.result | 77 ++ > test/engine/datetime.test.lua | 35 + > test/unit/CMakeLists.txt | 3 +- > test/unit/datetime.c | 381 ++++++++ > test/unit/datetime.result | 471 ++++++++++ > third_party/c-dt | 1 + > third_party/lua-cjson/lua_cjson.c | 8 + > third_party/lua-yaml/lyaml.cc | 6 +- > 43 files changed, 3591 insertions(+), 28 deletions(-) > create mode 100644 changelogs/unreleased/gh-5941-datetime-type-support.md > create mode 100644 cmake/BuildCDT.cmake > create mode 100644 perf/datetime-common.h > create mode 100644 perf/datetime-compare.cc > create mode 100644 perf/datetime-parser.cc > create mode 100644 src/lib/core/datetime.c > create mode 100644 src/lib/core/datetime.h > create mode 100644 src/lib/core/mp_datetime.c > create mode 100644 src/lib/core/mp_datetime.h > create mode 100644 src/lua/datetime.lua > create mode 100755 test/app-tap/datetime.test.lua > create mode 100644 test/engine/datetime.result > create mode 100644 test/engine/datetime.test.lua > create mode 100644 test/unit/datetime.c > create mode 100644 test/unit/datetime.result > create mode 160000 third_party/c-dt > -- Serge Petrenko