[tarantool-patches] Re: [PATCH 2/2] sql: compute resulting collation for concatenation

[n.pettik]

>>>> +			bool is_rhs_forced;
>>>> +			uint32_t rhs_coll_id;
>>>> +			if (sql_expr_coll(parse, p->pRight, &is_rhs_forced,
>>>> +					  &rhs_coll_id) != 0)
>>>> +				return -1;
>>>> +			if (is_lhs_forced && is_rhs_forced) {
>>>> +				if (lhs_coll_id != rhs_coll_id)
>>>> +					return -1;
>>> 
>>> 5. Did you miss diag_set?
>> No, I did it on purpose. Firstly, this function is recursive,
>> so in case error occurred on bottom levels of recursion,
>> diag would be reseted on each level above (and parser’s
>> counter of errors would be incremented several times as
>> well). No terrible consequences would take place in this case,
>> but it looks like a bad pattern. Anyway, fixed it according to
>> your suggestion.
> 
> Each ClientError is accounted in a global errors counter. It
> means, that the same error should be set multiple times, otherwise
> the statistics would be wrong. Instead of resetting diag each time
> check if parser->nErr > 0, and if it is, then do nothing.
> diag_is_empty() can not be used, because it happens sometimes, that
> there are no errors, but diag is not cleared from a previous error.

Hmm, Mergen is trying to remove nErr, if I’m not mistaken.
So, soon I guess we will be able to use diag_is_empty().
Now I’ve implemented your suggestion. Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 680075792..0de9a46a5 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -246,10 +246,17 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
                                return -1;
                        if (is_lhs_forced && is_rhs_forced) {
                                if (lhs_coll_id != rhs_coll_id) {
-                                       diag_set(ClientError,
-                                                ER_ILLEGAL_COLLATION_MIX);
-                                       parse->nErr++;
-                                       parse->rc = SQL_TARANTOOL_ERROR;
+                                       /*
+                                        * Don't set the same error
+                                        * several times: this
+                                        * function is recursive.
+                                        */
+                                       if (parse->nErr == 0) {
+                                               diag_set(ClientError,
+                                                        ER_ILLEGAL_COLLATION_MIX);
+                                               parse->nErr++;
+                                               parse->rc = SQL_TARANTOOL_ERROR;
+                                       }
                                        return -1;
                                }
                        }