[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