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

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

On 22/02/2019 16:40, n.pettik wrote:
> 
>>>>> +			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:

Yes, after Mergen did, we can remove nErr from there. Now
your patch does not increase nErr usage, so it is ok.

The whole patchset LGTM.