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

[n.pettik]

>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index cea453f08..ab0376a1d 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -2150,47 +2151,51 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
>>  }
>>    #ifndef SQLITE_OMIT_COMPOUND_SELECT
>> -static struct coll *
>> -multi_select_coll_seq_r(Parse *parser, Select *p, int n, bool *is_found,
>> -			uint32_t *coll_id)
>> +/**
>> + * This function determines resulting collation sequence for
>> + * @n-th column of the result set for the compound SELECT
>> + * statement. Since compound SELECT performs implicit comparisons
>> + * between values, all parts of compound queries must use
>> + * the same collation. Otherwise, an error is raised.
>> + *
>> + * @param parser Parse context.
>> + * @param p Select meta-information.
>> + * @param n Column number of the result set.
>> + * @param is_forced_coll Used if we fall into recursion.
>> + *        For most-outer call it is unused. Used to indicate that
>> + *        explicit COLLATE clause is used.
>> + * @retval Id of collation to be used during string comparison.
>> + */
>> +static uint32_t
>> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n,
>> +		      bool *is_forced_coll)
> 
> 1. I renamed this function into multi_select_coll_seq_r and
> created a wrapper without is_forced_coll to remove some of
> 'bool unused' things. Apply or drop the fix, up to you.

Ok, I don’t mind your fixes. Applied.

>> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
>> index 9fa6ce15d..e05967554 100644
>> --- a/src/box/sql/whereexpr.c
>> +++ b/src/box/sql/whereexpr.c
>> @@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
>>  			bool is_found;
>>  			uint32_t id;
>>  			sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
>> -			if (is_found) {
>> +			if (id != 0) {
> 
> 2. If you fix a comment about COLL_NONE constant return,
> do not forget to pick this hunk up as well, please. Maybe
> this patch contains more, I am not sure.

Ok, I’ve returned COLL_NONE usages:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 09c248154..b67b22c23 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -360,11 +360,11 @@ collations_check_compatibility(uint32_t lhs_id, bool is_lhs_forced,
                return 0;
        }
        if (lhs_id != rhs_id) {
-               if (lhs_id == 0) {
+               if (lhs_id == COLL_NONE) {
                        *res_id = rhs_id;
                        return 0;
                }
-               if (rhs_id == 0) {
+               if (rhs_id == COLL_NONE) {
                        *res_id = lhs_id;
                        return 0;
                }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 2db8e1c7f..efdac9dba 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1963,7 +1963,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,            /* Parsing contexts */
                uint32_t coll_id;
 
                if (pTab->def->fields[i].coll_id == COLL_NONE &&
-                   sql_expr_coll(pParse, p, &is_found, &coll_id) && coll_id != 0)
+                   sql_expr_coll(pParse, p, &is_found, &coll_id) &&
+                   coll_id != COLL_NONE)
                        pTab->def->fields[i].coll_id = coll_id;
        }
 }
@@ -2172,7 +2173,7 @@ multi_select_coll_seq_r(struct Parse *parser, struct Select *p, int n,
 {
        bool is_prior_forced = false;
        bool is_current_forced;
-       uint32_t prior_coll_id = 0;
+       uint32_t prior_coll_id = COLL_NONE;
        uint32_t current_coll_id;
        if (p->pPrior != NULL) {
                prior_coll_id = multi_select_coll_seq_r(parser, p->pPrior, n,
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index e05967554..971f428b7 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -168,7 +168,7 @@ exprCommute(Parse * pParse, Expr * pExpr)
                        bool is_found;
                        uint32_t id;
                        sql_expr_coll(pParse, pExpr->pLeft, &is_found, &id);
-                       if (id != 0) {
+                       if (id != COLL_NONE) {
                                /*
                                 * Neither X nor Y have COLLATE
                                 * operators, but X has ag