[Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values

[Vladislav Shpilevoy]

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 ':'.