[tarantool-patches] Re: [PATCH] sql: refactor SQL delete to allow Lua spaces
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon May 21 13:49:31 MSK 2018
On 21/05/2018 13:25, Kirill Yukhin wrote:
> On 21 мая 12:47, Vladislav Shpilevoy wrote:
>> Hello. Thanks for the fixes!
>>
>>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>>> index df75186..68f81e8 100644
>>> --- a/src/box/sql/build.c
>>> +++ b/src/box/sql/build.c
>>> @@ -2108,107 +2108,97 @@ sqlite3CreateView(Parse * pParse, /* The parsing context */
>>> -int
>>> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
>>> +bool
>>
>> In Tarantool code style we do not return boolean on non-check functions.
>> 0 or -1, please.
> Done.
No(( Tests are failing. See 5 comments below, why.
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index df75186..f59087b 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> int
> -sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
> +sql_view_column_names(struct Parse *parse, struct Table *table)
> {
> - Table *pSelTab; /* A fake table from which we get the result set */
> - Select *pSel; /* Copy of the SELECT that implements the view */
> - int nErr = 0; /* Number of errors encountered */
> - int n; /* Temporarily holds the number of cursors assigned */
> - sqlite3 *db = pParse->db; /* Database connection for malloc errors */
> -
> - assert(pTable);
> -
> -#ifndef SQLITE_OMIT_VIEW
> - /* A positive nCol means the columns names for this view are
> - * already known.
> + assert(table != NULL);
> + assert(space_is_view(table));
> + /* A positive nCol means the columns names for this view
> + * are already known.
> */
> - if (pTable->nCol > 0)
> - return 0;
> + if (table->nCol > 0)
> + return true;
1. Here you return 'true', but returned type is int. It is converted to 1,
and this function is treated as failed.
>
> - /* A negative nCol is a special marker meaning that we are currently
> - * trying to compute the column names. If we enter this routine with
> - * a negative nCol, it means two or more views form a loop, like this:
> + /* A negative nCol is a special marker meaning that we are
> + * currently trying to compute the column names. If we
> + * enter this routine with a negative nCol, it means two
> + * or more views form a loop, like this:
> *
> * CREATE VIEW one AS SELECT * FROM two;
> * CREATE VIEW two AS SELECT * FROM one;
> *
> - * Actually, the error above is now caught prior to reaching this point.
> - * But the following test is still important as it does come up
> - * in the following:
> + * Actually, the error above is now caught prior to
> + * reaching this point. But the following test is still
> + * important as it does come up in the following:
> *
> * CREATE TABLE main.ex1(a);
> * CREATE TEMP VIEW ex1 AS SELECT a FROM ex1;
> * SELECT * FROM temp.ex1;
> */
> - if (pTable->nCol < 0) {
> - sqlite3ErrorMsg(pParse, "view %s is circularly defined",
> - pTable->zName);
> - return 1;
> - }
> - assert(pTable->nCol >= 0);
> -
> - /* If we get this far, it means we need to compute the table names.
> - * Note that the call to sqlite3ResultSetOfSelect() will expand any
> - * "*" elements in the results set of the view and will assign cursors
> - * to the elements of the FROM clause. But we do not want these changes
> - * to be permanent. So the computation is done on a copy of the SELECT
> - * statement that defines the view.
> + if (table->nCol < 0) {
> + sqlite3ErrorMsg(parse, "view %s is circularly defined",
> + table->zName);
> + return false;
2. Same.
> + }
> + assert(table->nCol >= 0);
> +
> + /* If we get this far, it means we need to compute the
> + * table names. Note that the call to
> + * sqlite3ResultSetOfSelect() will expand any "*" elements
> + * in the results set of the view and will assign cursors
> + * to the elements of the FROM clause. But we do not want
> + * these changes to be permanent. So the computation is
> + * done on a copy of the SELECT statement that defines the
> + * view.
> */
> - assert(pTable->pSelect);
> - pSel = sqlite3SelectDup(db, pTable->pSelect, 0);
> - if (pSel) {
> - n = pParse->nTab;
> - sqlite3SrcListAssignCursors(pParse, pSel->pSrc);
> - pTable->nCol = -1;
> - db->lookaside.bDisable++;
> - pSelTab = sqlite3ResultSetOfSelect(pParse, pSel);
> - pParse->nTab = n;
> - if (pTable->pCheck) {
> - /* CREATE VIEW name(arglist) AS ...
> - * The names of the columns in the table are taken from
> - * arglist which is stored in pTable->pCheck. The pCheck field
> - * normally holds CHECK constraints on an ordinary table, but for
> - * a VIEW it holds the list of column names.
> - */
> - sqlite3ColumnsFromExprList(pParse, pTable->pCheck,
> - &pTable->nCol,
> - &pTable->aCol);
> - if (db->mallocFailed == 0 && pParse->nErr == 0
> - && pTable->nCol == pSel->pEList->nExpr) {
> - sqlite3SelectAddColumnTypeAndCollation(pParse,
> - pTable,
> - pSel);
> - }
> - } else if (pSelTab) {
> - /* CREATE VIEW name AS... without an argument list. Construct
> - * the column names from the SELECT statement that defines the view.
> - */
> - assert(pTable->aCol == 0);
> - pTable->nCol = pSelTab->nCol;
> - pTable->aCol = pSelTab->aCol;
> - pSelTab->nCol = 0;
> - pSelTab->aCol = 0;
> - } else {
> - pTable->nCol = 0;
> - nErr++;
> + assert(table->pSelect != NULL);
> + sqlite3 *db = parse->db;
> + struct Select *select = sqlite3SelectDup(db, table->pSelect, 0);
> + if (select == NULL)
> + return false;
3. Same.
> + int n = parse->nTab;
> + sqlite3SrcListAssignCursors(parse, select->pSrc);
> + table->nCol = -1;
> + db->lookaside.bDisable++;
> + struct Table *sel_tab = sqlite3ResultSetOfSelect(parse, select);
> + parse->nTab = n;
> + bool is_ok = true;
4. is_ok -> rc.
> + if (table->pCheck != NULL) {
> + /* CREATE VIEW name(arglist) AS ...
> + * The names of the columns in the table are taken
> + * from arglist which is stored in table->pCheck.
> + * The pCheck field normally holds CHECK
> + * constraints on an ordinary table, but for a
> + * VIEW it holds the list of column names.
> + */
> + sqlite3ColumnsFromExprList(parse, table->pCheck,
> + &table->nCol,
> + &table->aCol);
> + if (db->mallocFailed == 0 && parse->nErr == 0
> + && table->nCol == select->pEList->nExpr) {
> + sqlite3SelectAddColumnTypeAndCollation(parse, table,
> + select);
> }
> - sqlite3DeleteTable(db, pSelTab);
> - sqlite3SelectDelete(db, pSel);
> - db->lookaside.bDisable--;
> + } else if (sel_tab != NULL) {
> + /* CREATE VIEW name AS... without an argument
> + * list. Construct the column names from the
> + * SELECT statement that defines the view.
> + */
> + assert(table->aCol == NULL);
> + table->nCol = sel_tab->nCol;
> + table->aCol = sel_tab->aCol;
> + sel_tab->nCol = 0;
> + sel_tab->aCol = NULL;
> } else {
> - nErr++;
> + table->nCol = 0;
> + is_ok = false;
> }
> -#endif /* SQLITE_OMIT_VIEW */
> - return nErr;
> + sqlite3DeleteTable(db, sel_tab);
> + sqlite3SelectDelete(db, select);
> + db->lookaside.bDisable--;
> +
> + return is_ok;
5. Same as 1 - 3.
More information about the Tarantool-patches
mailing list