decimal to string serialization #10

parihaaraka · 2019-10-23T15:22:36Z

No description provided.

sergepetrenko

Thanks for the patch!
Please see some of my comments below.

msgpuck.c

sergepetrenko

Please add a test and check out my 2 comments below.

Regarding the test encode some simple decimal here
https://github.com/tarantool/msgpuck/blob/master/test/msgpuck.c#L887
You may say char decimal[] = "\xd6\x01\x02\x03\x45\x6d", this is -34.56 encoded,
and then print it with mp_snprint

Otherwise LGTM

msgpuck.c

sergepetrenko

Thank you for the fixes.
Please consider one last nit below. Otherwise LGTM
I will forward this to @Totktonada to take a look and merge.

test/msgpuck.c

sergepetrenko

LGTM, thanks. @Totktonada, take a look, please.

(use https://github.com/parihaaraka/msgpuck.git or wait for tarantool/msgpuck#10 to be merged)

Totktonada

@parihaaraka Thank you for your work on that, but I think it should not be done in this way (see more below). Maybe it would be better to file an issue to tarantool/tarantool or this repository and describe what exactly you want to achieve.

Totktonada · 2019-11-18T18:16:53Z

msgpuck.c

@@ -232,6 +232,94 @@ mp_format(char *data, size_t data_size, const char *format, ...)
 	return res;
 }

+size_t
+print_decimal(char **buf, size_t buf_size, const char *val, uint32_t val_bytes)


Should not be part of msgpuck library, but our mp_decimal implementation.

We should however provide an ability to set custom handlers for MP_EXT subtypes for mp_[v]format and mp_{f,sn}print.

I propose to provide subroutines and (maybe) mp_format percent-placeholders for extension types via defines that should exist before including of msgpuck.h.

Should not be part of mp_decimal, gmp or any other library. We convert messagepack to string here. Any decimal library internals are out of scope. This repository is a tarantool's one, so we may relay on ext subtypes being used by tarantool. But callbacks to encode/decode external data (e.g. mp_decimal) to/from decimal are expected.

It is the general purpose library, so it would be good to handle all operations with decimals from mp_decimal. Of course, if this will not block usage of this library by, say, tarantool connectors developers. That is why I keep asking you about your case.

Maybe we should provide mp_decimal.h with tarantool-dev/devel packages. Or ship extensions with msgpuck and allow to enable / disable them at build time.

Totktonada · 2019-11-18T18:18:59Z

test/msgpuck.c

 	assert(d <= msgpack + sizeof(msgpack));

 	const char *expected =
-		"[-5, 42, \"kill bill\", [], "
+		"["


Unneeded change.

I need it to match the mess with the array size. Unneeded reproval.

Totktonada · 2019-11-18T18:56:51Z

msgpuck.c

+		scale = mp_decode_int(&val);
+
+	if (scale_head == val || !val_bytes)
+		return 0;         /* "undefined" (is ext mp type 1 misused?) */


Don't got why the comment is indented with 9 whitespaces. We usually avoid comments on the same line with code.

The same for comments below.

I just like aligned comments. You guys love different things. Just prepare it by yourself.

msgpuck/msgpuck.c

Line 108 in 8ae606a

int int_status = 0; /* 1 - signed, 2 - unsigned */

msgpuck/msgpuck.h

Line 96 in 8ae606a

#define __STDC_CONSTANT_MACROS 1 /* make С++ to be happy */

and tons below

Totktonada · 2019-11-18T19:04:42Z

msgpuck.c

