[PATCH 3/5] lua: allow to encode and decode decimals as msgpack
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Jul 24 17:11:36 MSK 2019
On Wed, Jul 17, 2019 at 06:33:44PM +0300, Serge Petrenko wrote:
> diff --git a/src/lib/core/decimal.c b/src/lib/core/decimal.c
> index 1423ae418..2c69a773c 100644
> --- a/src/lib/core/decimal.c
> +++ b/src/lib/core/decimal.c
> @@ -33,6 +33,7 @@
> #include "third_party/decNumber/decContext.h"
> #include "third_party/decNumber/decPacked.h"
> #include "lib/core/tt_static.h"
> +#include "lib/msgpuck/msgpuck.h"
> #include <stddef.h>
> #include <stdlib.h>
> #include <float.h> /* DBL_DIG */
> @@ -312,12 +313,15 @@ char *
> decimal_pack(char *data, const decimal_t *dec)
> {
> uint32_t len = decimal_len(dec);
> - *data++ = decimal_scale(dec);
> + /* reserve space for resulting scale */
> + char *svp = data++;
> len--;
> int32_t scale;
> char *tmp = (char *)decPackedFromNumber((uint8_t *)data, len, &scale, dec);
> assert(tmp == data);
> - assert(scale == (int32_t)decimal_scale(dec));
> + /* scale may be negative, when exponent is > 0 */
> + assert(scale == (int32_t)decimal_scale(dec) || scale < 0);
> + mp_store_u8(svp, (int8_t)scale);
> (void)tmp;
> data += len;
> return data;
> @@ -326,7 +330,7 @@ decimal_pack(char *data, const decimal_t *dec)
> decimal_t *
> decimal_unpack(const char **data, uint32_t len, decimal_t *dec)
> {
> - int32_t scale = *((*data)++);
> + int32_t scale = (int8_t)mp_load_u8(data);
> len--;
> decimal_t *res = decPackedToNumber((uint8_t *)*data, len, &scale, dec);
> if (res)
How is this related to this patch? Is this a fix? If so, it should be
submitted in a separate patch with an appropriate test case.
> diff --git a/src/lib/core/mp_user_types.h b/src/lib/core/mp_user_types.h
> index 9158b40d3..8211e3e79 100644
> --- a/src/lib/core/mp_user_types.h
> +++ b/src/lib/core/mp_user_types.h
> @@ -32,7 +32,8 @@
> */
>
> enum mp_user_type {
> - MP_DECIMAL = 0
> + MP_UNKNOWN = 0,
> + MP_DECIMAL = 1
MP_UNKNOWN doesn't make any sense IMO. AFAICS you need it solely to
convert Lua to msgpack and back. Let's please try to get along without
it. See also my comment to luamp_encode_r.
> };
>
> #endif
> diff --git a/src/lib/core/mpstream.c b/src/lib/core/mpstream.c
> index 8b7276ab1..a46b7962b 100644
> --- a/src/lib/core/mpstream.c
> +++ b/src/lib/core/mpstream.c
> @@ -33,6 +33,7 @@
> #include <assert.h>
> #include <stdint.h>
> #include "msgpuck.h"
> +#include "mp_decimal.h"
>
> void
> mpstream_reserve_slow(struct mpstream *stream, size_t size)
> @@ -186,6 +187,16 @@ mpstream_encode_binl(struct mpstream *stream, uint32_t len)
> mpstream_advance(stream, pos - data);
> }
>
> +void
> +mpstream_encode_decimal(struct mpstream *stream, decimal_t *val)
val should be const
> +{
> + char *data = mpstream_reserve(stream, mp_sizeof_decimal(val));
> + if (data == NULL)
> + return;
> + char *pos = mp_encode_decimal(data, val);
> + mpstream_advance(stream, pos - data);
> +}
> +
> void
> mpstream_memcpy(struct mpstream *stream, const void *src, uint32_t n)
> {
> diff --git a/src/lua/decimal.c b/src/lua/decimal.c
> index e548cdb9d..ab8d85f75 100644
> --- a/src/lua/decimal.c
> +++ b/src/lua/decimal.c
> @@ -31,7 +31,7 @@
>
> #include "lua/decimal.h"
> #include "lib/core/decimal.h"
> -#include "lua/utils.h"
> +#include "lua/utils.h" /* CTID_DECIMAL, ... */
Why not define CTID in lua/decimal.h?
>
> #include <lua.h>
> #include <lauxlib.h>
> @@ -69,16 +69,18 @@ ldecimal_##name(struct lua_State *L) { \
> static int \
> ldecimal_##name(struct lua_State *L) { \
> assert(lua_gettop(L) == 2); \
> + if (lua_isnil(L, 1) || lua_isnil(L, 2)) { \
> + lua_pushboolean(L, false); \
> + return 1; \
> + } \
What is this change for?
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index 2126988eb..3cb7d7dcd 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -41,6 +41,10 @@
> #include <small/region.h>
> #include <small/ibuf.h>
>
> +#include "lua/decimal.h"
> +#include "lib/core/decimal.h"
> +#include "lib/core/mp_user_types.h"
> +
> #include <fiber.h>
>
> void
> @@ -175,16 +179,23 @@ restart: /* used by MP_EXT */
> assert(lua_gettop(L) == top);
> return MP_ARRAY;
> case MP_EXT:
> - /* Run trigger if type can't be encoded */
> - type = luamp_encode_extension(L, top, stream);
> - if (type != MP_EXT)
> - return type; /* Value has been packed by the trigger */
> - /* Try to convert value to serializable type */
> - luaL_convertfield(L, cfg, top, field);
> - /* handled by luaL_convertfield */
> - assert(field->type != MP_EXT);
> - assert(lua_gettop(L) == top);
> - goto restart;
> + switch (field->ext_type)
> + {
> + case MP_DECIMAL:
> + mpstream_encode_decimal(stream, field->decval);
> + return MP_EXT;
> + case MP_UNKNOWN:
> + /* Run trigger if type can't be encoded */
> + type = luamp_encode_extension(L, top, stream);
> + if (type != MP_EXT)
> + return type; /* Value has been packed by the trigger */
> + /* Try to convert value to serializable type */
> + luaL_convertfield(L, cfg, top, field);
> + /* handled by luaL_convertfield */
> + assert(field->type != MP_EXT || field->ext_type != MP_UNKNOWN);
> + assert(lua_gettop(L) == top);
> + goto restart;
> + }
I don't like this two-level switch-case. AFAIU we don't really need to
use MP_* types for luaL_field::type. We could instead introduce a new
enum (like LUA_FIELD_*). This would allow us to make this switch-case
plane. Please consider it. This could be done in a separate patch.
> }
> return MP_EXT;
> }
> @@ -283,9 +294,28 @@ luamp_decode(struct lua_State *L, struct luaL_serializer *cfg,
> return;
> }
> case MP_EXT:
> - luamp_decode_extension(L, data);
> + {
> + uint32_t len;
> + int8_t type;
> + len = mp_decode_extl(data, &type);
> + switch (type) {
> + case MP_DECIMAL:
> + {
> + decimal_t *dec = lua_pushdecimal(L);
> + dec = decimal_unpack(data, len, dec);
> + if (dec == NULL) {
> + lua_pop(L, -1);
> + luaL_error(L, "msgpack.decode: "
> + "invalid MsgPack.");
> + }
> + return;
> + }
> + default:
> + luamp_decode_extension(L, data);
> + }
> break;
> }
> + }
Again, two-level switch-case doesn't look good. In this case I think we
could add a helper function which would determine LUA_FIELD_* type by
looking at data.
> }
>
>
> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
> index bfeedbc4b..4c799eed0 100644
> --- a/src/lua/msgpackffi.lua
> +++ b/src/lua/msgpackffi.lua
> @@ -17,10 +17,18 @@ char *
> mp_encode_float(char *data, float num);
> char *
> mp_encode_double(char *data, double num);
> +char *
> +mp_encode_decimal(char *data, decimal_t *dec);
> +uint32_t
> +mp_sizeof_decimal(const decimal_t *dec);
> float
> mp_decode_float(const char **data);
> double
> mp_decode_double(const char **data);
> +uint32_t
> +mp_decode_extl(const char **data, int8_t *type);
> +decimal_t *
> +decimal_unpack(const char **data, uint32_t len, decimal_t *dec);
> ]])
>
> local strict_alignment = (jit.arch == 'arm')
> @@ -117,6 +125,11 @@ local function encode_double(buf, num)
> builtin.mp_encode_double(p, num)
> end
>
> +local function encode_decimal(buf, num)
> + local p = buf:alloc(builtin.mp_sizeof_decimal(num))
> + builtin.mp_encode_decimal(p, num)
> +end
> +
> local function encode_int(buf, num)
> if num >= 0 then
> if num <= 0x7f then
> @@ -294,6 +307,7 @@ on_encode(ffi.typeof('const unsigned char'), encode_int)
> on_encode(ffi.typeof('bool'), encode_bool_cdata)
> on_encode(ffi.typeof('float'), encode_float)
> on_encode(ffi.typeof('double'), encode_double)
> +on_encode(ffi.typeof('decimal_t'), encode_decimal)
>
> --------------------------------------------------------------------------------
> -- Decoder
> @@ -473,6 +487,23 @@ local function decode_map(data, size)
> return setmetatable(map, msgpack.map_mt)
> end
>
> +local function decode_ext(data)
> + local t = ffi.new("int8_t[1]")
> + -- mp_decode_extl and mp_decode_decimal
> + -- need type code
> + data[0] = data[0] - 1
> + local old_data = data[0]
> + local len = builtin.mp_decode_extl(data, t)
> + --MP_DECIMAL
> + if t[0] == 1 then
> + local num = ffi.new("decimal_t")
> + builtin.decimal_unpack(data, len, num)
> + return num
> + else
> + error("Unsupported extension type")
> + end
> +end
> +
> local decoder_hint = {
> --[[{{{ MP_BIN]]
> [0xc4] = function(data) return decode_str(data, decode_u8(data)) end;
> @@ -528,6 +559,8 @@ decode_r = function(data)
> return false
> elseif c == 0xc3 then
> return true
> + elseif c >= 0xd4 and c <= 0xd8 or c >= 0xc7 and c <= 0xc9 then
> + return decode_ext(data)
Could you please introduce some infrastructure that would allow us to
easily add new MP_EXT types in future? Some kind of conversion table
mapping ext type to a decoder function, may be?
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 7e7cdc0c6..3ca43292b 100644
> --- a/src/lua/utils.h
> +++ b/src/lua/utils.h
> @@ -54,6 +54,8 @@ extern "C" {
> #include <lj_meta.h>
>
> #include "lua/error.h"
> +#include "lib/core/mp_user_types.h"
This inclusion seems to be pointless.
> diff --git a/test/app/msgpack.test.lua b/test/app/msgpack.test.lua
> index e0880ac22..9224d870a 100644
> --- a/test/app/msgpack.test.lua
> +++ b/test/app/msgpack.test.lua
> @@ -84,3 +84,18 @@ size = msgpack.encode(100, buf)
> -- is not negative.
> --
> msgpack.decode(ffi.cast('char *', '\x04\x05\x06'), -1)
> +
> +--
> +-- gh-4333: msgpack encode/decode decimals.
> +--
> +decimal = require('decimal')
> +a = decimal.new('1e37')
> +b = decimal.new('1e-38')
> +c = decimal.new('1')
> +d = decimal.new('0.1234567')
> +e = decimal.new('123.4567')
> +msgpack.decode(msgpack.encode(a)) == a
> +msgpack.decode(msgpack.encode(b)) == b
> +msgpack.decode(msgpack.encode(c)) == c
> +msgpack.decode(msgpack.encode(d)) == d
> +msgpack.decode(msgpack.encode(e)) == e
Please also test
- msgpackffi
- storing and accessing decimal in tuples
- storing decimals in spaces
More information about the Tarantool-patches
mailing list