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 639446EC40; Wed, 18 Aug 2021 02:42:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 639446EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629243741; bh=Slo4KBs5//30TDeVAG7yAKaEQvBTAzvpgXCBsrkKI2Y=; 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=th4azxsBjz6DvlZd5KpUfwmdV3c8Xraf9TQHUI8SaIoOYlY8+XTgBefgyJ3GJBg6B jQLXyfYt/3D2Uy6e5nVN0laPvQTCGavDzp7RRZdsXkF0RKCvbgAoIB0NjhYiLfIK3v L4qiBEnZQaXt3Ezt5+YX5figznIvyfMSHZMLRMUc= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 486976EC40 for ; Wed, 18 Aug 2021 02:42:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 486976EC40 Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1mG8io-0006hX-H5; Wed, 18 Aug 2021 02:42:19 +0300 To: Serge Petrenko References: <5e5afbb7249c1dfa56f2695fc6a7140be98fa39d.1629071531.git.tsafin@tarantool.org> Message-ID: Date: Wed, 18 Aug 2021 02:42:06 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; 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-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9ECFD080E047A606F6525B29142351271182A05F5380850401A81D988940A1670BA12E625ABEB0BBF8D6C580506A6FF4F9C1D17B87892506F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE711269A7C2F827F16EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006377BAB68A65B44F13B8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F329C89981E6D33E53876223E4EA09A4117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FCE3B0F63F495E1F96D81D268191BDAD3DBD4B6F7A4D31EC0BEA7A3FFF5B025636D81D268191BDAD3D78DA827A17800CE7601EFBC916B793DFCD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D635BA3ABDB36C18089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458F0AFF96BAACF4158235E5A14AD4A4A4625E192CAD1D9E79D0B18DC6AC13D9A1CAB02C85BA2DF2BC3 X-C1DE0DAB: 0D63561A33F958A50B873FFE2AA2B14409456DCDF9971901BB88AC1CA304F0E4D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757E10A58996CBD514410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341E08D7EE1804B7637FBFDE08FA13DF66DF7A765B68FE79A83E9C792C5EDDB766CF8682105AA29D3C1D7E09C32AA3244CADC4B428B16C8AD4BCF64B08CB9543B205AB220A9D022EBC83B48618A63566E0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28tpavUbhjsq5Gg== X-Mailru-Sender: B5B6A6EBBD94DAD8C64AB2DDE1BF89F464E26962EA0CFC1412313857FB993096AD895ABCA0F1229D5C2808D6142752370A8ED71B308007E3DC85537438B7E1A423D748DE48713E689437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for 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: Safin Timur via Tarantool-patches Reply-To: Safin Timur Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks Sergey for your feedback, below you'll see few comments and incremental patch... On 17.08.2021 15:16, Serge Petrenko wrote: > > > 16.08.2021 02:59, Timur Safin via Tarantool-patches пишет: >> Serialize datetime_t as newly introduced MP_EXT type. >> It saves 1 required integer field and upto 2 optional >> unsigned fields in very compact fashion. >> - secs is required field; >> - but nsec, offset are both optional; >> >> * json, yaml serialization formats, lua output mode >>    supported; >> * exported symbols for datetime messagepack size calculations >>    so they are available for usage on Lua side. >> >> Part of #5941 >> Part of #5946 >> --- >>   extra/exports                     |   5 +- >>   src/box/field_def.c               |  35 +++--- >>   src/box/field_def.h               |   1 + >>   src/box/lua/serialize_lua.c       |   7 +- >>   src/box/msgpack.c                 |   7 +- >>   src/box/tuple_compare.cc          |  20 ++++ >>   src/lib/core/CMakeLists.txt       |   4 +- >>   src/lib/core/datetime.c           |   9 ++ >>   src/lib/core/datetime.h           |  11 ++ >>   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/msgpack.c                 |  12 ++ >>   src/lua/msgpackffi.lua            |  18 +++ >>   src/lua/serializer.c              |   4 + >>   src/lua/serializer.h              |   2 + >>   src/lua/utils.c                   |   1 - >>   test/unit/datetime.c              | 125 +++++++++++++++++++- >>   test/unit/datetime.result         | 115 +++++++++++++++++- >>   third_party/lua-cjson/lua_cjson.c |   8 ++ >>   third_party/lua-yaml/lyaml.cc     |   6 +- >>   23 files changed, 661 insertions(+), 23 deletions(-) >>   create mode 100644 src/lib/core/mp_datetime.c >>   create mode 100644 src/lib/core/mp_datetime.h >> >> diff --git a/extra/exports b/extra/exports >> index 2437e175c..c34a5c2b5 100644 >> --- a/extra/exports >> +++ b/extra/exports >> @@ -151,9 +151,10 @@ csv_setopt >>   datetime_asctime >>   datetime_ctime >>   datetime_now >> +datetime_pack >>   datetime_strftime >>   datetime_to_string >> -decimal_unpack >> +datetime_unpack > > > decimal_unpack should stay there. It's there, but 2 lines below :) That was me copy-pasted decimal_unpack a few patches before, but has not changed it to datetime_unpack. I've corrected it in this patch. I've now corrected the original appearance, now with correct name. > > >>   decimal_from_string >>   decimal_unpack ^^^ it was here >>   tnt_dt_dow >> @@ -397,6 +398,7 @@ mp_decode_uint >>   mp_encode_array >>   mp_encode_bin >>   mp_encode_bool >> +mp_encode_datetime >>   mp_encode_decimal >>   mp_encode_double >>   mp_encode_float >> @@ -413,6 +415,7 @@ mp_next >>   mp_next_slowpath >>   mp_parser_hint >>   mp_sizeof_array >> +mp_sizeof_datetime >>   mp_sizeof_decimal >>   mp_sizeof_str >>   mp_sizeof_uuid >> diff --git a/src/box/field_def.c b/src/box/field_def.c >> index 51acb8025..2682a42ee 100644 >> --- a/src/box/field_def.c >> +++ b/src/box/field_def.c >> @@ -72,6 +72,7 @@ const uint32_t field_mp_type[] = { >>       /* [FIELD_TYPE_UUID]     =  */ 0, /* only MP_UUID is supported */ >>       /* [FIELD_TYPE_ARRAY]    =  */ 1U << MP_ARRAY, >>       /* [FIELD_TYPE_MAP]      =  */ (1U << MP_MAP), >> +    /* [FIELD_TYPE_DATETIME] =  */ 0, /* only MP_DATETIME is >> supported */ >>   }; >>   const uint32_t field_ext_type[] = { >> @@ -83,11 +84,13 @@ const uint32_t field_ext_type[] = { >>       /* [FIELD_TYPE_INTEGER]   = */ 0, >>       /* [FIELD_TYPE_BOOLEAN]   = */ 0, >>       /* [FIELD_TYPE_VARBINARY] = */ 0, >> -    /* [FIELD_TYPE_SCALAR]    = */ (1U << MP_DECIMAL) | (1U << MP_UUID), >> +    /* [FIELD_TYPE_SCALAR]    = */ (1U << MP_DECIMAL) | (1U << >> MP_UUID) | >> +        (1U << MP_DATETIME), >>       /* [FIELD_TYPE_DECIMAL]   = */ 1U << MP_DECIMAL, >>       /* [FIELD_TYPE_UUID]      = */ 1U << MP_UUID, >>       /* [FIELD_TYPE_ARRAY]     = */ 0, >>       /* [FIELD_TYPE_MAP]       = */ 0, >> +    /* [FIELD_TYPE_DATETIME]  = */ 1U << MP_DATETIME, >>   }; >>   const char *field_type_strs[] = { >> @@ -104,6 +107,7 @@ const char *field_type_strs[] = { >>       /* [FIELD_TYPE_UUID]     = */ "uuid", >>       /* [FIELD_TYPE_ARRAY]    = */ "array", >>       /* [FIELD_TYPE_MAP]      = */ "map", >> +    /* [FIELD_TYPE_DATETIME] = */ "datetime", >>   }; >>   const char *on_conflict_action_strs[] = { >> @@ -128,20 +132,21 @@ field_type_by_name_wrapper(const char *str, >> uint32_t len) >>    * values can be stored in the j type. >>    */ >>   static const bool field_type_compatibility[] = { >> -       /*   ANY   UNSIGNED  STRING   NUMBER  DOUBLE  INTEGER  BOOLEAN >> VARBINARY SCALAR  DECIMAL   UUID    ARRAY    MAP  */ >> -/*   ANY    */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   false,   false, >> -/* UNSIGNED */ true,   true,    false,   true,    false,   true, >> false,   false,  true,   false,  false,   false,   false, >> -/*  STRING  */ true,   false,   true,    false,   false,   false, >> false,   false,  true,   false,  false,   false,   false, >> -/*  NUMBER  */ true,   false,   false,   true,    false,   false, >> false,   false,  true,   false,  false,   false,   false, >> -/*  DOUBLE  */ true,   false,   false,   true,    true,    false, >> false,   false,  true,   false,  false,   false,   false, >> -/*  INTEGER */ true,   false,   false,   true,    false,   true, >> false,   false,  true,   false,  false,   false,   false, >> -/*  BOOLEAN */ true,   false,   false,   false,   false,   false, >> true,    false,  true,   false,  false,   false,   false, >> -/* VARBINARY*/ true,   false,   false,   false,   false,   false, >> false,   true,   true,   false,  false,   false,   false, >> -/*  SCALAR  */ true,   false,   false,   false,   false,   false, >> false,   false,  true,   false,  false,   false,   false, >> -/*  DECIMAL */ true,   false,   false,   true,    false,   false, >> false,   false,  true,   true,   false,   false,   false, >> -/*   UUID   */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  true,    false,   false, >> -/*   ARRAY  */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   true,    false, >> -/*    MAP   */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   false,   true, >> +       /*   ANY   UNSIGNED  STRING   NUMBER  DOUBLE  INTEGER  BOOLEAN >> VARBINARY SCALAR  DECIMAL   UUID    ARRAY    MAP     DATETIME */ >> +/*   ANY    */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   false,   false,   false, >> +/* UNSIGNED */ true,   true,    false,   true,    false,   true, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  STRING  */ true,   false,   true,    false,   false,   false, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  NUMBER  */ true,   false,   false,   true,    false,   false, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  DOUBLE  */ true,   false,   false,   true,    true,    false, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  INTEGER */ true,   false,   false,   true,    false,   true, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  BOOLEAN */ true,   false,   false,   false,   false,   false, >> true,    false,  true,   false,  false,   false,   false,   false, >> +/* VARBINARY*/ true,   false,   false,   false,   false,   false, >> false,   true,   true,   false,  false,   false,   false,   false, >> +/*  SCALAR  */ true,   false,   false,   false,   false,   false, >> false,   false,  true,   false,  false,   false,   false,   false, >> +/*  DECIMAL */ true,   false,   false,   true,    false,   false, >> false,   false,  true,   true,   false,   false,   false,   false, >> +/*   UUID   */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  true,    false,   false,   false, >> +/*   ARRAY  */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   true,    false,   false, >> +/*    MAP   */ true,   false,   false,   false,   false,   false, >> false,   false,  false,  false,  false,   false,   true,    false, >> +/* DATETIME */ true,   false,   false,   false,   false,   false, >> false,   false,  true,   false,  false,   false,   false,   true, >>   }; >>   bool >> diff --git a/src/box/field_def.h b/src/box/field_def.h >> index c5cfe5e86..120b2a93d 100644 >> --- a/src/box/field_def.h >> +++ b/src/box/field_def.h >> @@ -63,6 +63,7 @@ enum field_type { >>       FIELD_TYPE_UUID, >>       FIELD_TYPE_ARRAY, >>       FIELD_TYPE_MAP, >> +    FIELD_TYPE_DATETIME, >>       field_type_MAX >>   }; > > > Please, define FIELD_TYPE_DATETIME higher. > Right after FIELD_TYPE_UUID. > > This way you won't need to rework field type allowed in index check > in the next commit. That's very straighforward and easy, my bad that I've overcomplicated it! But I'll move the change to the next patch, as it'scorrectly has pointed out by Vova, should be part of indices support, > > >> diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c >> index 1f791980f..51855011b 100644 >> --- a/src/box/lua/serialize_lua.c >> +++ b/src/box/lua/serialize_lua.c >> @@ -768,7 +768,7 @@ static int >>   dump_node(struct lua_dumper *d, struct node *nd, int indent) >>   { >>       struct luaL_field *field = &nd->field; >> -    char buf[FPCONV_G_FMT_BUFSIZE]; >> +    char buf[FPCONV_G_FMT_BUFSIZE + 8]; > > > Why "+8"? Well, because current FPCONV_G_FMT_BUFSIZE (32) was not enough for full ISO-8601 literal with nanoseconds :) Probably I should introduce some newer constant... [Or, as Vova has suggested - just to use MAX from those 2 values, my length and FPCONV_G_FMT_BUFSIZE.] -------------------------------------------- diff --git a/src/box/lua/serialize_lua.c b/src/box/lua/serialize_lua.c index 51855011b..eef3a4995 100644 --- a/src/box/lua/serialize_lua.c +++ b/src/box/lua/serialize_lua.c @@ -768,7 +768,7 @@ static int dump_node(struct lua_dumper *d, struct node *nd, int indent) { struct luaL_field *field = &nd->field; - char buf[FPCONV_G_FMT_BUFSIZE + 8]; + char buf[MAX(FPCONV_G_FMT_BUFSIZE, DT_TO_STRING_BUFSIZE)]; int ltype = lua_type(d->L, -1); const char *str = NULL; size_t len = 0; diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h index 497cd9f14..b8d179600 100644 --- a/src/lib/core/datetime.h +++ b/src/lib/core/datetime.h @@ -87,6 +87,11 @@ struct datetime_interval { int datetime_compare(const struct datetime *lhs, const struct datetime *rhs); +/** + * Required size of datetime_to_string string buffer + */ +#define DT_TO_STRING_BUFSIZE 48 + /** * Convert datetime to string using default format * @param date source datetime value -------------------------------------------- > > >>       int ltype = lua_type(d->L, -1); >>       const char *str = NULL; >>       size_t len = 0; > > > > > >> diff --git a/src/lib/core/mp_datetime.c b/src/lib/core/mp_datetime.c >> new file mode 100644 >> index 000000000..d0a3e562c >> --- /dev/null >> +++ b/src/lib/core/mp_datetime.c >> @@ -0,0 +1,189 @@ >> +/* >> + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. >> + * >> + * Redistribution and use in source and binary forms, with or >> + * without modification, are permitted provided that the following >> + * conditions are met: >> + * >> + * 1. Redistributions of source code must retain the above >> + *    copyright notice, this list of conditions and the >> + *    following disclaimer. >> + * >> + * 2. Redistributions in binary form must reproduce the above >> + *    copyright notice, this list of conditions and the following >> + *    disclaimer in the documentation and/or other materials >> + *    provided with the distribution. >> + * >> + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND >> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED >> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR >> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL >> + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, >> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL >> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF >> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR >> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF >> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF >> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF >> + * SUCH DAMAGE. >> + */ >> + > > Same about the license. > Please, replace that with > > /* >  * SPDX-License-Identifier: BSD-2-Clause >  * >  * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. >  */ > > And do the same for all new files. Updated. > >> +#include "mp_datetime.h" >> +#include "msgpuck.h" >> +#include "mp_extension_types.h" >> + >> +/* >> +  Datetime MessagePack serialization schema is MP_EXT (0xC7 for 1 >> byte length) >> +  extension, which creates container of 1 to 3 integers. >> + >> + >> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ >> >> +  |0xC7| 4 |len (uint8)| seconds (int) | nanoseconds (uint) | offset >> (uint) | >> + >> +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ >> > > The order should be 0xC7, len(uint8), 4, seconds, ... > according to > https://github.com/msgpack/msgpack/blob/master/spec.md#ext-format-family Indeed, that was my misconception, thanks for correction! [Updated picture in the patch and in the discussion - https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043990] > >> + >> +  MessagePack extension MP_EXT (0xC7), after 1-byte length, contains: >> + >> +  - signed integer seconds part (required). Depending on the value of >> +    seconds it may be from 1 to 8 bytes positive or negative integer >> number; >> + >> +  - [optional] fraction time in nanoseconds as unsigned integer. >> +    If this value is 0 then it's not saved (unless there is offset >> field, >> +    as below); >> + >> +  - [optional] timzeone offset in minutes as unsigned integer. >> +    If this field is 0 then it's not saved. >> + */ >> + >> +static inline uint32_t >> +mp_sizeof_Xint(int64_t n) >> +{ >> +    return n < 0 ? mp_sizeof_int(n) : mp_sizeof_uint(n); >> +} >> + >> +static inline char * >> +mp_encode_Xint(char *data, int64_t v) >> +{ >> +    return v < 0 ? mp_encode_int(data, v) : mp_encode_uint(data, v); >> +} >> + >> +static inline int64_t >> +mp_decode_Xint(const char **data) >> +{ >> +    switch (mp_typeof(**data)) { >> +    case MP_UINT: >> +        return (int64_t)mp_decode_uint(data); >> +    case MP_INT: >> +        return mp_decode_int(data); >> +    default: >> +        mp_unreachable(); >> +    } >> +    return 0; >> +} > > I believe mp_decode_Xint and mp_encode_Xint > belong to a more generic file, but I couldn't find an > appropriate one. Up to you. Yup, it was planned to be placed to more generic place once it would become useful at least the 2nd time. And this time is actually 2nd (1st was in SQL AST parser branch here https://github.com/tarantool/tarantool/commit/55a4182ebfbed1a3c916fb7e326f8f7861776a7f#diff-e3f5bdfa58bcaed35b89f22e94be7ad472a6b37d656a129722ea0d5609503c6aR132-R143). But that patchset has not yet landed to the master, so once again code usage is 1st time and worth only local application. When I'll return to distributed-sql AST parser I'll reshake them and put elsewhere. > >> + >> +static inline uint32_t >> +mp_sizeof_datetime_raw(const struct datetime *date) >> +{ >> +    uint32_t sz = mp_sizeof_Xint(date->secs); >> + >> +    // even if nanosecs == 0 we need to output anything >> +    // if we have non-null tz offset > > > Please, stick with our comment format: Oh, yup, that slipt thru. Corrected. > > /* >  * Even if nanosecs == 0 we need to output anything >  * if we have non-null tz offset > */ > > >> +    if (date->nsec != 0 || date->offset != 0) >> +        sz += mp_sizeof_Xint(date->nsec); >> +    if (date->offset) >> +        sz += mp_sizeof_Xint(date->offset); >> +    return sz; >> +} >> + >> +uint32_t >> +mp_sizeof_datetime(const struct datetime *date) >> +{ >> +    return mp_sizeof_ext(mp_sizeof_datetime_raw(date)); >> +} >> + >> +struct datetime * >> +datetime_unpack(const char **data, uint32_t len, struct datetime *date) >> +{ >> +    const char * svp = *data; >> + >> +    memset(date, 0, sizeof(*date)); >> + >> +    date->secs = mp_decode_Xint(data); > > > Please, leave a comment about date->secs possible range here. > Why is it ok to store a decoded int64_t in a double. Yes, that's reasonable complain. I'll document dt supported range in the datetime.h header, and to declare legal bounds there, so we could use them later in asserts. Please see incremental patch for this step below... > > >> + >> +    len -= *data - svp; >> +    if (len <= 0) >> +        return date; >> + >> +    svp = *data; >> +    date->nsec = mp_decode_Xint(data); >> +    len -= *data - svp; >> + >> +    if (len <= 0) >> +        return date; >> + >> +    date->offset = mp_decode_Xint(data); >> + >> +    return date; >> +} >> + >> +struct datetime * >> +mp_decode_datetime(const char **data, struct datetime *date) >> +{ >> +    if (mp_typeof(**data) != MP_EXT) >> +        return NULL; >> + >> +    int8_t type; >> +    uint32_t len = mp_decode_extl(data, &type); >> + >> +    if (type != MP_DATETIME || len == 0) { >> +        return NULL; > > > Please, revert data to savepoint when decoding fails. > If mp_decode_extl or datetime_unpack fail, you mustn't > modify data. > Didn't think about this case - will make sure data points to the original location if fails. > >> +    } >> +    return datetime_unpack(data, len, date); >> +} >> + >> +char * >> +datetime_pack(char *data, const struct datetime *date) >> +{ >> +    data = mp_encode_Xint(data, date->secs); >> +    if (date->nsec != 0 || date->offset != 0) >> +        data = mp_encode_Xint(data, date->nsec); >> +    if (date->offset) >> +        data = mp_encode_Xint(data, date->offset); >> + >> +    return data; >> +} > > > > > >> diff --git a/src/lua/serializer.h b/src/lua/serializer.h >> index 0a0501a74..e7a240e0a 100644 >> --- a/src/lua/serializer.h >> +++ b/src/lua/serializer.h >> @@ -52,6 +52,7 @@ extern "C" { >>   #include >>   #include "trigger.h" >> +#include "lib/core/datetime.h" >>   #include "lib/core/decimal.h" /* decimal_t */ >>   #include "lib/core/mp_extension_types.h" >>   #include "lua/error.h" >> @@ -223,6 +224,7 @@ struct luaL_field { >>           uint32_t size; >>           decimal_t *decval; >>           struct tt_uuid *uuidval; >> +        struct datetime *dateval; >>       }; >>       enum mp_type type; >>       /* subtypes of MP_EXT */ >> diff --git a/src/lua/utils.c b/src/lua/utils.c >> index 2c89326f3..771f6f278 100644 >> --- a/src/lua/utils.c >> +++ b/src/lua/utils.c >> @@ -254,7 +254,6 @@ luaL_setcdatagc(struct lua_State *L, int idx) >>       lua_pop(L, 1); >>   } >> - > > > Extraneous change. Please, remove. Removed from the patch. Thanks! > > >>   /** >>    * A helper to register a single type metatable. >>    */ >> diff --git a/test/unit/datetime.c b/test/unit/datetime.c >> index 1ae76003b..a72ac2253 100644 >> --- a/test/unit/datetime.c >> +++ b/test/unit/datetime.c >> @@ -6,6 +6,9 @@ >>   #include "unit.h" >>   #include "datetime.h" >> +#include "mp_datetime.h" >> +#include "msgpuck.h" >> +#include "mp_extension_types.h" >>   static const char sample[] = "2012-12-24T15:30Z"; >> @@ -247,12 +250,132 @@ tostring_datetime_test(void) >>       check_plan(); >>   } >> > > > > ----------------------------------------------------- diff --git a/src/lib/core/datetime.h b/src/lib/core/datetime.h index f98f7010d..df3c1c83d 100644 --- a/src/lib/core/datetime.h +++ b/src/lib/core/datetime.h @@ -5,6 +5,7 @@ * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. */ +#include #include #include #include "c-dt/dt.h" @@ -30,6 +31,26 @@ extern "C" #define DT_EPOCH_1970_OFFSET 719163 #endif +/** + * c-dt library uses int as type for dt value, which + * represents the number of days since Rata Die date. + * This implies limits to the number of seconds we + * could safely store in our structures and then safely + * pass to c-dt functions. + * + * So supported ranges will be + * - for seconds [-185604722870400 .. 185480451417600] + * - for dates [-5879610-06-22T00:00Z .. 5879611-07-11T00:00Z] + */ +#define MAX_DT_DAY_VALUE (int64_t)INT_MAX +#define MIN_DT_DAY_VALUE (int64_t)INT_MIN +#define SECS_EPOCH_1970_OFFSET \ + ((int64_t)DT_EPOCH_1970_OFFSET * SECS_PER_DAY) +#define MAX_EPOCH_SECS_VALUE \ + (MAX_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET) +#define MIN_EPOCH_SECS_VALUE \ + (MIN_DT_DAY_VALUE * SECS_PER_DAY - SECS_EPOCH_1970_OFFSET) + /** * datetime structure keeps number of seconds since * Unix Epoch. diff --git a/src/lib/core/mp_datetime.c b/src/lib/core/mp_datetime.c index 7e475d5f1..963752c23 100644 --- a/src/lib/core/mp_datetime.c +++ b/src/lib/core/mp_datetime.c @@ -1,34 +1,12 @@ /* - * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * 1. Redistributions of source code must retain the above - * copyright notice, this list of conditions and the - * following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials - * provided with the distribution. + * SPDX-License-Identifier: BSD-2-Clause * - * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED - * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL - * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR - * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF - * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. */ +#include +#include + #include "mp_datetime.h" #include "msgpuck.h" #include "mp_extension_types.h" @@ -37,9 +15,9 @@ Datetime MessagePack serialization schema is MP_EXT (0xC7 for 1 byte length) extension, which creates container of 1 to 3 integers. - +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ - |0xC7| 4 |len (uint8)| seconds (int) | nanoseconds (uint) | offset (uint) | - +----+---+-----------+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ + +----+-----------+---+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ + |0xC7|len (uint8)| 4 | seconds (int) | nanoseconds (uint) | offset (int) | + +----+-----------+---+====~~~~~~~====+-----~~~~~~~~-------+....~~~~~~~....+ MessagePack extension MP_EXT (0xC7), after 1-byte length, contains: @@ -50,7 +28,7 @@ If this value is 0 then it's not saved (unless there is offset field, as below); - - [optional] timzeone offset in minutes as unsigned integer. + - [optional] timezone offset in minutes as signed integer. If this field is 0 then it's not saved. */ @@ -80,17 +58,34 @@ mp_decode_Xint(const char **data) return 0; } +#define check_secs(secs) \ + assert((int64_t)(secs) <= MAX_EPOCH_SECS_VALUE);\ + assert((int64_t)(secs) >= MIN_EPOCH_SECS_VALUE); + +#define check_nanosecs(nsec) assert((nsec) < 1000000000); + +#define check_tz_offset(offset) \ + assert((offset) <= (12 * 60));\ + assert((offset) >= (-12 * 60)); + static inline uint32_t mp_sizeof_datetime_raw(const struct datetime *date) { + check_secs(date->secs); uint32_t sz = mp_sizeof_Xint(date->secs); - // even if nanosecs == 0 we need to output anything - // if we have non-null tz offset - if (date->nsec != 0 || date->offset != 0) + /* + * even if nanosecs == 0 we need to output something + * if we have a non-null tz offset + */ + if (date->nsec != 0 || date->offset != 0) { + check_nanosecs(date->nsec); sz += mp_sizeof_Xint(date->nsec); - if (date->offset) + } + if (date->offset) { + check_tz_offset(date->offset); sz += mp_sizeof_Xint(date->offset); + } return sz; } @@ -103,24 +98,30 @@ mp_sizeof_datetime(const struct datetime *date) struct datetime * datetime_unpack(const char **data, uint32_t len, struct datetime *date) { - const char * svp = *data; + const char *svp = *data; memset(date, 0, sizeof(*date)); - date->secs = mp_decode_Xint(data); + int64_t seconds = mp_decode_Xint(data); + check_secs(seconds); + date->secs = seconds; len -= *data - svp; if (len <= 0) return date; svp = *data; - date->nsec = mp_decode_Xint(data); + uint64_t nanoseconds = mp_decode_uint(data); + check_nanosecs(nanoseconds); + date->nsec = nanoseconds; len -= *data - svp; if (len <= 0) return date; - date->offset = mp_decode_Xint(data); + int64_t offset = mp_decode_Xint(data); + check_tz_offset(offset); + date->offset = offset; return date; } @@ -131,10 +132,12 @@ mp_decode_datetime(const char **data, struct datetime *date) if (mp_typeof(**data) != MP_EXT) return NULL; + const char *svp = *data; int8_t type; uint32_t len = mp_decode_extl(data, &type); if (type != MP_DATETIME || len == 0) { + *data = svp; return NULL; } return datetime_unpack(data, len, date); @@ -145,7 +148,7 @@ datetime_pack(char *data, const struct datetime *date) { data = mp_encode_Xint(data, date->secs); if (date->nsec != 0 || date->offset != 0) - data = mp_encode_Xint(data, date->nsec); + data = mp_encode_uint(data, date->nsec); if (date->offset) data = mp_encode_Xint(data, date->offset); @@ -165,7 +168,9 @@ mp_encode_datetime(char *data, const struct datetime *date) int mp_snprint_datetime(char *buf, int size, const char **data, uint32_t len) { - struct datetime date = {0, 0, 0}; + struct datetime date = { + .secs = 0, .nsec = 0, .offset = 0 + }; if (datetime_unpack(data, len, &date) == NULL) return -1; @@ -176,7 +181,9 @@ mp_snprint_datetime(char *buf, int size, const char **data, uint32_t len) int mp_fprint_datetime(FILE *file, const char **data, uint32_t len) { - struct datetime date = {0, 0, 0}; + struct datetime date = { + .secs = 0, .nsec = 0, .offset = 0 + }; if (datetime_unpack(data, len, &date) == NULL) return -1; diff --git a/src/lib/core/mp_datetime.h b/src/lib/core/mp_datetime.h index 9a4d2720c..92e94a243 100644 --- a/src/lib/core/mp_datetime.h +++ b/src/lib/core/mp_datetime.h @@ -1,33 +1,8 @@ #pragma once /* - * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * 1. Redistributions of source code must retain the above - * copyright notice, this list of conditions and the - * following disclaimer. + * SPDX-License-Identifier: BSD-2-Clause * - * 2. Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials - * provided with the distribution. - * - * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED - * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL - * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR - * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF - * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. + * Copyright 2021, Tarantool AUTHORS, please see AUTHORS file. */ #include ----------------------------------------------------- Thanks, Timur