From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: <v.shpilevoy@tarantool.org>, <olegrok@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v3 5/9] box, datetime: messagepack support for datetime
Date: Tue, 3 Aug 2021 16:38:44 +0300 [thread overview]
Message-ID: <0ea601d7886c$e12e6a80$a38b3f80$@tarantool.org> (raw)
In-Reply-To: <652dc466ebae43d3ac541785b57864d6519a91c7.1627864075.git.tsafin@tarantool.org>
After discussions with Sergey Petrenko I've realized
that there were several bugs in a way datetime was introduced
to fied_def.c, please see patch below...
> From: Timur Safin <tsafin@tarantool.org>
> Subject: [PATCH v3 5/9] box, datetime: messagepack support for datetime
>
> 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
>
> diff --git a/src/box/field_def.c b/src/box/field_def.c
> index 51acb8025..6964e3e9f 100644
> --- a/src/box/field_def.c
> +++ b/src/box/field_def.c
> @@ -67,11 +67,12 @@ const uint32_t field_mp_type[] = {
> /* [FIELD_TYPE_VARBINARY] = */ 1U << MP_BIN,
> /* [FIELD_TYPE_SCALAR] = */ (1U << MP_UINT) | (1U << MP_INT) |
> (1U << MP_FLOAT) | (1U << MP_DOUBLE) | (1U << MP_STR) |
> - (1U << MP_BIN) | (1U << MP_BOOL),
> + (1U << MP_BIN) | (1U << MP_BOOL) | (1U << MP_DATETIME),
> /* [FIELD_TYPE_DECIMAL] = */ 0, /* only MP_DECIMAL is supported */
> /* [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 */
> };
Here datetime is in the incorrect definition: it's in field_mp_type[]
but should be in field_ext_type[] as bein MP_EXT type.
>
> const uint32_t field_ext_type[] = {
> @@ -88,6 +89,7 @@ const uint32_t field_ext_type[] = {
> /* [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 +106,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 +131,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, false, false, false, false, false, true,
> };
Also here we did not put datetime to the scalar column, while as
being scalar type it should be possible to assign datetime values
to the scalar field. [Also we may pay attention to the fact that
uuid has the same problem - it will be investigated separately]
Here are changes we need to introduce to the code in patch:
------------------------------------------
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 6964e3e9f..2682a42ee 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -67,7 +67,7 @@ const uint32_t field_mp_type[] = {
/* [FIELD_TYPE_VARBINARY] = */ 1U << MP_BIN,
/* [FIELD_TYPE_SCALAR] = */ (1U << MP_UINT) | (1U << MP_INT) |
(1U << MP_FLOAT) | (1U << MP_DOUBLE) | (1U << MP_STR) |
- (1U << MP_BIN) | (1U << MP_BOOL) | (1U << MP_DATETIME),
+ (1U << MP_BIN) | (1U << MP_BOOL),
/* [FIELD_TYPE_DECIMAL] = */ 0, /* only MP_DECIMAL is supported */
/* [FIELD_TYPE_UUID] = */ 0, /* only MP_UUID is supported */
/* [FIELD_TYPE_ARRAY] = */ 1U << MP_ARRAY,
@@ -84,7 +84,8 @@ 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,
@@ -145,7 +146,7 @@ static const bool field_type_compatibility[] = {
/* 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, false, false, false, false, false, true,
+/* DATETIME */ true, false, false, false, false, false, false, false, true, false, false, false, false, true,
};
Bool
------------------------------------------
Will update patch to push it very soon.
Regards,
Timur
next prev parent reply other threads:[~2021-08-03 13:38 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-02 0:40 [Tarantool-patches] [PATCH v3 0/9] Initial datetime support Timur Safin via Tarantool-patches
2021-08-02 0:40 ` [Tarantool-patches] [PATCH v3 1/9] build: add Christian Hansen c-dt to the build Timur Safin via Tarantool-patches
2021-08-04 23:58 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-05 8:55 ` Safin Timur via Tarantool-patches
2021-08-08 14:34 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-02 0:40 ` [Tarantool-patches] [PATCH v3 2/9] lua: built-in module datetime Timur Safin via Tarantool-patches
2021-08-04 23:58 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-06 0:23 ` Safin Timur via Tarantool-patches
2021-08-06 1:30 ` Oleg Babin via Tarantool-patches
2021-08-06 13:00 ` Safin Timur via Tarantool-patches
2021-08-06 17:24 ` Safin Timur via Tarantool-patches
2021-08-08 11:26 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-08 16:35 ` Safin Timur via Tarantool-patches
2021-08-10 12:20 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-10 12:21 ` Igor Munkin via Tarantool-patches
2021-08-12 20:47 ` Safin Timur via Tarantool-patches
2021-08-15 20:52 ` Safin Timur via Tarantool-patches
2021-08-06 0:26 ` Safin Timur via Tarantool-patches
2021-08-08 14:34 ` Vladislav Shpilevoy via Tarantool-patches
2021-08-08 16:47 ` Safin Timur via Tarantool-patches
2021-08-02 0:40 ` [Tarantool-patches] [PATCH v3 3/9] lua, datetime: datetime tests Timur Safin via Tarantool-patches
2021-08-06 0:25 ` Safin Timur via Tarantool-patches
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 4/9] lua, datetime: display datetime Timur Safin via Tarantool-patches
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 5/9] box, datetime: messagepack support for datetime Timur Safin via Tarantool-patches
2021-08-03 13:38 ` Timur Safin via Tarantool-patches [this message]
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 6/9] box, datetime: datetime comparison for indices Timur Safin via Tarantool-patches
2021-08-03 12:02 ` Serge Petrenko via Tarantool-patches
2021-08-03 12:59 ` Timur Safin via Tarantool-patches
2021-08-04 10:12 ` Timur Safin via Tarantool-patches
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 7/9] lua, datetime: time intervals support Timur Safin via Tarantool-patches
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 8/9] datetime: changelog for datetime module Timur Safin via Tarantool-patches
2021-08-02 0:41 ` [Tarantool-patches] [PATCH v3 9/9] lua, box, datetime: rename struct datetime_t Timur Safin via Tarantool-patches
2021-08-06 0:27 ` Safin Timur via Tarantool-patches
2021-08-03 21:23 ` [Tarantool-patches] [PATCH v3 1/2] datetime: update tests for macosx Timur Safin via Tarantool-patches
2021-08-06 0:28 ` Safin Timur via Tarantool-patches
2021-08-03 21:23 ` [Tarantool-patches] [PATCH v3 2/2] lua, datetime: introduce ctime, strftime wrappers Timur Safin via Tarantool-patches
2021-08-06 0:30 ` Safin Timur via Tarantool-patches
2021-08-03 21:26 ` [Tarantool-patches] [PATCH v3 0/9] Initial datetime support Timur Safin via Tarantool-patches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='0ea601d7886c$e12e6a80$a38b3f80$@tarantool.org' \
--to=tarantool-patches@dev.tarantool.org \
--cc=olegrok@tarantool.org \
--cc=tsafin@tarantool.org \
--cc=v.shpilevoy@tarantool.org \
--subject='Re: [Tarantool-patches] [PATCH v3 5/9] box, datetime: messagepack support for datetime' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox