Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Mergen Imeev <imeevma@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
Date: Tue, 30 Nov 2021 23:04:47 +0100	[thread overview]
Message-ID: <f6554681-1d25-f48f-b1cd-a15be581ab39@tarantool.org> (raw)
In-Reply-To: <20211125085529.GB56448@tarantool.org>

Thanks for the fixes!

On 25.11.2021 09:55, Mergen Imeev wrote:
> Thank you for the review! My answers, diff and new patch below. Also, I added
> changelog and tests to show that it is possible to create an empty MAP and a
> map with more than 1000 key-value pairs.
> 
> On Sat, Nov 20, 2021 at 01:46:57AM +0100, Vladislav Shpilevoy wrote:
>> Thanks for the patch!
>>
>> See 7 comments below.
>>
>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>>> index 74a98c550..789d8906c 100644
>>> --- a/src/box/sql/expr.c
>>> +++ b/src/box/sql/expr.c
>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg)
>>>  	sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg);
>>>  }
>>>  
>>> +static void
>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
>>
>> 1. I thought the policy was that we name functions, generating VDBE code,
>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`.
>> Don't know about prefix though. I see both vdbe_ and sql_ are used.
>>
> This is usually true, but this function is actually part of sqlExprCodeTarget().
> I believe these functions were created to make sqlExprCodeTarget() more
> readable. All such functions are named sqlExprCode*(), code*() or
> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(),
> expr_code_int().
> 
> Since all these functions are static, I think we should drop "expr_" prefix for
> them. Not in this patch, though.

If functions take Expr as an argument like these do, they could be
considered methods of Expr. In that case dropping the expr_ prefix would
violate our naming convention. It is not about static or global here.

As an alternative they could be considered as methods of Parse, but
then they would need to have parse_ prefix.

For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But
if it goes public, then either 'code' or 'emit' must be chosen as one
correct suffix. Not a mix.

See 2 comments below.

> diff --git a/changelogs/unreleased/gh-4763-introduce-map-to-sql.md b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> new file mode 100644
> index 000000000..013ec8f67
> --- /dev/null
> +++ b/changelogs/unreleased/gh-4763-introduce-map-to-sql.md
> @@ -0,0 +1,4 @@
> +## feature/core

1. I noticed just now - it should be feature/sql, not core. In
other patches, which are not yet submitted, too. If there are
any similar mistakes.

> +
> + * Field type MAP is now available in SQL. The syntax has also been implemented
> +   to allow the creation of MAP values (gh-4763).> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 32b8825bc..7411b8f67 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -3070,6 +3070,47 @@ mem_encode_array(const struct Mem *mems, uint32_t count, uint32_t *size,
>  	return array;
>  }
>  
> +char *
> +mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
> +	       struct region *region)
> +{
> +	assert(count % 2 == 0);
> +	size_t used = region_used(region);
> +	bool is_error = false;
> +	struct mpstream stream;
> +	mpstream_init(&stream, region, region_reserve_cb, region_alloc_cb,
> +		      set_encode_error, &is_error);
> +	mpstream_encode_map(&stream, (count + 1) / 2);
> +	for (uint32_t i = 0; i < count / 2; ++i) {
> +		const struct Mem *key = &mems[2 * i];
> +		const struct Mem *value = &mems[2 * i + 1];
> +		if (mem_is_metatype(key) ||
> +		    (key->type & (MEM_TYPE_UINT | MEM_TYPE_INT | MEM_TYPE_UUID |
> +				  MEM_TYPE_STR)) == 0) {

2. Missed region truncate here. Looks like it would be easier to
add an 'error:' label in the end of the function to do the truncate
and return NULL.

> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(key), "integer, string or uuid");
> +			return NULL;
> +		}

  reply	other threads:[~2021-11-30 22:04 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-18 14:08 [Tarantool-patches] [PATCH v1 0/2] Introduce syntax for MAP values is SQL Mergen Imeev via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 1/2] sql: properly check bind variable names Mergen Imeev via Tarantool-patches
2021-11-20  0:45   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:33     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:02       ` Vladislav Shpilevoy via Tarantool-patches
2021-12-02  8:32         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:34             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:47               ` Vladislav Shpilevoy via Tarantool-patches
2021-11-18 14:08 ` [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values Mergen Imeev via Tarantool-patches
2021-11-20  0:46   ` Vladislav Shpilevoy via Tarantool-patches
2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-12-02  8:38         ` Mergen Imeev via Tarantool-patches
2021-12-09  0:31           ` Vladislav Shpilevoy via Tarantool-patches
2021-12-13  7:42             ` Mergen Imeev via Tarantool-patches
2021-12-13 21:48               ` Vladislav Shpilevoy 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=f6554681-1d25-f48f-b1cd-a15be581ab39@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values' \
    /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