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

[n.pettik]

> On 30 May 2018, at 11:32, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
>> 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. 

But sooner or later we will have to do it. Anyway, as you wish.

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

I extremely dislike this original SQLite approach, since it is so confusing.
I suggest to rework it somehow. I have dived into this problem.
Now aliases are stored as checks only for
one reason: to tell whether SELECT of view is resolved or not.
If not, aliases are extracted from checks list and put into regular fields
array. Thus, if we store aliases as fields right after view creation,
we can’t point out whether we have resolved names for this view,
or it is just view with aliases.

But since point of this patch slightly different, lets leave it as it is.

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

Well, l see now. Seems it can be fixed after introducing index_def in struct Index.