[tarantool-patches] Re: [PATCH 1/2] sql: refactor sql_expr_coll and sql_binary_compare_coll_seq functions
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Jan 24 21:28:59 MSK 2019
Hi! Thanks for the patch!
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.
Nevertheless, see 4 comments below.
>
> 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.
> *is_explicit_coll = true;
> break;
> }
> @@ -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.
> }
>
> /*
> 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.
> */
> -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.
> */
> -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 *);
More information about the Tarantool-patches
mailing list