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

[n.pettik]

After discussion with Konstantin in @dev mailing list, we have
decided to introduce real entities in _collation space to represent
binary and “none” collations. Hence, I am going to send updated
patch-set as second iteration (v2).
Also, I’ve took your comments into consideration.

>> 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?

Just another way to implement this. In updated patch I did
it as you suggested.

>> +{
>> +	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.

Just in case, it shouldn’t be really used. I removed it.

> 
>> +		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.

I guess it is debug remains. Removed.

> 
>>  	}
>>  }
>>  @@ -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’?

Yep, fixed.