[tarantool-patches] Re: [PATCH 1/2] sql: refactor sql_expr_coll and sql_binary_compare_coll_seq functions
n.pettik
korablev at tarantool.org
Fri Feb 15 02:26:21 MSK 2019
> On 16/01/2019 16:13, Nikita Pettik wrote:
>> 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.
>
> First of all, I agree with Kostja - it does not help in anything that
> you removed coll * from output parameters/return value. It just slows
> down some places like codeCompare, sqlite3ExprCodeIN, sqlite3ExprCodeTarget
> etc. - you just do coll_by_id() twice.
> Moreover, maybe making some
> places more accurate and compact, it pads out other ones and involves
> additional headers like in whereexpr.c. Please, return as it was.
Ok, then I will move struct coll * to output params for sql_expr_coll().
As for sql_binary_compare_coll_seq(), struct coll is needed only once -
in codeCompare(); other 5 usages of this function leads to throwing away
struct coll. In this case I believe it is really reasonable to operate only
with id. Anyway, id moved to output params, now it returns error code.
>> 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
>> @@ -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);
>
> 1. It is not necessary to prefix (void) each function result of which
> you ignore. By the way, I do not understand why do you ignore it. Here
> and in all other places.
Yep, it is not necessary, but this way is more descriptive.
Refactored whole function, so it is not longer ignored.
>> @@ -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;
>
> 2. Please, do not return some obscure error codes - we use only 0/-1
> and NULL/not NULL almost everywhere except some archaic or misreviewed
> code. If you want to return an error, it is better to move coll_id to
> out parameters.
Ok, that’s what I did. See diff below.
>> 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.
>
> 3. Please, use separate @retval for each value.
As you wish. It is not matter of discussion, but this
rule seems to be inconsistent: somewhere we use
one retval clause for all possible return values;
in other places - several ones.
>> */
>> -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.
>
> 4. You didn't mentioned that it can be COLL_NONE, that means an error.
Skipped this comment due to refactoring of this function.
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index a367a07d4..5617e0f62 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -193,10 +193,13 @@ sqlExprSkipCollate(Expr * pExpr)
}
int
-sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
+sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
+ struct coll **coll)
{
+ assert(coll != NULL);
*is_explicit_coll = false;
*coll_id = COLL_NONE;
+ *coll = NULL;
while (p != NULL) {
int op = p->op;
if (op == TK_CAST || op == TK_UPLUS) {
@@ -205,7 +208,9 @@ 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)) {
- (void) sql_get_coll_seq(parse, p->u.zToken, coll_id);
+ *coll = sql_get_coll_seq(parse, p->u.zToken, coll_id);
+ if (coll == NULL)
+ return -1;
*is_explicit_coll = true;
break;
}
@@ -217,8 +222,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
*/
int j = p->iColumn;
if (j >= 0) {
- (void) sql_column_collation(p->space_def, j,
- coll_id);
+ *coll = sql_column_collation(p->space_def, j,
+ coll_id);
}
break;
}
@@ -335,28 +340,30 @@ illegal_collation_mix:
return -1;
}
-uint32_t
-sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right)
+int
+sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right,
+ uint32_t *id)
{
assert(left != NULL);
+ assert(id != NULL);
bool is_lhs_forced;
bool is_rhs_forced;
uint32_t lhs_coll_id;
uint32_t 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;
+ struct coll *unused;
+ if (sql_expr_coll(parser, left, &is_lhs_forced, &lhs_coll_id,
+ &unused) != 0)
+ return -1;
+ if (sql_expr_coll(parser, right, &is_rhs_forced, &rhs_coll_id,
+ &unused) != 0)
+ return -1;
if (collations_check_compatibility(lhs_coll_id, is_lhs_forced,
- rhs_coll_id, is_rhs_forced,
- &coll_id) != 0)
- goto err;
- return coll_id;
-err:
- parser->rc = SQL_TARANTOOL_ERROR;
- parser->nErr++;
- return COLL_NONE;
+ rhs_coll_id, is_rhs_forced, id) != 0) {
+ parser->rc = SQL_TARANTOOL_ERROR;
+ parser->nErr++;
+ return -1;
+ }
+ return 0;
}
/*
@@ -372,7 +379,9 @@ codeCompare(Parse * pParse, /* The parsing (and code generating) context */
int jumpIfNull /* If true, jump if either operand is NULL */
)
{
- uint32_t id = sql_binary_compare_coll_seq(pParse, pLeft, pRight);
+ uint32_t id;
+ if (sql_binary_compare_coll_seq(pParse, pLeft, pRight, &id) != 0)
+ return -1;
struct coll *coll = coll_by_id(id)->coll;
int p5 = binaryCompareP5(pLeft, pRight, jumpIfNull);
int addr = sqlVdbeAddOp4(pParse->pVdbe, opcode, in2, dest, in1,
@@ -2428,8 +2437,9 @@ sqlFindInIndex(Parse * pParse, /* Parsing context */
for (i = 0; i < nExpr; i++) {
Expr *pLhs = sqlVectorFieldSubexpr(pX->pLeft, i);
Expr *pRhs = pEList->a[i].pExpr;
- uint32_t id =
- sql_binary_compare_coll_seq(pParse, pLhs, pRhs);
+ uint32_t id;
+ if (sql_binary_compare_coll_seq(pParse, pLhs, pRhs, &id) != 0)
+ break;
int j;
for (j = 0; j < nExpr; j++) {
@@ -2749,9 +2759,9 @@ sqlCodeSubselect(Parse * pParse, /* Parsing context */
Expr *p =
sqlVectorFieldSubexpr
(pLeft, i);
- key_info->parts[i].coll_id =
- sql_binary_compare_coll_seq(pParse, p,
- pEList->a[i].pExpr);
+ if (sql_binary_compare_coll_seq(pParse, p, pEList->a[i].pExpr,
+ &key_info->parts[i].coll_id) != 0)
+ return 0;
}
}
} else if (ALWAYS(pExpr->x.pList != 0)) {
@@ -2770,9 +2780,11 @@ sqlCodeSubselect(Parse * pParse, /* Parsing context */
enum field_type lhs_type =
sql_expr_type(pLeft);
bool unused;
- (void) sql_expr_coll(pParse, pExpr->pLeft,
- &unused,
- &key_info->parts[0].coll_id);
+ struct coll *unused_coll;
+ if (sql_expr_coll(pParse, pExpr->pLeft, &unused,
+ &key_info->parts[0].coll_id,
+ &unused_coll) != 0)
+ return 0;
/* Loop through each expression in <exprlist>. */
r1 = sqlGetTempReg(pParse);
@@ -3035,9 +3047,10 @@ sqlExprCodeIN(Parse * pParse, /* Parsing and code generating context */
bool unused;
uint32_t id;
ExprList *pList = pExpr->x.pList;
- if (sql_expr_coll(pParse, pExpr->pLeft, &unused, &id) != 0)
+ struct coll *coll;
+ if (sql_expr_coll(pParse, pExpr->pLeft, &unused, &id,
+ &coll) != 0)
goto sqlExprCodeIN_finished;
- struct coll *coll = coll_by_id(id)->coll;
int labelOk = sqlVdbeMakeLabel(v);
int r2, regToFree;
int regCkNull = 0;
@@ -3169,9 +3182,9 @@ sqlExprCodeIN(Parse * pParse, /* Parsing and code generating context */
uint32_t id;
int r3 = sqlGetTempReg(pParse);
Expr *p = sqlVectorFieldSubexpr(pLeft, i);
- if (sql_expr_coll(pParse, p, &unused, &id) != 0)
+ struct coll *pColl;
+ if (sql_expr_coll(pParse, p, &unused, &id, &pColl) != 0)
goto sqlExprCodeIN_finished;
- struct coll *pColl = coll_by_id(id)->coll;
/* Tarantool: Replace i -> aiMap [i], since original order of columns
* is preserved.
*/
@@ -4001,9 +4014,9 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
uint32_t id;
if (sql_expr_coll(pParse,
pFarg->a[i].pExpr,
- &unused, &id) != 0)
- break;
- coll = coll_by_id(id)->coll;;
+ &unused, &id,
+ &coll) != 0)
+ return 0;
}
}
if (pFarg) {
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index aed40455b..926f5d011 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1042,11 +1042,12 @@ selectInnerLoop(Parse * pParse, /* The parser context */
for (i = 0; i < nResultCol; i++) {
bool is_found;
uint32_t id;
+ struct coll *coll;
if (sql_expr_coll(pParse,
pEList->a[i].pExpr,
- &is_found, &id) != 0)
+ &is_found, &id,
+ &coll) != 0)
break;
- struct coll *coll = coll_by_id(id)->coll;
if (i < nResultCol - 1) {
sqlVdbeAddOp3(v, OP_Ne,
regResult + i,
@@ -1428,7 +1429,9 @@ 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;
- if (sql_expr_coll(parse, item->pExpr, &unused, &id) != 0) {
+ struct coll *unused_coll;
+ if (sql_expr_coll(parse, item->pExpr, &unused, &id,
+ &unused_coll) != 0) {
sqlDbFree(parse->db, key_info);
return NULL;
}
@@ -1970,10 +1973,10 @@ sqlSelectAddColumnTypeAndCollation(Parse * pParse, /* Parsing contexts */
pTab->def->fields[i].type = sql_expr_type(p);
bool is_found;
uint32_t coll_id;
-
+ struct coll *unused;
if (pTab->def->fields[i].coll_id == COLL_NONE &&
- sql_expr_coll(pParse, p, &is_found, &coll_id) == 0 &&
- coll_id != COLL_NONE)
+ sql_expr_coll(pParse, p, &is_found, &coll_id,
+ &unused) == 0 && coll_id != COLL_NONE)
pTab->def->fields[i].coll_id = coll_id;
}
}
@@ -2221,9 +2224,10 @@ 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);
+ struct coll *unused;
if (sql_expr_coll(parser, p->pEList->a[n].pExpr, &is_current_forced,
- ¤t_coll_id) != 0)
- return COLL_NONE;
+ ¤t_coll_id, &unused) != 0)
+ return 0;
uint32_t res_coll_id;
if (collations_check_compatibility(prior_coll_id, is_prior_forced,
current_coll_id, is_current_forced,
@@ -2279,7 +2283,10 @@ sql_multiselect_orderby_to_key_info(struct Parse *parse, struct Select *s,
uint32_t id;
bool unused;
if ((term->flags & EP_Collate) != 0) {
- (void) sql_expr_coll(parse, term, &unused, &id);
+ struct coll *unused_coll;
+ if (sql_expr_coll(parse, term, &unused, &id,
+ &unused_coll) != 0)
+ return 0;
} else {
id = multi_select_coll_seq(parse, s,
item->u.x.iOrderByCol - 1);
@@ -5357,9 +5364,8 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
for (j = 0, pItem = pList->a; coll == NULL && j < nArg;
j++, pItem++) {
if (sql_expr_coll(pParse, pItem->pExpr,
- &unused, &id) != 0)
+ &unused, &id, &coll) != 0)
return;
- coll = coll_by_id(id)->coll;
}
if (regHit == 0 && pAggInfo->nAccumulator)
regHit = ++pParse->nMem;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 87cdbbcc1..70f971f8c 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4344,12 +4344,14 @@ const char *sqlErrStr(int);
* @param[out] is_explicit_coll Flag set if explicit COLLATE
* clause is used.
* @param[out] coll_id Collation identifier.
+ * @param[out] coll Collation object.
*
- * @retval Return code: < 0 in case of error, 0 on success.
+ * @retval 0 on success.
+ * @retval -1 in case of error.
*/
int
-sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll,
- uint32_t *coll_id);
+sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
+ struct coll **coll);
Expr *sqlExprAddCollateToken(Parse * pParse, Expr *, const Token *, int);
Expr *sqlExprAddCollateString(Parse *, Expr *, const char *);
@@ -4681,11 +4683,14 @@ 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] id Id of resulting collation.
*
- * @retval Id of collation object.
+ * @retval 0 on success.
+ * @retval -1 in case of error (e.g. no collation found).
*/
-uint32_t
-sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right);
+int
+sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right,
+ uint32_t *id);
int sqlTempInMemory(const sql *);
#ifndef SQL_OMIT_CTE
With *sqlWithAdd(Parse *, With *, Token *, ExprList *, Select *);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index f27de5dd7..631a5feba 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -306,10 +306,12 @@ whereScanNext(WhereScan * pScan)
Parse *pParse =
pWC->pWInfo->pParse;
assert(pX->pLeft);
- uint32_t id =
- sql_binary_compare_coll_seq(
- pParse, pX->pLeft,
- pX->pRight);
+ uint32_t id;
+ if (sql_binary_compare_coll_seq(
+ pParse, pX->pLeft,
+ pX->pRight, &id) != 0)
+ break;
struct coll *coll =
id != COLL_NONE ?
coll_by_id(id)->coll :
@@ -559,8 +561,9 @@ findIndexCol(Parse * pParse, /* Parse context */
p->iColumn == (int) part_to_match->fieldno) {
bool is_found;
uint32_t id;
+ struct coll *unused;
if (sql_expr_coll(pParse, pList->a[i].pExpr,
- &is_found, &id) != 0)
+ &is_found, &id, &unused) != 0)
return -1;
if (id == part_to_match->coll_id)
return i;
@@ -2280,7 +2283,9 @@ whereRangeVectorLen(Parse * pParse, /* Parsing context */
space->def->fields[pLhs->iColumn].type : FIELD_TYPE_INTEGER;
if (type != idx_type)
break;
- uint32_t id = sql_binary_compare_coll_seq(pParse, pLhs, pRhs);
+ uint32_t id;
+ if (sql_binary_compare_coll_seq(pParse, pLhs, pRhs, &id) != 0)
+ break;
if (id == COLL_NONE)
break;
if (idx_def->key_def->parts[i + nEq].coll_id != id)
@@ -3264,13 +3269,14 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
bool unused;
uint32_t lhs_id;
uint32_t rhs_id;
+ struct coll *unused_coll;
if (sql_expr_coll(pWInfo->pParse,
pOrderBy->a[i].pExpr, &unused,
- &lhs_id) != 0)
+ &lhs_id, &unused_coll) != 0)
return 0;
if (sql_expr_coll(pWInfo->pParse,
pTerm->pExpr, &unused,
- &rhs_id) != 0)
+ &rhs_id, &unused_coll) != 0)
return 0;
if (lhs_id != rhs_id)
continue;
@@ -3395,9 +3401,11 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
if (iColumn >= 0) {
bool is_found;
uint32_t id;
- sql_expr_coll(pWInfo->pParse,
- pOrderBy->a[i].pExpr,
- &is_found, &id);
+ struct coll *unused;
+ if (sql_expr_coll(pWInfo->pParse,
+ pOrderBy->a[i].pExpr,
+ &is_found, &id, &unused) != 0)
+ return 0;
if (idx_def->key_def->parts[j].coll_id != id)
continue;
}
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 758c92896..703980f02 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -168,8 +168,10 @@ exprCommute(Parse * pParse, Expr * pExpr)
} else {
bool is_found;
uint32_t id;
- (void) sql_expr_coll(pParse, pExpr->pLeft, &is_found,
- &id);
+ struct coll *unused;
+ if (sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id,
+ &unused) != 0)
+ return;
if (id != COLL_NONE) {
/*
* Neither X nor Y have COLLATE
@@ -849,16 +851,21 @@ termIsEquivalence(Parse * pParse, Expr * pExpr)
if (lhs_type != rhs_type && (!sql_type_is_numeric(lhs_type) ||
!sql_type_is_numeric(rhs_type)))
return 0;
- uint32_t id = sql_binary_compare_coll_seq(pParse, pExpr->pLeft,
- pExpr->pRight);
+ uint32_t id;
+ if (sql_binary_compare_coll_seq(pParse, pExpr->pLeft, pExpr->pRight,
+ &id) != 0)
+ return 0;
if (id == COLL_NONE)
return 1;
bool unused1;
uint32_t lhs_id;
uint32_t rhs_id;
- if (sql_expr_coll(pParse, pExpr->pLeft, &unused1, &lhs_id) != 0)
+ struct coll *unused;
+ if (sql_expr_coll(pParse, pExpr->pLeft, &unused1, &lhs_id,
+ &unused) != 0)
return 0;
- if (sql_expr_coll(pParse, pExpr->pRight, &unused1, &rhs_id) != 0)
+ if (sql_expr_coll(pParse, pExpr->pRight, &unused1, &rhs_id,
+ &unused) != 0)
return 0;
return lhs_id != COLL_NONE && lhs_id == rhs_id;
}
More information about the Tarantool-patches
mailing list