Re: [tarantool-patches] [PATCH v3 4/4] decimal: add MessagePack encoding/decoding support

[Serge Petrenko <sergepetrenko@tarantool.org>]

> 6 июня 2019 г., в 12:27, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> [ RESEND ]
> 
> On Tue, Jun 04, 2019 at 07:04:19PM +0300, Serge Petrenko wrote:
>> Add methods to encode/decode decimals as msgpack.
>> 
>> Part of #692
>> ---
>> cmake/BuildDecNumber.cmake   |   1 +
>> src/lib/core/decimal.c       |  64 +++++++++++
>> src/lib/core/decimal.h       |  38 +++++++
>> src/lib/core/mp_user_types.h |  38 +++++++
>> test/unit/decimal.c          |  73 ++++++++++++-
>> test/unit/decimal.result     | 204 ++++++++++++++++++++++++++++++++++-
>> 6 files changed, 416 insertions(+), 2 deletions(-)
>> create mode 100644 src/lib/core/mp_user_types.h
>> 
>> diff --git a/cmake/BuildDecNumber.cmake b/cmake/BuildDecNumber.cmake
>> index 80942fe05..abc6c64c4 100644
>> --- a/cmake/BuildDecNumber.cmake
>> +++ b/cmake/BuildDecNumber.cmake
>> @@ -4,6 +4,7 @@ macro(decnumber_build)
>>     set(decnumber_src
>> 	${PROJECT_SOURCE_DIR}/third_party/decNumber/decNumber.c
>> 	${PROJECT_SOURCE_DIR}/third_party/decNumber/decContext.c
>> +	${PROJECT_SOURCE_DIR}/third_party/decNumber/decPacked.c
>>     )
>> 
>>     add_library(decNumber STATIC ${decnumber_src})
>> diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c
>> index 12250da95..7b3b1d8c9 100644
>> --- a/src/lib/core/decimal.c
>> +++ b/src/lib/core/decimal.c
>> @@ -30,7 +30,10 @@
>>  */
>> 
>> #include "decimal.h"
>> +#include "mp_user_types.h"
>> +#include "src/lib/msgpuck/msgpuck.h"
>> #include "third_party/decNumber/decContext.h"
>> +#include "third_party/decNumber/decPacked.h"
>> #include <stdlib.h>
>> #include <assert.h>
>> 
>> @@ -291,3 +294,64 @@ decimal_sqrt(decimal *res, const decimal *lhs, uint32_t precision)
>> 
>> 	return decimal_finalize(res, &op_context);
>> }
>> +
>> +static inline uint32_t
>> +decimal_len(decimal *dec)
>> +{
>> +	       /* 1 + ceil((digits + 1) / 2) */
> 
> Bad indentation.
> 
>> +	return 2 + dec->digits / 2;
>> +}
>> +
>> +uint32_t
>> +mp_sizeof_decimal(decimal *dec) {
>> +	uint32_t l = decimal_len(dec);
>> +	return mp_sizeof_extl(l) + l;
> 
> Better use mp_sizeof_ext instead of mp_sizeof_extl here?
> 
>> +}
>> +
>> +char *
>> +mp_encode_decimal(char *data, decimal *dec)
>> +{
>> +	uint32_t len = decimal_len(dec);
>> +	data = mp_encode_extl(data, MP_DECIMAL, len);
>> +	data = mp_store_u8(data, decimal_scale(dec));
>> +	len--;
>> +	int32_t scale;
>> +	char *tmp = (char *)decPackedFromNumber((uint8_t *)data, len, &scale, dec);
>> +	assert(tmp == data);
>> +	assert(scale == (int32_t)decimal_scale(dec));
>> +	data += len;
>> +	return data;
>> +}
>> +
>> +decimal *
>> +mp_decode_decimal_raw(const char **data, decimal *dec, uint32_t len)
>> +{
>> +	int32_t scale = mp_load_u8(data);
>> +	len--;
>> +	decimal *res = decPackedToNumber((uint8_t *)*data, len, &scale, dec);
>> +	if (res)
>> +		*data += len;
>> +	else
>> +		(*data)--;
>> +	return res;
>> +}
>> +
>> +decimal *
>> +mp_decode_decimal(const char **data, decimal *dec)
>> +{
>> +	int8_t type;
>> +	uint32_t len;
>> +
>> +	if (mp_typeof(**data) != MP_EXT)
>> +		return NULL;
>> +	const char *const svp = *data;
>> +	len = mp_decode_extl(data, &type);
>> +
>> +	if (type != MP_DECIMAL || len == 0)
>> +		return NULL;
>> +	decimal *res = mp_decode_decimal_raw(data, dec, len);
>> +	if (!res) {
>> +		*data = svp;
>> +	}
>> +	return res;
>> +}
>> diff --git a/src/lib/core/decimal.h b/src/lib/core/decimal.h
>> index f2812f500..cfcc98073 100644
>> --- a/src/lib/core/decimal.h
>> +++ b/src/lib/core/decimal.h
>> @@ -156,4 +156,42 @@ decimal_exp(decimal *res, const decimal *lhs, uint32_t precision);
>> decimal *
>> decimal_sqrt(decimal *res, const decimal *lhs, uint32_t precision);
>> 
>> +
>> +/**
>> + * Return length of message pack encoded decimal with extension header.
>> + * Equivalent to mp_sizeof_ext(decimal representation length)
>> + */
>> +uint32_t
>> +mp_sizeof_decimal(decimal *dec);
>> +
>> +/**
>> + * Encode a decimal value to msgpack.
>> + *
>> + * @return data + mp_sizeof_decimal(dec)
>> + */
>> +char *
>> +mp_encode_decimal(char *data, decimal *dec);
>> +
>> +/**
>> + * Decode a decimal value as though msgpack
>> + * ext header was already decoded.
>> + *
>> + * \post *data = *data + value representation length
>> + * @return NULL if value encoding is incorrect
>> + *	    dec  otherwise.
>> + */
>> +decimal *
>> +mp_decode_decimal_raw(const char **data, decimal *dec, uint32_t len);
>> +
>> +/**
>> + * Decode a decimal encoded as msgpack with ext header.
>> + *
>> + * \post *data = *data + mp_sizeof_decimal(dec)
>> + * @return NULL if mp_typeof(**data) != MP_EXT
>> + *		   or value encoding is incorrect
>> + *	   dec  otherwise.
>> + */
>> +decimal *
>> +mp_decode_decimal(const char **data, decimal *dec);
>> +
> 
> Mixing mp_ and decimal_ methods in the same file looks ugly, because
> msgpack is just one format - we might add other formats in future and it
> doesn't sound like a good idea to implement their decoders in decimal.c.
> IMO decimal.h should only contain methods to pack/unpack decimals in
> some format, say
> 
>  decimal_pack_len/decimal_pack/decimal_unpack
> 
> or
> 
>  decimal_encode/decimal_decode
> 
> mp_ methods should live in some other file (mp_ext, mp_ext_decimal,
> mp_decimal.[hc]?)

Will do after decimal to Lua exposure, no problem.

> 
> And I think it's better add and test those generic methods in patch 2,
> along with the decimal infrastructure while this patch should be about
> msgpack ext only.

Added the methods to patch 2 and resent the patchset as v4.

--
Serge Petrenko
sergepetrenko@tarantool.org