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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> You can specify that ‘opt’ is output param by [out] tag:
> @param[out] opt ...
- * @param opt pointer to store parsing result.
+ * @param[out] opt pointer to store parsing result.

> Mb it would be better to rename this struct according to our codestyle?
ExprList is a SQL structure not only for checks, so it's not a good idea to make a project-wide renaming. 
It is also not possible to rename it only in this place, because I don't like extra type casts thats are required in such scenario.

>> +void
>> +space_opts_destroy(struct space_opts *opts);
> 
> Seems like this function lacks some meaningful comment.
Here(in space_def.c) is just a function declaration since a definition is located in space_def.h and it match local code-style convention.  
> 
>> +++ 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.

> The same is here: you add checks for VIEW which are really aliases.
The VIEW checks are also represented as ExprList with name pointers but lacks Expr pointer.
So, this check is valid. 
The small comment bellow is about it:
/*
 * a[i].pExpr could be NULL for VIEW column names
 * represented as checks.
*/


> Comment isn’t consistent with code: if error is already
> set, why you set it again?
-               /* Error is already set with diag. */
+               /* Tarantool error may be already set with diag. */


> I would somehow rename this var, since it is easy to
> confuse one with things related to database indexes.
+++ b/src/box/sql.h
@@ -243,7 +243,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
 /**
  * Initialize check_list_item.
  * @param expr_list ExprList with item.
- * @param idx item index.
+ * @param column index.
  * @param expr_name expression name (optional).
  * @param expr_name_len expresson name length (optional).
  * @param expr_str expression to build string.
@@ -252,7 +252,7 @@ sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
  * @retval -1 on error.
  */
 int
-sql_check_list_item_init(struct ExprList *expr_list, int idx,
+sql_check_list_item_init(struct ExprList *expr_list, int column,
                         const char *expr_name, uint32_t expr_name_len,
                         const char *expr_str, uint32_t expr_str_len);

+++ b/src/box/sql.c
@@ -1788,12 +1788,12 @@ sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
 }
 
 int
-sql_check_list_item_init(struct ExprList *expr_list, int idx,
+sql_check_list_item_init(struct ExprList *expr_list, int column,
                         const char *expr_name, uint32_t expr_name_len,
                         const char *expr_str, uint32_t expr_str_len)
 {
-       assert(idx < expr_list->nExpr);
-       struct ExprList_item *item = &expr_list->a[idx];
+       assert(column < expr_list->nExpr);
+       struct ExprList_item *item = &expr_list->a[column];

> Why do we still need to create table wrappers?
> Is it possible to change signatures and use straight space_def instead of fake tables?
sql_resolve_self_reference initialize SrcList field pTab; 
Unfortunately sql_resolve_self_reference is not the only way of usage  SrcList and at least Table field pIndex is required in other scenarios.
Being unable to remove pTable pointer I've fellowed this way.