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 5363B22F11 for ; Wed, 16 Jan 2019 08:13:27 -0500 (EST) 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 BvFRNATcqnDM for ; Wed, 16 Jan 2019 08:13:25 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 41FF6245EC for ; Wed, 16 Jan 2019 08:13:25 -0500 (EST) From: Nikita Pettik Subject: [tarantool-patches] [PATCH 1/2] sql: refactor sql_expr_coll and sql_binary_compare_coll_seq functions Date: Wed, 16 Jan 2019 16:13:19 +0300 Message-Id: <276917ac3b405134ae0f075127888912395d511e.1547644179.git.korablev@tarantool.org> In-Reply-To: References: In-Reply-To: References: 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 Cc: v.shpilevoy@tarantool.org, Nikita Pettik Lets make sql_expr_coll() return error code for two reasons. Firstly, we are going to use this function to detect operands of concatenation with incompatible collations. Secondly, pointer to struct coll in most cases is redundant since collation id (which in turn returned via output parameter) is enough to proceed the same operations. For the same reason lets make sql_binary_compare_coll_seq() return collation id instead of struct coll* and remove corresponding output parameter. Needed for #3947 --- src/box/sql/expr.c | 82 ++++++++++++++++++++++++------------------------- src/box/sql/select.c | 27 ++++++++++------ src/box/sql/sqliteInt.h | 14 ++++----- src/box/sql/where.c | 56 ++++++++++++++++----------------- src/box/sql/whereexpr.c | 23 ++++++++------ 5 files changed, 105 insertions(+), 97 deletions(-) diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index b67b22c23..f8819f779 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -189,10 +189,9 @@ sqlite3ExprSkipCollate(Expr * pExpr) return pExpr; } -struct coll * +int sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) { - struct coll *coll = NULL; *is_explicit_coll = false; *coll_id = COLL_NONE; while (p != NULL) { @@ -205,7 +204,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) } if (op == TK_COLLATE || (op == TK_REGISTER && p->op2 == TK_COLLATE)) { - coll = sql_get_coll_seq(parse, p->u.zToken, coll_id); + (void) sql_get_coll_seq(parse, p->u.zToken, coll_id); *is_explicit_coll = true; break; } @@ -217,7 +216,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) */ int j = p->iColumn; if (j >= 0) { - coll = sql_column_collation(p->space_def, j, + (void) sql_column_collation(p->space_def, j, coll_id); } break; @@ -253,7 +252,7 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id) break; } } - return coll; + return 0; } enum affinity_type @@ -377,27 +376,28 @@ illegal_collation_mix: return -1; } -struct coll * -sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, - uint32_t *coll_id) +uint32_t +sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right) { assert(left != NULL); bool is_lhs_forced; bool is_rhs_forced; uint32_t lhs_coll_id; uint32_t rhs_coll_id; - struct coll *lhs_coll = sql_expr_coll(parser, left, &is_lhs_forced, - &lhs_coll_id); - struct coll *rhs_coll = sql_expr_coll(parser, right, &is_rhs_forced, - &rhs_coll_id); + if (sql_expr_coll(parser, left, &is_lhs_forced, &lhs_coll_id) != 0) + goto err; + if (sql_expr_coll(parser, right, &is_rhs_forced, &rhs_coll_id) != 0) + goto err; + uint32_t coll_id; if (collations_check_compatibility(lhs_coll_id, is_lhs_forced, rhs_coll_id, is_rhs_forced, - coll_id) != 0) { - parser->rc = SQL_TARANTOOL_ERROR; - parser->nErr++; - return NULL; - } - return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;; + &coll_id) != 0) + goto err; + return coll_id; +err: + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; + return COLL_NONE; } /* @@ -413,12 +413,11 @@ codeCompare(Parse * pParse, /* The parsing (and code generating) context */ int jumpIfNull /* If true, jump if either operand is NULL */ ) { - uint32_t id; - struct coll *p4 = - sql_binary_compare_coll_seq(pParse, pLeft, pRight, &id); + uint32_t id = sql_binary_compare_coll_seq(pParse, pLeft, pRight); + struct coll *coll = coll_by_id(id)->coll; int p5 = binaryCompareP5(pLeft, pRight, jumpIfNull); int addr = sqlite3VdbeAddOp4(pParse->pVdbe, opcode, in2, dest, in1, - (void *)p4, P4_COLLSEQ); + (void *)coll, P4_COLLSEQ); sqlite3VdbeChangeP5(pParse->pVdbe, (u8) p5); return addr; } @@ -2489,20 +2488,15 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */ for (i = 0; i < nExpr; i++) { Expr *pLhs = sqlite3VectorFieldSubexpr(pX->pLeft, i); Expr *pRhs = pEList->a[i].pExpr; - uint32_t id; - struct coll *pReq = sql_binary_compare_coll_seq - (pParse, pLhs, pRhs, &id); + uint32_t id = + sql_binary_compare_coll_seq(pParse, pLhs, pRhs); int j; for (j = 0; j < nExpr; j++) { if ((int) parts[j].fieldno != pRhs->iColumn) continue; - - struct coll *idx_coll = - parts[j].coll; - if (pReq != NULL && - pReq != idx_coll) + if (id != parts[j].coll_id) continue; break; } @@ -2815,9 +2809,9 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ Expr *p = sqlite3VectorFieldSubexpr (pLeft, i); - sql_binary_compare_coll_seq(pParse, p, - pEList->a[i].pExpr, - &key_info->parts[i].coll_id); + key_info->parts[i].coll_id = + sql_binary_compare_coll_seq(pParse, p, + pEList->a[i].pExpr); } } } else if (ALWAYS(pExpr->x.pList != 0)) { @@ -2839,8 +2833,9 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ affinity = AFFINITY_BLOB; } bool unused; - sql_expr_coll(pParse, pExpr->pLeft, - &unused, &key_info->parts[0].coll_id); + (void) sql_expr_coll(pParse, pExpr->pLeft, + &unused, + &key_info->parts[0].coll_id); /* Loop through each expression in . */ r1 = sqlite3GetTempReg(pParse); @@ -3100,8 +3095,9 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ bool unused; uint32_t id; ExprList *pList = pExpr->x.pList; - struct coll *coll = sql_expr_coll(pParse, pExpr->pLeft, - &unused, &id); + if (sql_expr_coll(pParse, pExpr->pLeft, &unused, &id) != 0) + goto sqlite3ExprCodeIN_finished; + struct coll *coll = coll_by_id(id)->coll; int labelOk = sqlite3VdbeMakeLabel(v); int r2, regToFree; int regCkNull = 0; @@ -3226,7 +3222,9 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ uint32_t id; int r3 = sqlite3GetTempReg(pParse); Expr *p = sqlite3VectorFieldSubexpr(pLeft, i); - struct coll *pColl = sql_expr_coll(pParse, p, &unused, &id); + if (sql_expr_coll(pParse, p, &unused, &id) != 0) + goto sqlite3ExprCodeIN_finished; + struct coll *pColl = coll_by_id(id)->coll; /* Tarantool: Replace i -> aiMap [i], since original order of columns * is preserved. */ @@ -4052,9 +4050,11 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target) 0 && coll == NULL) { bool unused; uint32_t id; - coll = sql_expr_coll(pParse, - pFarg->a[i].pExpr, - &unused, &id); + if (sql_expr_coll(pParse, + pFarg->a[i].pExpr, + &unused, &id) != 0) + break; + coll = coll_by_id(id)->coll;; } } if (pFarg) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 02ee225f1..8ba9554f6 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1042,10 +1042,11 @@ selectInnerLoop(Parse * pParse, /* The parser context */ for (i = 0; i < nResultCol; i++) { bool is_found; uint32_t id; - struct coll *coll = - sql_expr_coll(pParse, + if (sql_expr_coll(pParse, pEList->a[i].pExpr, - &is_found, &id); + &is_found, &id) != 0) + break; + struct coll *coll = coll_by_id(id)->coll; if (i < nResultCol - 1) { sqlite3VdbeAddOp3(v, OP_Ne, regResult + i, @@ -1424,7 +1425,10 @@ sql_expr_list_to_key_info(struct Parse *parse, struct ExprList *list, int start) struct key_part_def *part = &key_info->parts[i - start]; bool unused; uint32_t id; - sql_expr_coll(parse, item->pExpr, &unused, &id); + if (sql_expr_coll(parse, item->pExpr, &unused, &id) != 0) { + sqlite3DbFree(parse->db, key_info); + return NULL; + } part->coll_id = id; part->sort_order = item->sort_order; } @@ -1974,7 +1978,7 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */ uint32_t coll_id; if (pTab->def->fields[i].coll_id == COLL_NONE && - sql_expr_coll(pParse, p, &is_found, &coll_id) && + sql_expr_coll(pParse, p, &is_found, &coll_id) == 0 && coll_id != COLL_NONE) pTab->def->fields[i].coll_id = coll_id; } @@ -2196,8 +2200,9 @@ multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n, * resolution and we would not have got this far. */ assert(n >= 0 && n < p->pEList->nExpr); - sql_expr_coll(parser, p->pEList->a[n].pExpr, &is_current_forced, - ¤t_coll_id); + if (sql_expr_coll(parser, p->pEList->a[n].pExpr, &is_current_forced, + ¤t_coll_id) != 0) + return COLL_NONE; uint32_t res_coll_id; if (collations_check_compatibility(prior_coll_id, is_prior_forced, current_coll_id, is_current_forced, @@ -2253,7 +2258,7 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s, uint32_t id; bool unused; if ((term->flags & EP_Collate) != 0) { - sql_expr_coll(parse, term, &unused, &id); + (void) sql_expr_coll(parse, term, &unused, &id); } else { id = multi_select_coll_seq(parse, s, item->u.x.iOrderByCol - 1); @@ -5328,8 +5333,10 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo) uint32_t id; for (j = 0, pItem = pList->a; coll == NULL && j < nArg; j++, pItem++) { - coll = sql_expr_coll(pParse, pItem->pExpr, - &unused, &id); + if (sql_expr_coll(pParse, pItem->pExpr, + &unused, &id) != 0) + return; + coll = coll_by_id(id)->coll; } if (regHit == 0 && pAggInfo->nAccumulator) regHit = ++pParse->nMem; diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 7501fadc8..50d297815 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4337,10 +4337,10 @@ const char *sqlite3ErrStr(int); * clause is used. * @param[out] coll_id Collation identifier. * - * @retval Pointer to collation. + * @retval Return code: < 0 in case of error, 0 on success. */ -struct coll * -sql_expr_coll(Parse * pParse, Expr * pExpr, bool *is_explicit_coll, +int +sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id); Expr *sqlite3ExprAddCollateToken(Parse * pParse, Expr *, const Token *, int); @@ -4684,13 +4684,11 @@ collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced, * @param parser Parser. * @param left Left expression. * @param right Right expression. Can be NULL. - * @param[out] coll_id Collation identifier. * - * @retval Collation object. + * @retval Id of collation object. */ -struct coll * -sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right, - uint32_t *coll_id); +uint32_t +sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right); int sqlite3TempInMemory(const sqlite3 *); #ifndef SQLITE_OMIT_CTE With *sqlite3WithAdd(Parse *, With *, Token *, ExprList *, Select *); diff --git a/src/box/sql/where.c b/src/box/sql/where.c index 571b5af78..d1edbdfa9 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -304,11 +304,14 @@ whereScanNext(WhereScan * pScan) Parse *pParse = pWC->pWInfo->pParse; assert(pX->pLeft); - uint32_t unused; - struct coll *coll = + uint32_t id = sql_binary_compare_coll_seq( pParse, pX->pLeft, - pX->pRight, &unused); + pX->pRight); + struct coll *coll = + id != COLL_NONE ? + coll_by_id(id)->coll : + NULL; if (coll != pScan->coll) continue; } @@ -556,10 +559,10 @@ findIndexCol(Parse * pParse, /* Parse context */ p->iColumn == (int) part_to_match->fieldno) { bool is_found; uint32_t id; - struct coll *coll = sql_expr_coll(pParse, - pList->a[i].pExpr, - &is_found, &id); - if (coll == part_to_match->coll) + if (sql_expr_coll(pParse, pList->a[i].pExpr, + &is_found, &id) != 0) + return -1; + if (id == part_to_match->coll_id) return i; } } @@ -2263,7 +2266,6 @@ whereRangeVectorLen(Parse * pParse, /* Parsing context */ */ char aff; /* Comparison affinity */ char idxaff = 0; /* Indexed columns affinity */ - struct coll *pColl; /* Comparison collation sequence */ Expr *pLhs = pTerm->pExpr->pLeft->x.pList->a[i].pExpr; Expr *pRhs = pTerm->pExpr->pRight; if (pRhs->flags & EP_xIsSelect) { @@ -2288,11 +2290,10 @@ whereRangeVectorLen(Parse * pParse, /* Parsing context */ sqlite3TableColumnAffinity(space->def, pLhs->iColumn); if (aff != idxaff) break; - uint32_t unused; - pColl = sql_binary_compare_coll_seq(pParse, pLhs, pRhs, &unused); - if (pColl == 0) + uint32_t id = sql_binary_compare_coll_seq(pParse, pLhs, pRhs); + if (id == COLL_NONE) break; - if (idx_def->key_def->parts[i + nEq].coll != pColl) + if (idx_def->key_def->parts[i + nEq].coll_id != id) break; } return i; @@ -3269,16 +3270,18 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */ } if ((pTerm->eOperator & WO_EQ) != 0 && pOBExpr->iColumn >= 0) { - struct coll *coll1, *coll2; bool unused; - uint32_t id; - coll1 = sql_expr_coll(pWInfo->pParse, - pOrderBy->a[i].pExpr, - &unused, &id); - coll2 = sql_expr_coll(pWInfo->pParse, - pTerm->pExpr, - &unused, &id); - if (coll1 != coll2) + uint32_t lhs_id; + uint32_t rhs_id; + if (sql_expr_coll(pWInfo->pParse, + pOrderBy->a[i].pExpr, &unused, + &lhs_id) != 0) + return 0; + if (sql_expr_coll(pWInfo->pParse, + pTerm->pExpr, &unused, + &rhs_id) != 0) + return 0; + if (lhs_id != rhs_id) continue; } obSat |= MASKBIT(i); @@ -3401,13 +3404,10 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */ if (iColumn >= 0) { bool is_found; uint32_t id; - struct coll *coll = - sql_expr_coll(pWInfo->pParse, - pOrderBy->a[i].pExpr, - &is_found, &id); - struct coll *idx_coll = - idx_def->key_def->parts[j].coll; - if (coll != idx_coll) + sql_expr_coll(pWInfo->pParse, + pOrderBy->a[i].pExpr, + &is_found, &id); + if (idx_def->key_def->parts[j].coll_id != id) continue; } isMatch = 1; diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c index 342064ec8..e371cda35 100644 --- a/src/box/sql/whereexpr.c +++ b/src/box/sql/whereexpr.c @@ -37,6 +37,7 @@ * readability and editabiliity. This file contains utility routines for * analyzing Expr objects in the WHERE clause. */ +#include "box/coll_id_cache.h" #include "coll.h" #include "sqliteInt.h" #include "whereInt.h" @@ -167,7 +168,8 @@ exprCommute(Parse * pParse, Expr * pExpr) } else { bool is_found; uint32_t id; - sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id); + (void) sql_expr_coll(pParse, pExpr->pLeft, &is_found, + &id); if (id != COLL_NONE) { /* * Neither X nor Y have COLLATE @@ -851,17 +853,18 @@ termIsEquivalence(Parse * pParse, Expr * pExpr) ) { return 0; } - uint32_t unused; - struct coll *coll1 = - sql_binary_compare_coll_seq(pParse, pExpr->pLeft, pExpr->pRight, - &unused); - if (coll1 == NULL) + uint32_t id = sql_binary_compare_coll_seq(pParse, pExpr->pLeft, + pExpr->pRight); + if (id == COLL_NONE) return 1; bool unused1; - coll1 = sql_expr_coll(pParse, pExpr->pLeft, &unused1, &unused); - struct coll *coll2 = sql_expr_coll(pParse, pExpr->pRight, &unused1, - &unused); - return coll1 != NULL && coll1 == coll2; + uint32_t lhs_id; + uint32_t rhs_id; + if (sql_expr_coll(pParse, pExpr->pLeft, &unused1, &lhs_id) != 0) + return 0; + if (sql_expr_coll(pParse, pExpr->pRight, &unused1, &rhs_id) != 0) + return 0; + return lhs_id != COLL_NONE && lhs_id == rhs_id; } /* -- 2.15.1