[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server

["n.pettik" <korablev@tarantool.org>]

>sql: remove Checks to server

I guess, it would be better to use word ‘move’...

> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 9e0e7e3..7bff86f 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -33,17 +33,33 @@
> #include "diag.h"
> #include "error.h"
> #include "sql.h"
> +#include "msgpuck.h"
> +
> +/**
> + * Make checks from msgpack.
> + * @param str pointer to array of maps
> + *         e.g. [{"expr_str": "x < y", "name": "ONE"}, ..].
> + * @param len array items count.
> + * @param opt pointer to store parsing result.

You can specify that ‘opt’ is output param by [out] tag:
@param[out] opt ...

>+++ b/src/box/space_def.h
>@@ -40,6 +40,8 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
> 
>+struct ExprList;
>+

Mb it would be better to rename this struct according to our codestyle?

>@@ -79,12 +81,8 @@ space_opts_create(struct space_opts *opts)
> /**
>  * Destroy space options
>  */
>-static inline void
>-space_opts_destroy(struct space_opts *opts)
>-{
>-       free(opts->sql);
>-       TRASH(opts);
>-}
>+void
>+space_opts_destroy(struct space_opts *opts);

Seems like this function lacks some meaningful comment.

>+++ b/src/box/sql.c
>@@ -1500,22 +1500,47 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>  *
>  * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
>  */
>-int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
>+int
>+tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
> {
>        const struct Enc *enc = get_enc(buf);
>-       char *base = buf, *p;
>+       bool is_view = pTable != NULL && pTable->def->opts.is_view;
>+       bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;

In fact, it is incorrect since opts.checks may also contain aliases for VIEW
columns: CREATE VIEW V1(a, b, c) AS SELECT * FROM t1;
In this case, checks contain NULL exprs with names ‘A’, ‘B’ and ‘C’.
So you should add another one condition: && is_view.

>+               if (a[i].zName != NULL) {
>+                       p = enc->encode_str(p, "name", 4);
>+                       p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
>+               }

The same is here: you add checks for VIEW which are really aliases.

>+int
>+sql_checks_resolve_space_def_reference(ExprList *expr_list,
>+                                      struct space_def *def)
>+{
>+       Parse parser;
>+       sql_parser_create(&parser, sql_get());
>+       parser.parse_only = true;
>+
>+       Table dummy_table;
>+       memset(&dummy_table, 0, sizeof(dummy_table));
>+       dummy_table.def = def;
>+
>+       sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+                                  expr_list);
>+
>+       sql_parser_destroy(&parser);
>+       if (parser.rc != SQLITE_OK) {
>+               /* Error is already set with diag. */
>+               if (parser.rc != SQL_TARANTOOL_ERROR)
>+                       diag_set(ClientError, ER_SQL, parser.zErrMsg);

Comment isn’t consistent with code: if error is already
set, why you set it again?

>@@ -232,6 +240,42 @@ void
> sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
>                           struct Expr *expr, struct ExprList *expr_list);
> 
>+/**
>+ * Initialize check_list_item.
>+ * @param expr_list ExprList with item.
>+ * @param idx item index.

I would somehow rename this var, since it is easy to
confuse one with things related to database indexes.

>+int
>+sql_checks_resolve_space_def_reference(ExprList *expr_list,
>+                                      struct space_def *def)
>+{
>+       Parse parser;
>+       sql_parser_create(&parser, sql_get());
>+       parser.parse_only = true;
>+
>+       Table dummy_table;
>+       memset(&dummy_table, 0, sizeof(dummy_table));
>+       dummy_table.def = def;
>+
>+       sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+                                  expr_list);

Why do we still need to create table wrappers?
Is it possible to change signatures and use straight space_def instead of fake tables?