+		if (ext_type != 1) {						\
+			PRINTF("undefined");					\
+		} else {							\
+			char static_buf[128];					\


I think we should use PRINTF inside print_decimal and don't create static (automatic really) / dynamic buffers explicitly in this part of code.

You the boss (the second one)

Totktonada · 2019-11-18T19:13:40Z

test/msgpuck.c

 	int esize = strlen(expected);

-	char result[256];
+	char result[512];


Side note: the branch contains commits from rtsisyk/master, they should be removed. Other commits should be squashed.

?!
256 bytes is not enougth

parihaaraka · 2019-11-23T06:59:09Z

@parihaaraka Thank you for your work on that, but I think it should not be done in this way (see more below). Maybe it would be better to file an issue to tarantool/tarantool or this repository and describe what exactly you want to achieve.

I want to print all the mp types supported by tarantool.

Totktonada · 2020-05-19T11:15:54Z

tarantool/tarantool#4719

Totktonada · 2020-05-19T12:31:47Z

Usage example: parihaaraka/cpp2tnt@3ead452

…s to print decimal values

Totktonada · 2022-05-20T10:00:07Z

I added @alyapunov to make final decisions and proceed somehow (or decline).

sergepetrenko · 2022-05-20T13:53:09Z

@Totktonada
We already have decimal printer in tarantool's libcore:
https://github.com/tarantool/tarantool/blob/a74c549a16f1168b21644bd9aa85bceba99ea6a1/src/lib/core/mp_decimal.c#L74-L81

Same stands for each other ext type used in tarantool.
See, for example, mp_snprint_datetime, mp_snprint_uuid and so on.\

And msgpack has mp_snprint_ext stub, which is replaced by tarantool's msgpack_snprint_ext.
msgpack_snprint_ext in turn chooses the appropriate handler (e.g. mp_snprint_decimal)

parihaaraka · 2022-05-21T15:21:55Z

@Totktonada We already have decimal printer in tarantool's libcore: https://github.com/tarantool/tarantool/blob/a74c549a16f1168b21644bd9aa85bceba99ea6a1/src/lib/core/mp_decimal.c#L74-L81

Same stands for each other ext type used in tarantool. See, for example, mp_snprint_datetime, mp_snprint_uuid and so on.\

And msgpack has mp_snprint_ext stub, which is replaced by tarantool's msgpack_snprint_ext. msgpack_snprint_ext in turn chooses the appropriate handler (e.g. mp_snprint_decimal)

External application doesn't deal with internal tarantool's structures. It needs to print msgpack (and convert to mpdecimal or another corresponding stuff). BTW string to tnt ext types conversion would be great too.

sergos · 2022-11-18T07:45:19Z

@alyapunov please proceed with review

alyapunov · 2022-11-29T16:26:01Z

Hi, thanks for effort!
We decided to leave msgpuck as independent library that provides external configuration for ext types (see mp_fprint_ext etc). Tarantool sets those handlers, introducing tarantool-specific exts. There's no more need to patch msgpuck.
If you need to use tarantool extensions outside of tarantool - just grab msgpack_init etc.

sergepetrenko requested changes Oct 24, 2019

View reviewed changes

msgpuck.c Outdated Show resolved Hide resolved

msgpuck.c Outdated Show resolved Hide resolved

msgpuck.c Outdated Show resolved Hide resolved

parihaaraka requested a review from sergepetrenko October 25, 2019 16:28

sergepetrenko requested changes Oct 25, 2019

View reviewed changes

msgpuck.c Outdated Show resolved Hide resolved

msgpuck.c Outdated Show resolved Hide resolved

parihaaraka requested a review from sergepetrenko October 25, 2019 19:55

sergepetrenko requested changes Oct 28, 2019

View reviewed changes

test/msgpuck.c Outdated Show resolved Hide resolved

parihaaraka requested a review from sergepetrenko October 29, 2019 20:32

sergepetrenko approved these changes Nov 1, 2019

View reviewed changes

sergepetrenko requested a review from Totktonada November 1, 2019 10:56

parihaaraka pushed a commit to parihaaraka/cpp2tnt that referenced this pull request Nov 5, 2019

decimal to string serilization is moved to msgpuck

3ead452

(use https://github.com/parihaaraka/msgpuck.git or wait for tarantool/msgpuck#10 to be merged)

Totktonada reviewed Nov 18, 2019

View reviewed changes

Totktonada mentioned this pull request May 19, 2020

msgpuck extension with tarantool msgpack types tarantool/tarantool#5002

Open

msgpuck updated; breaking change: need to set up ext types serializer…

df23665

…s to print decimal values

parihaaraka force-pushed the master branch from 17a3ec7 to df23665 Compare October 11, 2020 19:57

parihaaraka added 2 commits October 26, 2020 16:09

extern C wrapper

d9552c3

floating point values serialization by mp_snprint() improved

f6f510f

Totktonada assigned alyapunov May 20, 2022

Totktonada requested a review from alyapunov May 20, 2022 09:59

alyapunov closed this Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decimal to string serialization #10

decimal to string serialization #10

parihaaraka commented Oct 23, 2019

sergepetrenko left a comment

sergepetrenko left a comment

sergepetrenko left a comment

sergepetrenko left a comment

Totktonada left a comment

Totktonada Nov 18, 2019

parihaaraka Nov 23, 2019

Totktonada Nov 23, 2019

Totktonada Nov 18, 2019

parihaaraka Nov 23, 2019

Totktonada Nov 18, 2019

parihaaraka Nov 23, 2019

Totktonada Nov 18, 2019

parihaaraka Nov 23, 2019

Totktonada Nov 18, 2019

parihaaraka Nov 23, 2019

parihaaraka commented Nov 23, 2019

Totktonada commented May 19, 2020

Totktonada commented May 19, 2020

Totktonada commented May 20, 2022

sergepetrenko commented May 20, 2022 •

edited

parihaaraka commented May 21, 2022

sergos commented Nov 18, 2022

alyapunov commented Nov 29, 2022

decimal to string serialization #10

decimal to string serialization #10

Conversation

parihaaraka commented Oct 23, 2019

sergepetrenko left a comment

Choose a reason for hiding this comment

sergepetrenko left a comment

Choose a reason for hiding this comment

sergepetrenko left a comment

Choose a reason for hiding this comment

sergepetrenko left a comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parihaaraka commented Nov 23, 2019

Totktonada commented May 19, 2020

Totktonada commented May 19, 2020

Totktonada commented May 20, 2022

sergepetrenko commented May 20, 2022 • edited

parihaaraka commented May 21, 2022

sergos commented Nov 18, 2022

alyapunov commented Nov 29, 2022

sergepetrenko commented May 20, 2022 •

edited