Re: [Tarantool-patches] [PATCH v1 3/3] sql: allow to bind uuid values

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 05.07.2021 17:06, Mergen Imeev via Tarantool-patches wrote:
> After this patch, uuid values can be binded like any other supported by

1. binded -> bound.

> diff --git a/src/box/bind.h b/src/box/bind.h
> index 568c558f3..20f3e7942 100644
> --- a/src/box/bind.h
> +++ b/src/box/bind.h
> @@ -40,6 +40,8 @@ extern "C" {
>  #include <stdlib.h>
>  
>  #include "msgpuck.h"
> +#include "uuid/tt_uuid.h"
> +#include "lib/core/mp_extension_types.h"

2. lib/core is already in the paths. You can omit it.

> diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
> index aaae12e41..a9700350d 100644
> --- a/src/box/sql/vdbeapi.c
> +++ b/src/box/sql/vdbeapi.c
> @@ -840,6 +840,17 @@ sql_bind_zeroblob64(sql_stmt * pStmt, int i, sql_uint64 n)
>  	return sql_bind_zeroblob(pStmt, i, n);
>  }
>  
> +int
> +sql_bind_uuid(struct sql_stmt *stmt, int i, const struct tt_uuid *uuid)
> +{
> +	struct Vdbe *p = (struct Vdbe *)stmt;
> +	if (vdbeUnbind(p, i) != 0)
> +		return -1;
> +	int rc = sql_bind_type(p, i, "uuid");
> +	mem_set_uuid(&p->aVar[i - 1], uuid);

3. Why do you set UUID even if bind_type() failed?

> diff --git a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
> index 8872f9f23..426717972 100755
> --- a/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
> +++ b/test/sql-tap/gh-6164-uuid-follow-ups.test.lua
> @@ -32,4 +32,18 @@ test:do_execsql_test(
>          true
>      })
>  
> +-- Make sure that uuid value can be binded.
> +box.execute('CREATE TABLE t(i INT PRIMARY KEY, a UUID);')
> +box.execute('INSERT INTO t VALUES(1, ?);', {uuid});

4. Do you need a table for that? Can you just make 'SELECT ?' with
the uuid argument?

> +
> +test:do_execsql_test(
> +    "gh-6164-4",
> +    [[
> +        SELECT * FROM t;
> +    ]], {
> +        1, uuid
> +    })
> +
> +box.execute([[DROP TABLE t;]])
> +
>  test:finish_test()
>