[tarantool-patches] Re: [PATCH 1/2] sql: refactor sql_expr_coll and sql_binary_compare_coll_seq functions

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 *);