[Tarantool-patches] [PATCH v1 1/1] sql: introduce operator []

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Fri Dec 3 02:43:55 MSK 2021


Thanks for the patch!

>> 2. Why not error? This looks a bit controversial with how hard the rules
>> for explicit and implict casts are.
>>
> Since there is no good way to catch an error, we try to avoid them as much as
> possible.

But for implicit cast violations we raise a lot of errors.

>>>> +  if (expr == NULL) {
>>>> +    sql_expr_list_delete(pParse->db, Y);
>>>> +    pParse->is_aborted = true;
>>>> +    return;
>>>> +  }
>>>> +  Y = sql_expr_list_append(pParse->db, Y, X.pExpr);
>>>> +  expr->x.pList = Y;
>>>> +  expr->type = FIELD_TYPE_ANY;
>>>> +  sqlExprSetHeightAndFlags(pParse, expr);
>>>> +  A.pExpr = expr;
>>>> +  A.zStart = X.zStart;
>>>> +  A.zEnd = &E.z[E.n];
>>>> +}
>>
>> 8. I got an assertion fail.
>>
>> box.cfg{listen = 3313}
>> s = box.schema.create_space('TEST', {format = {{'ID'}, {'VALUE'}}})
>> _ = s:create_index('pk')
>> s:replace{1, {[{key = 100}] = 200}}
>> box.execute('SELECT value[1] FROM test')
>>
>> Assertion failed: (!mem_is_map(&mem) && !mem_is_array(&mem)), function mp_getitem, file /Users/gerold/Work/Repositories/tarantool/src/box/sql/mem.c, line 3136.
> I replaced assert with error. I did not write about this either in the rules or
> in the commit message, since I am not sure if it is correct. To find a key, I
> create a MEM from MP, so looking for MAP or ARRAY key is inconvenient. We can
> try to go the other way - from MEM to MP. In this case, we can use memcmp() to
> look up the ARRAY and MAP keys, although this would be a fairly shallow
> comparison.

I would just forget about it. Comparing arrays and maps might be
possible for == and !=, but it is not possible for other comparison
operators. Also comparing maps for == would be very expensive because
order of fields in 2 equal maps can be different. Hence you have
N^2 complexity.

See 4 comments below. Nothing serious.

1. Lets add a changelog file.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 7411b8f67..32b0c3fb9 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -3111,6 +3111,68 @@ mem_encode_map(const struct Mem *mems, uint32_t count, uint32_t *size,
>  	return map;
>  }
>  
> +static int
> +mp_getitem(const char **data, const struct Mem *key)
> +{
> +	if (mp_typeof(**data) != MP_ARRAY && mp_typeof(**data) != MP_MAP) {
> +		*data = NULL;
> +		return 0;
> +	}
> +	const char *end = *data;
> +	if (mp_typeof(**data) == MP_ARRAY) {
> +		uint32_t size = mp_decode_array(data);
> +		if (!mem_is_uint(key) || key->u.u == 0 || key->u.u > size) {
> +			*data = NULL;
> +			return 0;
> +		}
> +		for (uint32_t i = 0; i < key->u.u - 1; ++i)

2. This might be nitpicking but when you use an expression
in the loop condition, you risk to force is recalculation on
each iteration. Because when mp_next(data) is called, compiler
can't be sure data != &key. So it will recalculate key->u.u - 1
on the next iteration.

To avoid that it usually helps to save the value somewhere. For
instance:

	for (uint32_t i = 0, end = key->u.u - 1; i < end; ++i)
		mp_next(data);

or here it can be even simpler:

	for (uint32_t i = 1, end = key->u.u; i < end; ++i)
		mp_next(data);

Up to you.

> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 998acadea..d49920133 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -1099,6 +1100,32 @@ expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). {
>    sqlExprAttachSubtrees(pParse->db, A.pExpr, E.pExpr, 0);
>  }
>  
> +expr(A) ::= expr(X) LB getlist(Y) RB(E). {
> +  struct Expr *expr = sql_expr_new_anon(pParse->db, TK_GETITEM);
> +  if (expr == NULL) {
> +    sql_expr_list_delete(pParse->db, Y);
> +    pParse->is_aborted = true;
> +    return;
> +  }
> +  Y = sql_expr_list_append(pParse->db, Y, X.pExpr);

3. It might be better to introduce a function sql_expr_list_prepend.
Otherwise it is very counter-intuitive that the first expression
is on the last place, while the others are in correct places.

Or add a comment in expr_code_getitem().

Or save the first expression into expr->pLeft/pRight instead
of the index list.

> +  expr->x.pList = Y;
> +  expr->type = FIELD_TYPE_ANY;
> +  sqlExprSetHeightAndFlags(pParse, expr);
> +  A.pExpr = expr;
> +  A.zStart = X.zStart;
> +  A.zEnd = &E.z[E.n];
> +}
> +
> +getlist(A) ::= getlist(A) RB LB expr(X). {
> +  A = sql_expr_list_append(pParse->db, A, X.pExpr);
> +}
> +getlist(A) ::= expr(X). {
> +  A = sql_expr_list_append(pParse->db, NULL, X.pExpr);
> +}
> +
> +%type getlist {ExprList *}
> +%destructor getlist {sql_expr_list_delete(pParse->db, $$);}
> +
>  expr(A) ::= LB(X) exprlist(Y) RB(E). {
>    struct Expr *expr = sql_expr_new_anon(pParse->db, TK_ARRAY);
>    if (expr == NULL) {
4. In SQL NULL != anything else. But I managed to achieve this:

box.cfg{}
_ = box.schema.create_space('TEST', {format = {{name = 'F1', type = 'unsigned'}, {name = 'F2', type = 'map'}}}):create_index('pk')
box.space.TEST:replace{1, {[box.NULL] = 1}}

tarantool> box.execute('SELECT f2[NULL] FROM test')
---
- metadata:
  - name: COLUMN_1
    type: any
  rows:
  - [1]
...

Here NULL index = NULL key. Maybe it is correct. But it is worth
discussing with somebody to be sure. I can't tell how would we
get NULL keys if NULL = NULL wouldn't work here TBH. IS NULL
won't help.


More information about the Tarantool-patches mailing list