Tarantool development patches archive
 help / color / mirror / Atom feed
From: Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "'Mergen Imeev'" <imeevma@tarantool.org>,
	"'Vladislav Shpilevoy'" <v.shpilevoy@tarantool.org>
Cc: "TML" <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type
Date: Wed, 26 May 2021 01:58:41 +0300	[thread overview]
Message-ID: <158401d751b9$82223b00$8666b100$@tarantool.org> (raw)
In-Reply-To: <20210525141313.GA81084@tarantool.org>

There is one major issue I'd like to discuss, please see below...

: From: Mergen Imeev
: Subject: Re: [Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type
: 
...

: +static inline int
: +str_to_uuid(struct Mem *mem)
: +{
: +	assert(mem->type == MEM_TYPE_STR);
: +	struct tt_uuid uuid;
: +	if (mem->n != UUID_STR_LEN ||
: +	    tt_uuid_from_string(tt_cstr(mem->z, mem->n), &uuid) != 0)

Here lies the limitation I've specifically wanted to avoid and put
below to RFC:

"The most complete implementation seemingly is in PostgreSQL, 
which allows various relaxed formats for string literals 
which may be accepted as UUID - the plan is to be as close
as possible to PostgreSQL here."

Checking for UUID_STR_LEN restricts only to 1 specific form with
dashes in specific positions, but we would rather want to allow
relaxed formats also, with some dashes omitted. Also with optional
braces around. i.e. putting this as regexp

  \{?[0-9a-f]{8}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{4}-?[0-9a-f]{12}\}?

And yes, I mean we need to reimplement tt_uuid_from_string() differently.
(If you were asking me then I'd use RE2C for that purpose, but that up
to you)

: +		return -1;
: +	mem_set_uuid(mem, &uuid);
: +	return 0;
: +}
: +
:  static inline int
:  str_to_bool(struct Mem *mem)
:  {
...
: diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
: index 15d97da0e..b3cd5c545 100644
: --- a/src/box/sql/mem.h
: +++ b/src/box/sql/mem.h
: @@ -30,6 +30,7 @@
:   * SUCH DAMAGE.
:   */
:  #include "box/field_def.h"
: +#include "uuid/tt_uuid.h"
: 
:  struct sql;
:  struct Vdbe;
: @@ -47,10 +48,11 @@ enum mem_type {
:  	MEM_TYPE_MAP		= 1 << 6,
:  	MEM_TYPE_BOOL		= 1 << 7,
:  	MEM_TYPE_DOUBLE		= 1 << 8,
: -	MEM_TYPE_INVALID	= 1 << 9,
: -	MEM_TYPE_FRAME		= 1 << 10,
: -	MEM_TYPE_PTR		= 1 << 11,
: -	MEM_TYPE_AGG		= 1 << 12,
: +	MEM_TYPE_UUID		= 1 << 9,
: +	MEM_TYPE_INVALID	= 1 << 10,
: +	MEM_TYPE_FRAME		= 1 << 11,
: +	MEM_TYPE_PTR		= 1 << 12,
: +	MEM_TYPE_AGG		= 1 << 13,

I guess there should be no incompatibility problems here, but just
in case I ask - why did you insert new constant to the middle of prior list?

Also I see there is another omission from RFC - there is nothing added 
for SQL built-in support to generate UUID from within SQL mode.

  "Introduce UUID([version#]) functions which would allow generating
   any particular type of GUID. If version argument is omitted then
   we generate UUID v4 (as used by default in box);"

Regards,
Timur


  parent reply	other threads:[~2021-05-25 22:58 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20  9:44 Mergen Imeev via Tarantool-patches
2021-05-21  7:16 ` Mergen Imeev via Tarantool-patches
2021-05-22 16:04 ` Vladislav Shpilevoy via Tarantool-patches
2021-05-25 14:13   ` Mergen Imeev via Tarantool-patches
2021-05-25 21:50     ` Vladislav Shpilevoy via Tarantool-patches
2021-05-27 16:40       ` Mergen Imeev via Tarantool-patches
2021-05-25 22:58     ` Timur Safin via Tarantool-patches [this message]
2021-05-27 15:33       ` Mergen Imeev 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='158401d751b9$82223b00$8666b100$@tarantool.org' \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tsafin@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type' \
    /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