Re: [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for datetime

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.


>   decimal_from_string
>   decimal_unpack
>   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.


>   
> 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"?


>   	int ltype = lua_type(d->L, -1);
>   	const char *str = NULL;
>   	size_t len = 0;


<stripped>


> 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 <COPYRIGHT HOLDER> ``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
> + * <COPYRIGHT HOLDER> 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.

> +#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

> +
> +  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.

> +
> +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:

/*
  * 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.


> +
> +	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.


> +	}
> +	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;
> +}


<stripped>


> 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 <lauxlib.h>
>   
>   #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.


>   /**
>    * 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();
>   }
>   
>


<stripped>

-- 
Serge Petrenko