Re: [Tarantool-patches] [PATCH v2 4/4] sql: introduce mem_cmp_msgpack()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thank you for the review! My answer below.

On 15.07.2021 00:53, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
>>>> +		if (mem_cmp_msgpack(mem, &p, &rc, coll) != 0)
>>>> +			rc = 0;
>>>>   		if (rc != 0) {
>>>> -			if (unpacked->key_def->parts[i].sort_order !=
>>>> -			    SORT_ORDER_ASC)
>>>> +			if (part->sort_order == SORT_ORDER_ASC)
>>> Why did you change != to ==?
>>>
>> I did this because function sqlVdbeCompareMsgpack() compared packed value as
>> left operand and MEM as right operand. In mem_cmp_msgpack() order was reversed,
>> now MEM is left operand and packed value is right operand.
> Shouldn't you then change the other place too?
There I changed returned value instead of operation.
>
>> @@ -2721,13 +2586,15 @@ sqlVdbeRecordCompareMsgpack(const void *key1,
>>   	n = MIN(n, key2->nField);
>>   
>>   	for (i = 0; i != n; i++) {
>> -		rc = sqlVdbeCompareMsgpack((const char**)&key1, key2, i);
>> +		struct key_part *part = &key2->key_def->parts[i];
>> +		struct Mem *mem = key2->aMem + i;
>> +		struct coll *coll = part->coll;
>> +		if (mem_cmp_msgpack(mem, (const char **)&key1, &rc, coll) != 0)
>> +			rc = 0;
>>   		if (rc != 0) {
>> -			if (key2->key_def->parts[i].sort_order !=
>> -			    SORT_ORDER_ASC) {
>> -				rc = -rc;
>> -			}
>> -			return rc;
>> +			if (part->sort_order != SORT_ORDER_ASC)
>> +				return rc;
>> +			return -rc;
>>   		}
> Here it was cmp(key1, key), now it is cmp(key2, key1). Shouldn't
> then the sort_order check become `== SORT_ORDER_ASC`?
Although it is true that I have not changed '!=' to '==', now in case 
'!=' the
function returns rc, and in the case of '==' it returns -rc. Previously 
it was
-rc for '!=' and rc for '=='. Is there any other reason to change '!=' 
to '=='?