[tarantool-patches] Re: [PATCH 3/3] sql: change collation compatibility rules

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

Thanks for the patch! See 5 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index a13de4f0e..bd25ed656 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -316,25 +315,62 @@ binaryCompareP5(Expr * pExpr1, Expr * pExpr2, int jumpIfNull)
>   	return aff;
>   }
>   
> +int
> +collations_are_compatible(uint32_t lhs_id, bool is_lhs_forced,
> +			  uint32_t rhs_id, bool is_rhs_forced,
> +			  uint32_t *res_id)

1. Why not to rename to _check_compatibility() and set
diag right here?

> +{
> +	assert(res_id != NULL);
> +	if (is_lhs_forced && is_rhs_forced) {
> +		if (lhs_id != rhs_id)
> +			return -1;
> +	}
> +	if (is_lhs_forced) {
> +		*res_id = lhs_id;
> +		return 0;
> +	}
> +	if (is_rhs_forced) {
> +		*res_id = rhs_id;
> +		return 0;
> +	}
> +	if (lhs_id != rhs_id) {
> +		if (lhs_id == COLL_NONE) {
> +			*res_id = rhs_id;
> +			return 0;
> +		}
> +		if (rhs_id == COLL_NONE) {
> +			*res_id = lhs_id;
> +			return 0;
> +		}
> +		return -1;
> +	}
> +	*res_id = lhs_id;
> +	return 0;
> +}
> +
>   struct coll *
>   sql_binary_compare_coll_seq(Parse *parser, Expr *left, Expr *right,
>   			    uint32_t *coll_id)
>   {
> -	struct coll *coll;
> -	bool is_found;
>   	assert(left != NULL);
> -	if ((left->flags & EP_Collate) != 0) {
> -		coll = sql_expr_coll(parser, left, &is_found, coll_id);
> -	} else if (right != NULL && (right->flags & EP_Collate) != 0) {
> -		coll = sql_expr_coll(parser, right, &is_found, coll_id);
> -	} else {
> -		coll = sql_expr_coll(parser, left, &is_found, coll_id);
> -		if (! is_found) {
> -			coll = sql_expr_coll(parser, right, &is_found,
> -					     coll_id);
> -		}
> +	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 (collations_are_compatible(lhs_coll_id, is_lhs_forced,
> +				      rhs_coll_id, is_rhs_forced,
> +				      coll_id) != 0) {
> +		diag_set(ClientError, ER_ILLEGAL_COLLATION_MIX);
> +		parser->rc = SQL_TARANTOOL_ERROR;
> +		parser->nErr++;
> +		*coll_id = COLL_NONE;

3. Why do you need to set coll_id in a case of an error? As I
understand, after an error the compilation is terminated.

> +		return NULL;
>   	}
> -	return coll;
> +	return *coll_id == rhs_coll_id ? rhs_coll : lhs_coll;;
>   }
>   
>   /*
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 1ec92aac7..c3403670b 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2035,8 +2035,9 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
>   		bool is_found;
>   		uint32_t coll_id;
>   		if (pTab->def->fields[i].coll_id == COLL_NONE &&
> -		    sql_expr_coll(pParse, p, &is_found, &coll_id) && is_found)
> +		    sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != COLL_NONE)
>   			pTab->def->fields[i].coll_id = coll_id;
> +		assert(pTab->def->fields[i].coll_id != COLL_BINARY);

4. Why? I can create a table with binary collation after the
previous commit.

>   	}
>   }
>   
> @@ -2223,47 +2224,53 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> +static uint32_t
> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
> +		      bool *is_forced_coll)
>   {
> -	struct coll *coll;
> +	bool is_prior_forced = false;
> +	bool is_current_forced;
> +	uint32_t prior_coll_id = COLL_NONE;
> +	uint32_t current_coll_id;
>   	if (p->pPrior != NULL) {
> -		coll = multi_select_coll_seq_r(parser, p->pPrior, n, is_found,
> -					       coll_id);
> -	} else {
> -		coll = NULL;
> -		*coll_id = COLL_NONE;
> +		prior_coll_id = multi_select_coll_seq(parser, p->pPrior, n,
> +						      &is_prior_forced);
>   	}
> -	assert(n >= 0);
> -	/* iCol must be less than p->pEList->nExpr.  Otherwise an error would
> -	 * have been thrown during name resolution and we would not have gotten
> -	 * this far
> +	/*
> +	 * Column number must be less than p->pEList->nExpr.
> +	 * Otherwise an error would have been thrown during name
> +	 * resolution and we would not have gotten this far.

5. 'have gotten'? Maybe 'have got'?

>   	 */
> -	if (!*is_found && ALWAYS(n < p->pEList->nExpr)) {
> -		coll = sql_expr_coll(parser, p->pEList->a[n].pExpr, is_found,
> -				     coll_id);
> -	}
> -	return coll;
> -}