From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 005032A83B for ; Tue, 26 Mar 2019 13:08:52 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id x4dqxwe-jfyG for ; Tue, 26 Mar 2019 13:08:52 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 605D028768 for ; Tue, 26 Mar 2019 13:08:52 -0400 (EDT) From: Vladislav Shpilevoy Subject: [tarantool-patches] Re: [PATCH v2 6/7] sql: refactor sql_expr_create to set diag References: <645e4a4d0e2ddeaf747fe8a11affb52cb3157df3.1551265819.git.kshcherbatov@tarantool.org> <30714d52-a753-0d00-b28d-d05a1fd2a623@tarantool.org> <23e9426a-d4f3-7954-f342-6a5c738d1cca@tarantool.org> <12d01358-2f48-0b26-9560-d7f274de3f0f@tarantool.org> <750aaaa8-7ab9-5d0f-38c2-6782ed90db6f@tarantool.org> Message-ID: Date: Tue, 26 Mar 2019 20:08:49 +0300 MIME-Version: 1.0 In-Reply-To: <750aaaa8-7ab9-5d0f-38c2-6782ed90db6f@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov Thanks for the fixes! Firstly, please, append that to the commit message: " After that patch there are basically 2 ways of errors processing and forwarding: - Use diag only. It works for all the places out of src/box/sql, and for some functions inside it. For example, sql_expr_new(); - Use global flags Parse.is_aborted, sql.mallocFailed. It is easy to see, that some places use both of them implicitly. For example, sql_expr_new() and every other place which uses SQLite memory allocators + diag. But it is ok until the former is removed. What is more important, is that at least one of these two methods should be consistent and finished in every functions. And match a declared behaviour. For example, it is incorrect to declare a function as setting flags on an error, but in fact set diag only. Or vice versa, that it throws a diag, but factually sets flags only. " Also see my comments below, fixes at the end of the email and on the branch. While previous commits I've fixed silently, there I have something to say. > Refactored sqlExpr routine as sql_expr_new and reworked it to set > diag message in case of memory allocation error. Also performed some > additional name refactoring in adjacent places. > This change is necessary because the sqlExpr body has a > sqlNormalizeName call that will be changed in subsequent patches. > > Part of #3931 > --- > src/box/sql/build.c | 42 ++++-- > src/box/sql/expr.c | 248 ++++++++++++++---------------------- > src/box/sql/fk_constraint.c | 190 +++++++++++++++++---------- > src/box/sql/parse.y | 51 ++++++-- > src/box/sql/resolve.c | 46 ++++--- > src/box/sql/select.c | 86 +++++++++---- > src/box/sql/sqlInt.h | 84 +++++++++++- > src/box/sql/wherecode.c | 8 +- > src/box/sql/whereexpr.c | 21 +-- > 9 files changed, 471 insertions(+), 305 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index f52cfd7dd..f527928bf 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c> @@ -1383,13 +1388,19 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name, > struct Expr *where = NULL; > if (idx_name != NULL) { > struct Expr *expr = sql_id_eq_str_expr(parse, "idx", idx_name); > - if (expr != NULL) > - where = sqlExprAnd(db, expr, where); > + if (expr != NULL && (expr != NULL || where != NULL)) { 1. This is what I meant and asked you to do - *self review*. Accurate, attentive, diligent labor. But look at that condition: - You check 'expr' twice. If 'expr' != NULL first time, then the second time nothing has changed. What is more, the whole second part can be removed then; - 3 lines above 'where' is declared as NULL. And here you check if it is NULL. It is not hard to find such mistakes. It requires just one single self review, which usually removes most of such things. Exactly the same about the code below. Also, a couple of words about that pattern, that you obviously just copy-pasted throughout the patch and got pearls like above: if (a != NULL || b != NULL) { c = sql_and_expr_new(a, b); if (c != NULL) is_aborted = true } It is bulky, hard to read and can be avoided. It appeared because you forbid to pass NULL NULL into sql_and_expr_new. But why? Because in such a case it returns NULL, but there is no an error? Usually it means exactly an error, and only a couple of places require such a check. Which can be done out of sql_and_expr_new by the way. > + where = sql_and_expr_new(db, expr, where); > + if (where == NULL) > + parse->is_aborted = true; > + } > } > if (table_name != NULL) { > struct Expr *expr = sql_id_eq_str_expr(parse, "tbl", table_name); > - if (expr != NULL) > - where = sqlExprAnd(db, expr, where); > + if (expr != NULL && (expr != NULL || where != NULL)) { > + where = sql_and_expr_new(db, expr, where); > + if (where == NULL) > + parse->is_aborted = true; 2. Above you have an assertion that either table_name != NULL, or idx_name != NULL, or both. It means, that in a normal case 'where' != NULL at the end of the function. And if it is NULL, then is_aborted can be set. Out of these 'if's. No code duplication. > + } > } > /** > * On memory allocation error sql_table delete_from > @@ -2245,9 +2256,12 @@ sql_create_index(struct Parse *parse, struct Token *token, > struct Token prev_col; > uint32_t last_field = def->field_count - 1; > sqlTokenInit(&prev_col, def->fields[last_field].name); > - col_list = sql_expr_list_append(parse->db, NULL, > - sqlExprAlloc(db, TK_ID, > - &prev_col, 0)); > + struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col, false); > + if (expr == NULL) { > + parse->is_aborted = true; > + goto exit_create_index; > + } > + col_list = sql_expr_list_append(parse->db, NULL, expr); 3. You used 'db' 5 lines above. I see, that it was in the old code, but it is not an excuse. Now it is your code, and it should be proper. > if (col_list == NULL) > goto exit_create_index; > assert(col_list->nExpr == 1); > diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c > index cc4dc6448..3b27b2d4f 100644 > --- a/src/box/sql/fk_constraint.c > +++ b/src/box/sql/fk_constraint.c > @@ -303,37 +303,36 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent, > > /** > * Return an Expr object that refers to a memory register > - * corresponding to column iCol of given space. > + * corresponding to column of given space. > * > - * regBase is the first of an array of register that contains > - * the data for given space. regBase+1 holds the first column. > - * regBase+2 holds the second column, and so forth. > + * reg_base is the first of an array of register that contains > + * the data for given space. reg_base+1 holds the first column. > + * reg_base+2 holds the second column, and so forth. > * > - * @param pParse Parsing and code generating context. > + * @param parser The parsing context. > * @param def Definition of space whose content is at r[regBase]... > - * @param regBase Contents of table defined by def. > - * @param iCol Which column of space is desired. > - * @return an Expr object that refers to a memory register > - * corresponding to column iCol of given space. > + * @param reg_base Contents of table table. > + * @param column Index of table column is desired. > + * @retval Not NULL expression pointer on success. > + * @retval NULL Otherwise. 4. Sorry. But sometimes I think, that probably you are kidding. In the previous commits I after all fixed that myself in scope of 'Review fixes'. But since here I do a detailed review, not just a silent fix, I will say that again: - Start sentences from a capital letter; - Do not profane comments writing. You write them just to write anything, but it should not be so. I asked you to rewrite places like this: /** * @retval Not NULL my_struct pointer on success. */ struct my_struct * func(...) Because the information you have provided in that comment is useless. Even without the comment I see, that this function returns a my_struct pointer. The same about sql_register_expr_new - I see, that it returns struct Expr pointer. C language guarantees that. And it is not only about that place. Here it is simple. But sometimes a function can return not NULL on both error and success. And then you can not say 'it returns a struct Expr pointer'. Or vice versa - return NULL on both success and error, like sql_and_expr_new. - Explicitly say if a function sets diagnostics area message. In SQL we are in a middle of two phases: when all the functions set diag, and when no one. This concrete function sets diag on any error. > */ > -static Expr * > -space_field_register(Parse *pParse, struct space_def *def, int regBase, > - i16 iCol) > +static struct Expr * > +sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base, > + int column)> { > - Expr *pExpr; > - sql *db = pParse->db; > - > - pExpr = sqlExpr(db, TK_REGISTER, 0); > - if (pExpr) { > - if (iCol >= 0) { > - pExpr->iTable = regBase + iCol + 1; > - pExpr->type = def->fields[iCol].type; > - } else { > - pExpr->iTable = regBase; > - pExpr->type = FIELD_TYPE_INTEGER; > - } > + struct Expr *expr = sql_op_expr_new(parser->db, TK_REGISTER, NULL); > + if (expr == NULL) { > + parser->is_aborted = true; > + return NULL; > + } > + if (column >= 0) { 5. When I first saw that check I wondered how is it possible that a column number is negative? Appeared that there is no way for that. I've added an assertion about column >= 0 and the tests passed. What is more, in all places where that function is called, it takes uint32_t. So this check and the 'else' body are not necessary. > + expr->iTable = reg_base + column + 1; > + expr->type = def->fields[column].type; > + } else { > + expr->iTable = reg_base; > + expr->type = FIELD_TYPE_INTEGER; > } > - return pExpr; > + return expr; > } > > /** > @@ -435,12 +435,18 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src, > for (uint32_t i = 0; i < fk_def->field_count; i++) { > uint32_t fieldno = fk_def->links[i].parent_field; > struct Expr *pexpr = > - space_field_register(parser, def, reg_data, fieldno); > + sql_register_expr_new(parser, def, reg_data, fieldno); > fieldno = fk_def->links[i].child_field; > const char *field_name = child_space->def->fields[fieldno].name; > - struct Expr *chexpr = sqlExpr(db, TK_ID, field_name); > + struct Expr *chexpr = sql_op_expr_new(db, TK_ID, field_name); > + if (chexpr == NULL) > + parser->is_aborted = true; > struct Expr *eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr); > - where = sqlExprAnd(db, where, eq); > + if (where != NULL || eq != NULL) { 6. You do not need that check here, because if eq == NULL, then pexpr and cheexpr are nulls. If they are nulls, then it was an error, and it is ok to set is_aborted. Even if it is already set. The same in many other places. Of course, after revert of sql_and_expr_new asserting on NULL NULL. > + where = sql_and_expr_new(db, where, eq); > + if (where == NULL) > + parser->is_aborted = true; > + } > } > > /*> @@ -785,14 +802,26 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, > * type and collation sequence associated with > * the parent table are used for the comparison. > */ > - struct Expr *to_col = > - sqlPExpr(pParse, TK_DOT, > - sqlExprAlloc(db, TK_ID, &t_old, 0), > - sqlExprAlloc(db, TK_ID, &t_to_col, 0)); > - struct Expr *from_col = > - sqlExprAlloc(db, TK_ID, &t_from_col, 0); > - struct Expr *eq = sqlPExpr(pParse, TK_EQ, to_col, from_col); > - where = sqlExprAnd(db, where, eq); > + struct Expr *old_expr = NULL, *new_expr = NULL, *expr = NULL; > + old_expr = sql_expr_new(db, TK_ID, &t_old, false); > + if (old_expr == NULL) > + pParse->is_aborted = true; > + expr = sql_expr_new(db, TK_ID, &t_to_col, false); > + if (expr == NULL) > + pParse->is_aborted = true; > + struct Expr *from_col_expr = > + sql_expr_new(db, TK_ID, &t_from_col, false); > + if (from_col_expr == NULL) > + pParse->is_aborted = true; > + struct Expr *to_col_expr = > + sqlPExpr(pParse, TK_DOT, old_expr, expr); > + struct Expr *eq = > + sqlPExpr(pParse, TK_EQ, to_col_expr, from_col_expr); > + if (where != NULL || eq != NULL) { > + where = sql_and_expr_new(db, where, eq); > + if (where == NULL) > + pParse->is_aborted = true; > + } 7. Talking of this hunk and the hunk below - I do not know what to say. Even before the patch it looked cursed and corrupted, but now it is even worse. Much worse. The code padded out in 2.5 times. Also, it is in fact repeated here and below. Fully repeated with just different variable names. I've encapsulated most of these into a new function sql_expr_new_2part_id() and batched checks for expr == NULL to set aborts. > > /* > * For ON UPDATE, construct the next term of the > @@ -810,12 +839,22 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, > * no_action_needed(colN)) > */ > if (is_update) { > + old_expr = sql_expr_new(db, TK_ID, &t_old, false); > + if (old_expr == NULL) > + pParse->is_aborted = true; > + expr = sql_expr_new(db, TK_ID, &t_to_col, false); > + if (expr == NULL) > + pParse->is_aborted = true; > struct Expr *old_val = sqlPExpr(pParse, TK_DOT, > - sqlExprAlloc(db, TK_ID, &t_old, 0), > - sqlExprAlloc(db, TK_ID, &t_to_col, 0)); > + old_expr, expr); > + new_expr = sql_expr_new(db, TK_ID, &t_new, false); > + if (new_expr == NULL) > + pParse->is_aborted = true; > + expr = sql_expr_new(db, TK_ID, &t_to_col, false); > + if (expr == NULL) > + pParse->is_aborted = true; > struct Expr *new_val = sqlPExpr(pParse, TK_DOT, > - sqlExprAlloc(db, TK_ID, &t_new, 0), > - sqlExprAlloc(db, TK_ID, &t_to_col, 0)); > + new_expr, expr); > struct Expr *old_is_null = sqlPExpr( > pParse, TK_ISNULL, > sqlExprDup(db, old_val, 0), NULL); > @@ -828,29 +867,41 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, > if (action != FKEY_ACTION_RESTRICT && > (action != FKEY_ACTION_CASCADE || is_update)) { > struct Expr *new, *d; > if (action == FKEY_ACTION_CASCADE) { > - new = sqlPExpr(pParse, TK_DOT, > - sqlExprAlloc(db, TK_ID, > - &t_new, 0), > - sqlExprAlloc(db, TK_ID, > - &t_to_col, > - 0)); > + new_expr = sql_expr_new(db, TK_ID, &t_new, > + false); > + if (new_expr == NULL) > + pParse->is_aborted = true; > + expr = sql_expr_new(db, TK_ID, &t_to_col, > + false); > + if (expr == NULL) > + pParse->is_aborted = true; > + new = sqlPExpr(pParse, TK_DOT, new_expr, expr); > } else if (action == FKEY_ACTION_SET_DEFAULT) { > d = child_fields[chcol].default_value_expr; > if (d != NULL) { > new = sqlExprDup(db, d, 0); > } else { > - new = sqlExprAlloc(db, TK_NULL, > - NULL, 0); > + new = sql_expr_new(db, TK_NULL, NULL, > + false); > + if (new == NULL) > + pParse->is_aborted = true; > } > } else { > - new = sqlExprAlloc(db, TK_NULL, NULL, 0); > + new = sql_expr_new(db, TK_NULL, NULL, false); > + if (new == NULL) > + pParse->is_aborted = true; > } 8. Exactly the same about this code. Before the patch is was not the best, but good enough because of low code duplication. But now the last branch and the branch 'action == FKEY_ACTION_SET_DEFAULT && d == NULL' are too big and still do the same. I've reworked that to remove the duplication. > list = sql_expr_list_append(db, list, new); > sqlExprListSetName(pParse, list, &t_from_col, 0);> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y > index f71990dee..4243e799c 100644 > --- a/src/box/sql/parse.y > +++ b/src/box/sql/parse.y > @@ -898,15 +906,28 @@ term(A) ::= NULL(X). {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/} > expr(A) ::= id(X). {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/} > expr(A) ::= JOIN_KW(X). {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/} > expr(A) ::= nm(X) DOT nm(Y). { > - Expr *temp1 = sqlExprAlloc(pParse->db, TK_ID, &X, 1); > - Expr *temp2 = sqlExprAlloc(pParse->db, TK_ID, &Y, 1); > + struct Expr *temp1 = sql_expr_new(pParse->db, TK_ID, &X, true); > + if (temp1 == NULL) { > + pParse->is_aborted = true; > + return; > + } > + struct Expr *temp2 = sql_expr_new(pParse->db, TK_ID, &Y, true); > + if (temp2 == NULL) { > + sql_expr_delete(pParse->db, temp2, false); 9. temp2 is NULL. You checked it one line above. It should be temp1. > + pParse->is_aborted = true; > + return; > + }> diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 1b7d52b68..5bba0b5d0 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -497,8 +498,12 @@ addWhereTerm(Parse * pParse, /* Parsing context */ > assert(pSrc->a[iLeft].space != NULL); > assert(pSrc->a[iRight].space != NULL); > > - pE1 = sqlCreateColumnExpr(db, pSrc, iLeft, iColLeft); > - pE2 = sqlCreateColumnExpr(db, pSrc, iRight, iColRight); > + pE1 = sql_column_expr_new(pParse->db, pSrc, iLeft, iColLeft); > + if (pE1 == NULL) > + pParse->is_aborted = true; > + pE2 = sql_column_expr_new(pParse->db, pSrc, iRight, iColRight); > + if (pE2 == NULL) > + pParse->is_aborted = true; 10. You could check for both pE1 and pE2 == NULL in one 'if' using '||'. > > pEq = sqlPExpr(pParse, TK_EQ, pE1, pE2); > if (pEq && isOuterJoin) { > @@ -628,8 +637,13 @@ sqlProcessJoin(Parse * pParse, Select * p) > if (pRight->pOn) { > if (isOuter) > setJoinExpr(pRight->pOn, pRight->iCursor); > - p->pWhere = > - sqlExprAnd(pParse->db, p->pWhere, pRight->pOn); > + if (p->pWhere != NULL || pRight->pOn != NULL) { 11. One another example why self-review is necessary. pRight->pOn is already checked for not being equal NULL 2 lines above. It makes your 'if' useless. > + p->pWhere = > + sql_and_expr_new(pParse->db, p->pWhere, > + pRight->pOn); > + if (p->pWhere == NULL) > + pParse->is_aborted = true; > + } > pRight->pOn = 0; > } > > @@ -4324,8 +4346,13 @@ pushDownWhereTerms(Parse * pParse, /* Parse context (for malloc() and error repo > while (pSubq) { > pNew = sqlExprDup(pParse->db, pWhere, 0); > pNew = substExpr(pParse, pNew, iCursor, pSubq->pEList); > - pSubq->pWhere = > - sqlExprAnd(pParse->db, pSubq->pWhere, pNew); > + if (pSubq->pWhere != NULL || pNew != NULL) { 12. If pNew is NULL, then it is an error, because pWhere is not NULL, it is guaranteed by the code above (implicitly via attributes access, for example). In such a case pNew NULL and pSubq->pWhere NULL mean an error, and it is ok to set abort. > + pSubq->pWhere = > + sql_and_expr_new(pParse->db, > + pSubq->pWhere, pNew); > + if (pSubq->pWhere == NULL) > + pParse->is_aborted = true; > + } > pSubq = pSubq->pPrior; > } > } > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index d3ffa9d9b..13d0b16f8 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3223,13 +3223,73 @@ void sqlClearTempRegCache(Parse *); > +/** > + * This routine is the core allocator for Expr nodes. > + * Construct a new expression node and return a pointer to it. > + * Memory for this node and for the token argument is a single > + * allocation obtained from sqlDbMalloc(). The calling function > + * is responsible for making sure the node eventually gets freed. > + * > + * If dequote is true, then the token (if it exists) is dequoted. > + * If dequote is false, no dequoting is performed. The dequote > + * parameter is ignored if token is NULL or if the token does > + * not appear to be quoted. If the quotes were of the form "..." > + * (double-quotes) then the EP_DblQuoted flag is set on the > + * expression node. > + * > + * Special case: If op==TK_INTEGER and token points to a string > + * that can be translated into a 32-bit integer, then the token is > + * not stored in u.zToken. Instead, the integer values is written > + * into u.iValue and the EP_IntValue flag is set. No extra storage > + * is allocated to hold the integer text and the dequote flag is > + * ignored. > + * > + * @param db The database connection. > + * @param op Expression opcode (TK_*). > + * @param token Source token. Might be NULL. > + * @param dequote True to dequote string. > + * @retval Not NULL New expression object on success. > + * @retval NULL Otherwise. The diag message is set. > + */ > +struct Expr * > +sql_expr_new(struct sql *db, int op, const Token *token, bool dequote); 13. After you has changed the last argument type to boolean, the function call looks too long. I've split sql_expr_new into two functions: sql_expr_new and sql_expr_new_dequoted. > + > +/** > + * Allocate a new expression node from a zero-terminated token > + * that has already been dequoted. > + * > + * @param db The database connection. > + * @param op Expression opcode. > + * @param name The object name string. > + * @retval Not NULL Expression pointer on success. > + * @retval NULL Otherwise. The diag message is set. > + */ > +struct Expr * > +sql_op_expr_new(struct sql *db, int op, const char *name); 14. That name looks strange. You want to emphasize that this function takes an 'op'. But sql_expr_new() takes 'op' as well. And what is more - uses it in the exactly same way. I've renamed it to sql_expr_new_named(). Also, I've added an alias for NULL names: sql_expr_new_anon(). > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index be30a40c0..036540d61 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1372,7 +1372,13 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > continue; > testcase(pWC->a[iTerm].wtFlags & TERM_ORINFO); > pExpr = sqlExprDup(db, pExpr, 0); > - pAndExpr = sqlExprAnd(db, pAndExpr, pExpr); > + if (pAndExpr != NULL || pExpr != NULL) { 15. If pExpr here is NULL, then it is already an error, because 2 lines above it was not NULL - it was used to check ExprHasProperty. It means, that sql_and_expr_new() NULL result always is an error and it is ok to abort. > + pAndExpr = > + sql_and_expr_new(db, pAndExpr, > + pExpr); > + if (pAndExpr == NULL) > + pParse->is_aborted = true; > + } > } > if (pAndExpr) { > pAndExpr = ========================================================================== commit fc09568fad97d9bc548adaa9e54c8fcd7a88a980 Author: Vladislav Shpilevoy Date: Tue Mar 26 11:38:07 2019 +0300 Review fixes diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 10861dd15..2e4558408 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -616,7 +616,7 @@ sqlAddPrimaryKey(Parse * pParse, /* Parsing context */ struct ExprList *list; struct Token token; sqlTokenInit(&token, space->def->fields[iCol].name); - struct Expr *expr = sql_expr_new(db, TK_ID, &token, false); + struct Expr *expr = sql_expr_new(db, TK_ID, &token); if (expr == NULL) { pParse->is_aborted = true; goto primary_key_exit; @@ -1358,12 +1358,13 @@ sql_id_eq_str_expr(struct Parse *parse, const char *col_name, const char *col_value) { struct sql *db = parse->db; - struct Expr *col_name_expr = sql_op_expr_new(db, TK_ID, col_name); + struct Expr *col_name_expr = sql_expr_new_named(db, TK_ID, col_name); if (col_name_expr == NULL) { parse->is_aborted = true; return NULL; } - struct Expr *col_value_expr = sql_op_expr_new(db, TK_STRING, col_value); + struct Expr *col_value_expr = + sql_expr_new_named(db, TK_STRING, col_value); if (col_value_expr == NULL) { sql_expr_delete(db, col_name_expr, false); parse->is_aborted = true; @@ -1385,23 +1386,17 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name, return; } src_list->a[0].zName = sqlDbStrDup(db, stat_table_name); - struct Expr *where = NULL; + struct Expr *expr, *where = NULL; if (idx_name != NULL) { - struct Expr *expr = sql_id_eq_str_expr(parse, "idx", idx_name); - if (expr != NULL && (expr != NULL || where != NULL)) { - where = sql_and_expr_new(db, expr, where); - if (where == NULL) - parse->is_aborted = true; - } + expr = sql_id_eq_str_expr(parse, "idx", idx_name); + where = sql_and_expr_new(db, expr, where); } if (table_name != NULL) { - struct Expr *expr = sql_id_eq_str_expr(parse, "tbl", table_name); - if (expr != NULL && (expr != NULL || where != NULL)) { - where = sql_and_expr_new(db, expr, where); - if (where == NULL) - parse->is_aborted = true; - } + expr = sql_id_eq_str_expr(parse, "tbl", table_name); + where = sql_and_expr_new(db, expr, where); } + if (where == NULL) + parse->is_aborted = true; /** * On memory allocation error sql_table delete_from * releases memory for its own. @@ -2256,12 +2251,12 @@ sql_create_index(struct Parse *parse, struct Token *token, struct Token prev_col; uint32_t last_field = def->field_count - 1; sqlTokenInit(&prev_col, def->fields[last_field].name); - struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col, false); + struct Expr *expr = sql_expr_new(db, TK_ID, &prev_col); if (expr == NULL) { parse->is_aborted = true; goto exit_create_index; } - col_list = sql_expr_list_append(parse->db, NULL, expr); + col_list = sql_expr_list_append(db, NULL, expr); if (col_list == NULL) goto exit_create_index; assert(col_list->nExpr == 1); diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index dad4ce3a6..9c8d9fdf4 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -151,8 +151,12 @@ sqlExprAddCollateToken(Parse * pParse, /* Parsing context */ { if (pCollName->n == 0) return pExpr; - struct Expr *new_expr = - sql_expr_new(pParse->db, TK_COLLATE, pCollName, dequote); + struct Expr *new_expr; + struct sql *db = pParse->db; + if (dequote) + new_expr = sql_expr_new_dequoted(db, TK_COLLATE, pCollName); + else + new_expr = sql_expr_new(db, TK_COLLATE, pCollName); if (new_expr == NULL) { pParse->is_aborted = true; return pExpr; @@ -876,7 +880,7 @@ sqlExprSetHeightAndFlags(Parse * pParse, Expr * p) #endif /* SQL_MAX_EXPR_DEPTH>0 */ struct Expr * -sql_expr_new(struct sql *db, int op, const Token *token, bool dequote) +sql_expr_new(struct sql *db, int op, const struct Token *token) { int extra_sz = 0; int val = 0; @@ -911,25 +915,23 @@ sql_expr_new(struct sql *db, int op, const Token *token, bool dequote) assert(token->z != NULL || token->n == 0); memcpy(expr->u.zToken, token->z, token->n); expr->u.zToken[token->n] = '\0'; - if (dequote) { - if (expr->u.zToken[0] == '"') - expr->flags |= EP_DblQuoted; - if (expr->op == TK_ID || expr->op == TK_COLLATE || - expr->op == TK_FUNCTION) - sqlNormalizeName(expr->u.zToken); - else - sqlDequote(expr->u.zToken); - } } return expr; } struct Expr * -sql_op_expr_new(struct sql *db, int op, const char *name) +sql_expr_new_dequoted(struct sql *db, int op, const struct Token *token) { - struct Token name_token; - sqlTokenInit(&name_token, (char *)name); - return sql_expr_new(db, op, &name_token, false); + struct Expr *e = sql_expr_new(db, op, token); + if (e == NULL || (e->flags & EP_IntValue) != 0 || e->u.zToken == NULL) + return e; + if (e->u.zToken[0] == '"') + e->flags |= EP_DblQuoted; + if (e->op == TK_ID || e->op == TK_COLLATE || e->op == TK_FUNCTION) + sqlNormalizeName(e->u.zToken); + else + sqlDequote(e->u.zToken); + return e; } /* @@ -979,8 +981,6 @@ sqlPExpr(Parse * pParse, /* Parsing context */ * Take advantage of short-circuit false * optimization for AND. */ - if (pLeft == NULL && pRight == NULL) - return NULL; p = sql_and_expr_new(pParse->db, pLeft, pRight); if (p == NULL) pParse->is_aborted = true; @@ -1057,17 +1057,15 @@ sql_and_expr_new(struct sql *db, struct Expr *left_expr, struct Expr *right_expr) { if (left_expr == NULL) { - assert(right_expr != NULL); return right_expr; } else if (right_expr == NULL) { - assert(left_expr != NULL); return left_expr; } else if (exprAlwaysFalse(left_expr) || exprAlwaysFalse(right_expr)) { sql_expr_delete(db, left_expr, false); sql_expr_delete(db, right_expr, false); - return sql_expr_new(db, TK_INTEGER, &sqlIntTokens[0], false); + return sql_expr_new(db, TK_INTEGER, &sqlIntTokens[0]); } else { - struct Expr *new_expr = sql_expr_new(db, TK_AND, NULL, false); + struct Expr *new_expr = sql_expr_new_anon(db, TK_AND); sqlExprAttachSubtrees(db, new_expr, left_expr, right_expr); return new_expr; } @@ -1082,7 +1080,7 @@ sqlExprFunction(Parse * pParse, ExprList * pList, Token * pToken) { struct sql *db = pParse->db; assert(pToken != NULL); - struct Expr *new_expr = sql_expr_new(db, TK_FUNCTION, pToken, true); + struct Expr *new_expr = sql_expr_new_dequoted(db, TK_FUNCTION, pToken); if (new_expr == NULL) { sql_expr_list_delete(db, pList); pParse->is_aborted = true; @@ -2903,7 +2901,7 @@ sqlCodeSubselect(Parse * pParse, /* Parsing context */ if (pSel->pLimit == NULL) { pSel->pLimit = sql_expr_new(pParse->db, TK_INTEGER, - &sqlIntTokens[1], false); + &sqlIntTokens[1]); if (pSel->pLimit == NULL) { pParse->is_aborted = true; } else { diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c index 3b27b2d4f..8151c66e5 100644 --- a/src/box/sql/fk_constraint.c +++ b/src/box/sql/fk_constraint.c @@ -302,36 +302,28 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent, } /** - * Return an Expr object that refers to a memory register - * corresponding to column of given space. - * - * reg_base is the first of an array of register that contains - * the data for given space. reg_base+1 holds the first column. - * reg_base+2 holds the second column, and so forth. - * - * @param parser The parsing context. - * @param def Definition of space whose content is at r[regBase]... - * @param reg_base Contents of table table. - * @param column Index of table column is desired. - * @retval Not NULL expression pointer on success. - * @retval NULL Otherwise. + * Build an expression that refers to a memory register + * corresponding to @a column of given space. + * + * @param db SQL context. + * @param def Definition of space whose content starts from + * @a reg_base register. + * @param reg_base Index of a first element in an array of + * registers, containing data of a space. Register + * reg_base + i holds an i-th column, i >= 1. + * @param column Index of a first table column to point at. + * @retval Not NULL Success. An expression representing register. + * @retval NULL Error. A diag message is set. */ static struct Expr * -sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base, - int column) +sql_expr_new_register(struct sql *db, struct space_def *def, int reg_base, + uint32_t column) { - struct Expr *expr = sql_op_expr_new(parser->db, TK_REGISTER, NULL); - if (expr == NULL) { - parser->is_aborted = true; + struct Expr *expr = sql_expr_new_anon(db, TK_REGISTER); + if (expr == NULL) return NULL; - } - if (column >= 0) { - expr->iTable = reg_base + column + 1; - expr->type = def->fields[column].type; - } else { - expr->iTable = reg_base; - expr->type = FIELD_TYPE_INTEGER; - } + expr->iTable = reg_base + column + 1; + expr->type = def->fields[column].type; return expr; } @@ -346,10 +338,10 @@ sql_register_expr_new(struct Parse *parser, struct space_def *def, int reg_base, * @retval NULL on error. */ static struct Expr * -sql_column_cursor_expr_create(struct sql *db, struct space_def *def, +sql_expr_new_column_by_cursor(struct sql *db, struct space_def *def, int cursor, int column) { - struct Expr *expr = sql_op_expr_new(db, TK_COLUMN, NULL); + struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN); if (expr == NULL) return NULL; expr->space_def = def; @@ -435,18 +427,14 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src, for (uint32_t i = 0; i < fk_def->field_count; i++) { uint32_t fieldno = fk_def->links[i].parent_field; struct Expr *pexpr = - sql_register_expr_new(parser, def, reg_data, fieldno); + sql_expr_new_register(db, def, reg_data, fieldno); fieldno = fk_def->links[i].child_field; const char *field_name = child_space->def->fields[fieldno].name; - struct Expr *chexpr = sql_op_expr_new(db, TK_ID, field_name); - if (chexpr == NULL) - parser->is_aborted = true; + struct Expr *chexpr = sql_expr_new_named(db, TK_ID, field_name); struct Expr *eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr); - if (where != NULL || eq != NULL) { - where = sql_and_expr_new(db, where, eq); - if (where == NULL) - parser->is_aborted = true; - } + where = sql_and_expr_new(db, where, eq); + if (where == NULL || chexpr == NULL || pexpr == NULL) + parser->is_aborted = true; } /* @@ -462,26 +450,20 @@ fk_constraint_scan_children(struct Parse *parser, struct SrcList *src, struct Expr *expr = NULL, *pexpr, *chexpr, *eq; for (uint32_t i = 0; i < fk_def->field_count; i++) { uint32_t fieldno = fk_def->links[i].parent_field; - pexpr = sql_register_expr_new(parser, def, reg_data, + pexpr = sql_expr_new_register(db, def, reg_data, fieldno); int cursor = src->a[0].iCursor; - chexpr = sql_column_cursor_expr_create(db, def, cursor, + chexpr = sql_expr_new_column_by_cursor(db, def, cursor, fieldno); - if (chexpr == NULL) - parser->is_aborted = true; eq = sqlPExpr(parser, TK_EQ, pexpr, chexpr); - if (expr != NULL || eq != NULL) { - expr = sql_and_expr_new(db, expr, eq); - if (expr == NULL) - parser->is_aborted = true; - } - } - struct Expr *pNe = sqlPExpr(parser, TK_NOT, expr, 0); - if (where != NULL || pNe != NULL) { - where = sql_and_expr_new(db, where, pNe); - if (where == NULL) + expr = sql_and_expr_new(db, expr, eq); + if (expr == NULL || chexpr == NULL || pexpr == NULL) parser->is_aborted = true; } + struct Expr *pNe = sqlPExpr(parser, TK_NOT, expr, 0); + where = sql_and_expr_new(db, where, pNe); + if (where == NULL) + parser->is_aborted = true; } /* Resolve the references in the WHERE clause. */ @@ -720,6 +702,28 @@ fk_constraint_is_required(struct space *space, const int *changes) return false; } +/** + * Create a new expression representing two-part path + * '
.'. + * @param parser SQL Parser. + * @param main First and main part of a result path. For example, + * a table name. + * @param sub Second and last part of a result path. For example, + * a column name. + * @retval Not NULL Success. An expression with two-part path. + * @retval NULL Error. A diag message is set. + */ +static inline struct Expr * +sql_expr_new_2part_id(struct Parse *parser, const struct Token *main, + const struct Token *sub) +{ + struct Expr *emain = sql_expr_new(parser->db, TK_ID, main); + struct Expr *esub = sql_expr_new(parser->db, TK_ID, sub); + if (emain == NULL || esub == NULL) + parser->is_aborted = true; + return sqlPExpr(parser, TK_DOT, emain, esub); +} + /** * This function is called when an UPDATE or DELETE operation is * being compiled on table pTab, which is the parent table of @@ -802,27 +806,13 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, * type and collation sequence associated with * the parent table are used for the comparison. */ - struct Expr *old_expr = NULL, *new_expr = NULL, *expr = NULL; - old_expr = sql_expr_new(db, TK_ID, &t_old, false); - if (old_expr == NULL) - pParse->is_aborted = true; - expr = sql_expr_new(db, TK_ID, &t_to_col, false); - if (expr == NULL) - pParse->is_aborted = true; - struct Expr *from_col_expr = - sql_expr_new(db, TK_ID, &t_from_col, false); - if (from_col_expr == NULL) + struct Expr *new, *old = + sql_expr_new_2part_id(pParse, &t_old, &t_to_col); + struct Expr *from = sql_expr_new(db, TK_ID, &t_from_col); + struct Expr *eq = sqlPExpr(pParse, TK_EQ, old, from); + where = sql_and_expr_new(db, where, eq); + if (where == NULL || from == NULL) pParse->is_aborted = true; - struct Expr *to_col_expr = - sqlPExpr(pParse, TK_DOT, old_expr, expr); - struct Expr *eq = - sqlPExpr(pParse, TK_EQ, to_col_expr, from_col_expr); - if (where != NULL || eq != NULL) { - where = sql_and_expr_new(db, where, eq); - if (where == NULL) - pParse->is_aborted = true; - } - /* * For ON UPDATE, construct the next term of the * WHEN clause, which should return false in case @@ -839,67 +829,36 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, * no_action_needed(colN)) */ if (is_update) { - old_expr = sql_expr_new(db, TK_ID, &t_old, false); - if (old_expr == NULL) - pParse->is_aborted = true; - expr = sql_expr_new(db, TK_ID, &t_to_col, false); - if (expr == NULL) - pParse->is_aborted = true; - struct Expr *old_val = sqlPExpr(pParse, TK_DOT, - old_expr, expr); - new_expr = sql_expr_new(db, TK_ID, &t_new, false); - if (new_expr == NULL) - pParse->is_aborted = true; - expr = sql_expr_new(db, TK_ID, &t_to_col, false); - if (expr == NULL) - pParse->is_aborted = true; - struct Expr *new_val = sqlPExpr(pParse, TK_DOT, - new_expr, expr); - struct Expr *old_is_null = sqlPExpr( - pParse, TK_ISNULL, - sqlExprDup(db, old_val, 0), NULL); - eq = sqlPExpr(pParse, TK_EQ, old_val, - sqlExprDup(db, new_val, 0)); + old = sql_expr_new_2part_id(pParse, &t_old, &t_to_col); + new = sql_expr_new_2part_id(pParse, &t_new, &t_to_col); + struct Expr *old_is_null = + sqlPExpr(pParse, TK_ISNULL, + sqlExprDup(db, old, 0), NULL); + eq = sqlPExpr(pParse, TK_EQ, old, + sqlExprDup(db, new, 0)); struct Expr *new_non_null = - sqlPExpr(pParse, TK_NOTNULL, new_val, NULL); + sqlPExpr(pParse, TK_NOTNULL, new, NULL); struct Expr *non_null_eq = sqlPExpr(pParse, TK_AND, new_non_null, eq); struct Expr *no_action_needed = sqlPExpr(pParse, TK_OR, old_is_null, non_null_eq); - if (when != NULL || no_action_needed != NULL) { - when = sql_and_expr_new(db, when, - no_action_needed); - if (when == NULL) - pParse->is_aborted = true; - } + when = sql_and_expr_new(db, when, no_action_needed); + if (when == NULL) + pParse->is_aborted = true; } if (action != FKEY_ACTION_RESTRICT && (action != FKEY_ACTION_CASCADE || is_update)) { - struct Expr *new, *d; + struct Expr *d = child_fields[chcol].default_value_expr; if (action == FKEY_ACTION_CASCADE) { - new_expr = sql_expr_new(db, TK_ID, &t_new, - false); - if (new_expr == NULL) - pParse->is_aborted = true; - expr = sql_expr_new(db, TK_ID, &t_to_col, - false); - if (expr == NULL) - pParse->is_aborted = true; - new = sqlPExpr(pParse, TK_DOT, new_expr, expr); - } else if (action == FKEY_ACTION_SET_DEFAULT) { - d = child_fields[chcol].default_value_expr; - if (d != NULL) { - new = sqlExprDup(db, d, 0); - } else { - new = sql_expr_new(db, TK_NULL, NULL, - false); - if (new == NULL) - pParse->is_aborted = true; - } + new = sql_expr_new_2part_id(pParse, &t_new, + &t_to_col); + } else if (action == FKEY_ACTION_SET_DEFAULT && + d != NULL) { + new = sqlExprDup(db, d, 0); } else { - new = sql_expr_new(db, TK_NULL, NULL, false); + new = sql_expr_new_anon(db, TK_NULL); if (new == NULL) pParse->is_aborted = true; } @@ -915,9 +874,8 @@ fk_constraint_action_trigger(struct Parse *pParse, struct space_def *def, struct Token err; err.z = space_name; err.n = name_len; - struct Expr *r = - sql_op_expr_new(db, TK_RAISE, - "FOREIGN KEY constraint failed"); + struct Expr *r = sql_expr_new_named(db, TK_RAISE, "FOREIGN "\ + "KEY constraint failed"); if (r == NULL) pParse->is_aborted = true; else diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 4243e799c..c749cf1bd 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -536,7 +536,7 @@ selcollist(A) ::= sclp(A) expr(X) as(Y). { sqlExprListSetSpan(pParse,A,&X); } selcollist(A) ::= sclp(A) STAR. { - struct Expr *p = sql_op_expr_new(pParse->db, TK_ASTERISK, NULL); + struct Expr *p = sql_expr_new_anon(pParse->db, TK_ASTERISK); if (p == NULL) { pParse->is_aborted = true; return; @@ -544,7 +544,7 @@ selcollist(A) ::= sclp(A) STAR. { A = sql_expr_list_append(pParse->db, A, p); } selcollist(A) ::= sclp(A) nm(X) DOT STAR. { - struct Expr *pLeft = sql_expr_new(pParse->db, TK_ID, &X, true); + struct Expr *pLeft = sql_expr_new_dequoted(pParse->db, TK_ID, &X); if (pLeft == NULL) { pParse->is_aborted = true; return; @@ -906,14 +906,14 @@ term(A) ::= NULL(X). {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/} expr(A) ::= id(X). {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/} expr(A) ::= JOIN_KW(X). {spanExpr(&A,pParse,TK_ID,X); /*A-overwrites-X*/} expr(A) ::= nm(X) DOT nm(Y). { - struct Expr *temp1 = sql_expr_new(pParse->db, TK_ID, &X, true); + struct Expr *temp1 = sql_expr_new_dequoted(pParse->db, TK_ID, &X); if (temp1 == NULL) { pParse->is_aborted = true; return; } - struct Expr *temp2 = sql_expr_new(pParse->db, TK_ID, &Y, true); + struct Expr *temp2 = sql_expr_new_dequoted(pParse->db, TK_ID, &Y); if (temp2 == NULL) { - sql_expr_delete(pParse->db, temp2, false); + sql_expr_delete(pParse->db, temp1, false); pParse->is_aborted = true; return; } @@ -923,7 +923,7 @@ expr(A) ::= nm(X) DOT nm(Y). { term(A) ::= FLOAT|BLOB(X). {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/} term(A) ::= STRING(X). {spanExpr(&A,pParse,@X,X);/*A-overwrites-X*/} term(A) ::= INTEGER(X). { - A.pExpr = sql_expr_new(pParse->db, TK_INTEGER, &X, true); + A.pExpr = sql_expr_new_dequoted(pParse->db, TK_INTEGER, &X); if (A.pExpr == NULL) { pParse->is_aborted = true; return; @@ -963,7 +963,7 @@ expr(A) ::= expr(A) COLLATE id(C). { %ifndef SQL_OMIT_CAST expr(A) ::= CAST(X) LP expr(E) AS typedef(T) RP(Y). { spanSet(&A,&X,&Y); /*A-overwrites-X*/ - A.pExpr = sql_expr_new(pParse->db, TK_CAST, NULL, true); + A.pExpr = sql_expr_new_dequoted(pParse->db, TK_CAST, NULL); if (A.pExpr == NULL) { pParse->is_aborted = true; return; @@ -1161,7 +1161,7 @@ expr(A) ::= expr(A) in_op(N) LP exprlist(Y) RP(E). [IN] { ** regardless of the value of expr1. */ sql_expr_delete(pParse->db, A.pExpr, false); - A.pExpr = sql_expr_new(pParse->db, TK_INTEGER, &sqlIntTokens[N], true); + A.pExpr = sql_expr_new_dequoted(pParse->db, TK_INTEGER, &sqlIntTokens[N]); if (A.pExpr == NULL) { pParse->is_aborted = true; return; @@ -1506,7 +1506,7 @@ expr(A) ::= RAISE(X) LP IGNORE RP(Y). { } expr(A) ::= RAISE(X) LP raisetype(T) COMMA STRING(Z) RP(Y). { spanSet(&A,&X,&Y); /*A-overwrites-X*/ - A.pExpr = sql_expr_new(pParse->db, TK_RAISE, &Z, true); + A.pExpr = sql_expr_new_dequoted(pParse->db, TK_RAISE, &Z); if(A.pExpr == NULL) { pParse->is_aborted = true; return; diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index eb74d024c..075e1fab4 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -488,10 +488,10 @@ lookupName(Parse * pParse, /* The parsing context */ } struct Expr * -sql_column_expr_new(struct sql *db, struct SrcList *src_list, int src_idx, +sql_expr_new_column(struct sql *db, struct SrcList *src_list, int src_idx, int column) { - struct Expr *expr = sql_expr_new(db, TK_COLUMN, NULL, false); + struct Expr *expr = sql_expr_new_anon(db, TK_COLUMN); if (expr == NULL) return NULL; struct SrcList_item *item = &src_list->a[src_idx]; @@ -995,7 +995,7 @@ resolveCompoundOrderBy(Parse * pParse, /* Parsing context. Leave error messages * taking care to preserve the COLLATE clause if it exists */ struct Expr *pNew = - sql_op_expr_new(db, TK_INTEGER, NULL); + sql_expr_new_anon(db, TK_INTEGER); if (pNew == NULL) { pParse->is_aborted = true; return 1; @@ -1349,7 +1349,7 @@ resolveSelectStep(Walker * pWalker, Select * p) */ sql_expr_delete(db, p->pLimit, false); p->pLimit = sql_expr_new(db, TK_INTEGER, - &sqlIntTokens[1], false); + &sqlIntTokens[1]); if (p->pLimit == NULL) pParse->is_aborted = true; } else { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 5bba0b5d0..6d115182d 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -164,10 +164,10 @@ sqlSelectNew(Parse * pParse, /* Parsing context */ pNew = &standin; } if (pEList == 0) { - struct Expr *expr = sql_op_expr_new(db, TK_ASTERISK, NULL); + struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK); if (expr == NULL) pParse->is_aborted = true; - pEList = sql_expr_list_append(pParse->db, NULL, expr); + pEList = sql_expr_list_append(db, NULL, expr); } struct session MAYBE_UNUSED *user_session; user_session = current_session(); @@ -489,8 +489,7 @@ addWhereTerm(Parse * pParse, /* Parsing context */ int isOuterJoin, /* True if this is an OUTER join */ Expr ** ppWhere) /* IN/OUT: The WHERE clause to add to */ { - Expr *pE1; - Expr *pE2; + struct sql *db = pParse->db; Expr *pEq; assert(iLeft < iRight); @@ -498,13 +497,10 @@ addWhereTerm(Parse * pParse, /* Parsing context */ assert(pSrc->a[iLeft].space != NULL); assert(pSrc->a[iRight].space != NULL); - pE1 = sql_column_expr_new(pParse->db, pSrc, iLeft, iColLeft); - if (pE1 == NULL) + struct Expr *pE1 = sql_expr_new_column(db, pSrc, iLeft, iColLeft); + struct Expr *pE2 = sql_expr_new_column(db, pSrc, iRight, iColRight); + if (pE1 == NULL || pE2 == NULL) pParse->is_aborted = true; - pE2 = sql_column_expr_new(pParse->db, pSrc, iRight, iColRight); - if (pE2 == NULL) - pParse->is_aborted = true; - pEq = sqlPExpr(pParse, TK_EQ, pE1, pE2); if (pEq && isOuterJoin) { ExprSetProperty(pEq, EP_FromJoin); @@ -512,11 +508,9 @@ addWhereTerm(Parse * pParse, /* Parsing context */ ExprSetVVAProperty(pEq, EP_NoReduce); pEq->iRightJoinTable = (i16) pE2->iTable; } - if (*ppWhere != NULL || pEq != NULL) { - *ppWhere = sql_and_expr_new(pParse->db, *ppWhere, pEq); - if (*ppWhere == NULL) - pParse->is_aborted = true; - } + *ppWhere = sql_and_expr_new(db, *ppWhere, pEq); + if (*ppWhere == NULL) + pParse->is_aborted = true; } /* @@ -637,13 +631,10 @@ sqlProcessJoin(Parse * pParse, Select * p) if (pRight->pOn) { if (isOuter) setJoinExpr(pRight->pOn, pRight->iCursor); - if (p->pWhere != NULL || pRight->pOn != NULL) { - p->pWhere = - sql_and_expr_new(pParse->db, p->pWhere, - pRight->pOn); - if (p->pWhere == NULL) - pParse->is_aborted = true; - } + p->pWhere = sql_and_expr_new(pParse->db, p->pWhere, + pRight->pOn); + if (p->pWhere == NULL) + pParse->is_aborted = true; pRight->pOn = 0; } @@ -3349,10 +3340,10 @@ multiSelectOrderBy(Parse * pParse, /* Parsing context */ } if (j == nOrderBy) { struct Expr *pNew = - sql_op_expr_new(db, TK_INTEGER, NULL); + sql_expr_new_anon(db, TK_INTEGER); if (pNew == NULL) { pParse->is_aborted = true; - return SQL_NOMEM; + return 1; } pNew->flags |= EP_IntValue; pNew->u.iValue = i; @@ -4226,7 +4217,8 @@ flattenSubquery(Parse * pParse, /* Parsing context */ assert(pParent->pHaving == 0); pParent->pHaving = pParent->pWhere; pParent->pWhere = pWhere; - struct Expr *sub_having = sqlExprDup(db, pSub->pHaving, 0); + struct Expr *sub_having = + sqlExprDup(db, pSub->pHaving, 0); if (sub_having != NULL || pParent->pHaving != NULL) { pParent->pHaving = sql_and_expr_new(db, sub_having, @@ -4346,13 +4338,10 @@ pushDownWhereTerms(Parse * pParse, /* Parse context (for malloc() and error repo while (pSubq) { pNew = sqlExprDup(pParse->db, pWhere, 0); pNew = substExpr(pParse, pNew, iCursor, pSubq->pEList); - if (pSubq->pWhere != NULL || pNew != NULL) { - pSubq->pWhere = - sql_and_expr_new(pParse->db, + pSubq->pWhere = sql_and_expr_new(pParse->db, pSubq->pWhere, pNew); - if (pSubq->pWhere == NULL) - pParse->is_aborted = true; - } + if (pSubq->pWhere == NULL) + pParse->is_aborted = true; pSubq = pSubq->pPrior; } } @@ -4533,7 +4522,7 @@ convertCompoundSelectToSubquery(Walker * pWalker, Select * p) return WRC_Abort; *pNew = *p; p->pSrc = pNewSrc; - struct Expr *expr = sql_op_expr_new(db, TK_ASTERISK, NULL); + struct Expr *expr = sql_expr_new_anon(db, TK_ASTERISK); if (expr == NULL) pParse->is_aborted = true; p->pEList = sql_expr_list_append(pParse->db, NULL, expr); @@ -5020,7 +5009,7 @@ selectExpander(Walker * pWalker, Select * p) continue; } } - pRight = sql_op_expr_new(db, + pRight = sql_expr_new_named(db, TK_ID, zName); if (pRight == NULL) pParse->is_aborted = true; @@ -5029,7 +5018,7 @@ selectExpander(Walker * pWalker, Select * p) if (longNames || pTabList->nSrc > 1) { Expr *pLeft; - pLeft = sql_op_expr_new( + pLeft = sql_expr_new_named( db, TK_ID, zTabName); diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 5a7a67f2d..7182f58fd 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -3225,48 +3225,54 @@ int sqlNoTempsInRange(Parse *, int, int); #endif /** - * This routine is the core allocator for Expr nodes. - * Construct a new expression node and return a pointer to it. - * Memory for this node and for the token argument is a single - * allocation obtained from sqlDbMalloc(). The calling function - * is responsible for making sure the node eventually gets freed. - * - * If dequote is true, then the token (if it exists) is dequoted. - * If dequote is false, no dequoting is performed. The dequote - * parameter is ignored if token is NULL or if the token does - * not appear to be quoted. If the quotes were of the form "..." - * (double-quotes) then the EP_DblQuoted flag is set on the - * expression node. + * Construct a new expression. Memory for this node and for the + * token argument is a single allocation obtained from + * sqlDbMalloc(). The calling function is responsible for making + * sure the node eventually gets freed. * * Special case: If op==TK_INTEGER and token points to a string * that can be translated into a 32-bit integer, then the token is * not stored in u.zToken. Instead, the integer values is written * into u.iValue and the EP_IntValue flag is set. No extra storage - * is allocated to hold the integer text and the dequote flag is - * ignored. + * is allocated to hold the integer text. * * @param db The database connection. * @param op Expression opcode (TK_*). * @param token Source token. Might be NULL. - * @param dequote True to dequote string. * @retval Not NULL New expression object on success. * @retval NULL Otherwise. The diag message is set. */ struct Expr * -sql_expr_new(struct sql *db, int op, const Token *token, bool dequote); +sql_expr_new(struct sql *db, int op, const struct Token *token); /** - * Allocate a new expression node from a zero-terminated token - * that has already been dequoted. - * - * @param db The database connection. - * @param op Expression opcode. - * @param name The object name string. - * @retval Not NULL Expression pointer on success. - * @retval NULL Otherwise. The diag message is set. + * The same as @sa sql_expr_new, but normalizes name, stored in + * @a token. Quotes are removed if they are presented. */ struct Expr * -sql_op_expr_new(struct sql *db, int op, const char *name); +sql_expr_new_dequoted(struct sql *db, int op, const struct Token *token); + +/** + * The same as @a sql_expr_new, but takes const char instead of + * Token. Just sugar to do not touch tokens in many places. + */ +static inline struct Expr * +sql_expr_new_named(struct sql *db, int op, const char *name) +{ + struct Token name_token; + sqlTokenInit(&name_token, (char *)name); + return sql_expr_new(db, op, &name_token); +} + +/** + * The same as @a sql_expr_new, but a result expression has no + * name. + */ +static inline struct Expr * +sql_expr_new_anon(struct sql *db, int op) +{ + return sql_expr_new_named(db, op, NULL); +} void sqlExprAttachSubtrees(sql *, Expr *, Expr *, Expr *); Expr *sqlPExpr(Parse *, int, Expr *, Expr *); @@ -3284,7 +3290,8 @@ void sqlPExprAddSelect(Parse *, Expr *, Select *); * @param left_expr The left-branch expresion to join. * @param right_expr The right-branch expression to join. * @retval Not NULL New expression root node pointer on success. - * @retval NULL Otherwise. The diag message is set. + * @retval NULL Error. A diag message is set. + * @retval NULL Not an error. Both arguments were NULL. */ struct Expr * sql_and_expr_new(struct sql *db, struct Expr *left_expr, @@ -4810,18 +4817,18 @@ void sqlStrAccumReset(StrAccum *); void sqlSelectDestInit(SelectDest *, int, int, int); /* - * Allocate and return a pointer to an expression to load the - * column from datasource src_idx in SrcList src_list. + * Create an expression to load @a column from datasource + * @a src_idx in @a src_list. * * @param db The database connection. * @param src_list The source list described with FROM clause. * @param src_idx The resource index to use in src_list. * @param column The column index. - * @retval Not NULL Expression pointer on success. - * @retval NULL Otherwise. The diag message is set. + * @retval Not NULL Success. An expression to load @a column. + * @retval NULL Error. A diag message is set. */ struct Expr * -sql_column_expr_new(struct sql *db, struct SrcList *src_list, int src_idx, +sql_expr_new_column(struct sql *db, struct SrcList *src_list, int src_idx, int column); int sqlExprCheckIN(Parse *, Expr *); diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index 036540d61..a453fe979 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -1372,13 +1372,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W continue; testcase(pWC->a[iTerm].wtFlags & TERM_ORINFO); pExpr = sqlExprDup(db, pExpr, 0); - if (pAndExpr != NULL || pExpr != NULL) { - pAndExpr = - sql_and_expr_new(db, pAndExpr, - pExpr); - if (pAndExpr == NULL) - pParse->is_aborted = true; - } + pAndExpr = sql_and_expr_new(db, pAndExpr, + pExpr); + if (pAndExpr == NULL) + pParse->is_aborted = true; } if (pAndExpr) { pAndExpr = diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c index a9dec8e99..d22a4e0f4 100644 --- a/src/box/sql/whereexpr.c +++ b/src/box/sql/whereexpr.c @@ -307,7 +307,7 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix, Expr *pPrefix; *pisComplete = c == MATCH_ALL_WILDCARD && z[cnt + 1] == 0; - pPrefix = sql_op_expr_new(db, TK_STRING, z); + pPrefix = sql_expr_new_named(db, TK_STRING, z); if (pPrefix == NULL) pParse->is_aborted = true; else @@ -1308,7 +1308,7 @@ exprAnalyze(SrcList * pSrc, /* the FROM clause */ Expr *pLeft = pExpr->pLeft; int idxNew; WhereTerm *pNewTerm; - struct Expr *expr = sql_expr_new(db, TK_NULL, NULL, false); + struct Expr *expr = sql_expr_new_anon(db, TK_NULL); if (expr == NULL) pParse->is_aborted = true; pNewExpr = sqlPExpr(pParse, TK_GT, sqlExprDup(db, pLeft, 0), @@ -1505,7 +1505,7 @@ sqlWhereTabFuncArgs(Parse * pParse, /* Parsing context */ space_def->name, j); return; } - pColRef = sql_expr_new(pParse->db, TK_COLUMN, NULL, false); + pColRef = sql_expr_new_anon(pParse->db, TK_COLUMN); if (pColRef == NULL) { pParse->is_aborted = true; return;