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

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

Thanks for the patch!

See 8 comments below.

>> @TarantoolBot document
>> Title: Operator [] in SQL
>>
>> Operator [] allows to get elements of MAP, ARRAY and ANY values.
>>
>> Rules for operator []:
>> 1) operator applied to the value to the left of `[` ("left-value");
>> 2) if left-value is not MAP, ARRAY or ANY, an error is thrown;
>> 3) if there is no values between `[` and `]`, left-value is returned as
>>    the result;

1. Why isn't it an error? Did I understand correctly that you mean []
with nothing inside?

>> 4) if there is one value between `[` and `]` ("right-value"), the result
>>    is:
>>   a) if left-value is ANY and its primitive type is not MAP or ARRAY,
>>      the result is NULL;

2. Why not error? This looks a bit controversial with how hard the rules
for explicit and implict casts are.

>>   b) if the type or primitive type of left-value is ARRAY, and
>>      right-value is INTEGER and its value is greater than 0 and not
>>      greater than the number of elements in ARRAY, the result will be a
>>      value with right-value as the index, otherwise the result will be
>>      NULL;

3. What if the right-value is not an integer?

>>   c) if the type or primitive type of left-value is MAP and it contains
>>      right-value as one of its keys, the result is the value with
>>      right-value as the key in left-value, otherwise the result is NULL;
>> 5) if there is more than one value between `[` and `]` than
>>    left-value[a, b, c, ...] == left-value[a][b][c]... except it will
>>    return NULL, if any of the `[]` operators return NULL.
>>
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index 789d8906c..abf47b8ef 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -3461,6 +3461,30 @@ expr_code_map(struct Parse *parser, struct Expr *expr, int reg)
>>  	sqlVdbeAddOp3(vdbe, OP_Map, count, reg, values_reg);
>>  }
>>  
>> +static void
>> +expr_code_getitem(struct Parse *parser, struct Expr *expr, int reg)

4. The same as for the array/map patches - should be sql_expr_emit_... or
expr_emit - depending on what prefix will you choose to use everywhere.

>> +{
>> +	struct Vdbe *vdbe = parser->pVdbe;
>> +	struct ExprList *list = expr->x.pList;
>> +	assert(list != NULL);
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index cfb88bffe..195dfde2b 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -3107,6 +3107,65 @@ 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)) {

5. Double (()). Need only one.

>> +		*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)
>> +			mp_next(data);
>> +		return 0;
>> +	}
>> +	struct Mem mem;
>> +	mem_create(&mem);
>> +	uint32_t size = mp_decode_map(data);
>> +	for (uint32_t i = 0; i < size; ++i) {
>> +		uint32_t len;
>> +		if (mem_from_mp_ephemeral(&mem, *data, &len) != 0)
>> +			return -1;

6. Lets add mem_is_trivial() assertion. Since you do not destroy the mem.

>> +		*data += len;
>> +		assert(!mem_is_map(&mem) && !mem_is_array(&mem));
>> +		if (mem_cmp_scalar(&mem, key, NULL) == 0)
>> +			return 0;
>> +		mp_next(data);
>> +	}
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index db7fef71a..30922b7f6 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1126,6 +1127,22 @@ 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 exprlist(Y) RB(E). {
>> +  struct Expr *expr = sql_expr_new_dequoted(pParse->db, TK_GETITEM, NULL);

7. sql_expr_new_anon().

>> +  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.