Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: imeevma@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values
Date: Sat, 20 Nov 2021 01:46:57 +0100	[thread overview]
Message-ID: <662f6b87-f085-ab10-53b8-d087d9598b19@tarantool.org> (raw)
In-Reply-To: <4ecfb3439688bef76c96270624410dee8822176f.1637244389.git.imeevma@gmail.com>

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.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index b598fe5c2..fe7029341 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -3043,6 +3043,45 @@ 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) {
> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> +				 mem_str(key), "integer, string or uuid");
> +			return NULL;
> +		}
> +		mem_to_mpstream(key, &stream);
> +		mem_to_mpstream(value, &stream);
> +	}
> +	mpstream_flush(&stream);
> +	if (is_error) {

2. The error could happen in the last moment after some allocations
were made. Better add a truncate here.

> +		diag_set(OutOfMemory, stream.pos - stream.buf,
> +			 "mpstream_flush", "stream");
> +		return NULL;
> +	}
> +	*size = region_used(region) - used;
> +	char *map = region_join(region, *size);
> +	if (map == NULL) {

3. Ditto. And the same in mem_encode_array().

> +		diag_set(OutOfMemory, *size, "region_join", "map");
> +		return NULL;
> +	}
> +	return map;
> +}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 06e6244e3..db7fef71a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1140,6 +1140,37 @@ expr(A) ::= LB(X) exprlist(Y) RB(E). {
>    spanSet(&A, &X, &E);
>  }
>  
> +expr(A) ::= LCB(X) maplist(Y) RCB(E). {
> +  struct sql *db = pParse->db;
> +  struct Expr *expr = sql_expr_new_dequoted(db, TK_MAP, NULL);

4. sql_expr_new_anon().

> +  if (expr == NULL) {
> +    sql_expr_list_delete(db, Y);
> +    pParse->is_aborted = true;
> +    return;
> +  }
> +  expr->x.pList = Y;
> +  expr->type = FIELD_TYPE_MAP;
> +  sqlExprSetHeightAndFlags(pParse, expr);
> +  A.pExpr = expr;
> +  spanSet(&A, &X, &E);
> +}
> +
> +maplist(A) ::= nmaplist(A).
> +maplist(A) ::= .                                  {A = 0;}

5. Lets remove these extra spaces between . and the code block.

> +nmaplist(A) ::= nmaplist(A) COMMA expr(X) COLON expr(Y). {
> +  A = sql_expr_list_append(pParse->db, A, X.pExpr);
> +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> +}
> +nmaplist(A) ::= expr(X) COLON expr(Y). {
> +  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
> +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> +}
> +
> +%type maplist {ExprList*}
> +%destructor maplist {sql_expr_list_delete(pParse->db, $$);}
> +%type nmaplist {ExprList*}
> +%destructor nmaplist {sql_expr_list_delete(pParse->db, $$);}

6. Could you please add a whitespace between type and '*'?


7. Lets add some more complicated syntax tests:

tarantool> box.execute('SELECT {:name}', {{[':name'] = 1}})
---
- null
- Syntax error at line 1 near '}'
...

tarantool> box.execute('SELECT {:name: 5}', {{[':name'] = 1}})
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{1: 5}]
...

tarantool> box.execute('SELECT {5::name}', {{[':name'] = 1}})
---
- metadata:
  - name: COLUMN_1
    type: map
  rows:
  - [{5: 1}]
...

To see if the parse is able to handle both map ':' and variable ':'.

  reply	other threads:[~2021-11-20  0:47 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 [this message]
2021-11-25  8:55     ` Mergen Imeev via Tarantool-patches
2021-11-30 22:04       ` Vladislav Shpilevoy via Tarantool-patches
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=662f6b87-f085-ab10-53b8-d087d9598b19@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