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

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the fixes!

>>> +static inline enum mem_class
>>> +mem_type_class(enum mem_type type)
>>> +{
>>> +	switch (type) {
>>> +	case MEM_TYPE_NULL:
>>> +		return MEM_CLASS_NULL;
>>> +	case MEM_TYPE_UINT:
>>> +	case MEM_TYPE_INT:
>>> +	case MEM_TYPE_DOUBLE:
>>> +		return MEM_CLASS_NUMBER;
>>> +	case MEM_TYPE_STR:
>>> +		return MEM_CLASS_STR;
>>> +	case MEM_TYPE_BIN:
>>> +		return MEM_CLASS_BIN;
>>> +	case MEM_TYPE_BOOL:
>>> +		return MEM_CLASS_BOOL;
>>> +	case MEM_TYPE_UUID:
>>> +		return MEM_CLASS_UUID;
>>
>> 3. It might work faster without branching if done like
>> 'static enum mp_class mp_classes[]' - would allow to take
>> the class for any type as simple as an array access
>> operation.
> This cannot be done directly in current design since value in enum mem_type are
> not sequential numbers. However, this can be done using something like this:

Ok, I see now. This is because I asked to make them bit fields
before, shame on me. There is another option - use `32 - bit_ctz_u32()`
to get a number from [1, 13] range and use it as an index in a small
array. The problem here is that this is a bit crazy and I can't be sure
if it works faster than this switch-case on all platforms. Hence, lets
leave it as is now then.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 32d02d96e..220e8b269 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1828,7 +1828,7 @@ case OP_Compare: {
>  		assert(i < (int)def->part_count);
>  		struct coll *coll = def->parts[i].coll;
>  		bool is_rev = def->parts[i].sort_order == SORT_ORDER_DESC;
> -		iCompare = sqlMemCompare(&aMem[p1+idx], &aMem[p2+idx], coll);
> +		mem_cmp_scalar(&aMem[p1+idx], &aMem[p2+idx], &iCompare, coll);

Why don't you check the result here? AFAIU, previously
sqlMemCompare() never returned something undefined. Now iCompare
might be left garbage leading to unstable behaviour. The same
below.

>  		if (iCompare) {
>  			if (is_rev)
>  				iCompare = -iCompare;
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index e5f35fbf8..dadc6d4a2 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -1272,12 +1272,14 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
>  			rc = sql_stat4_column(db, samples[i].sample_key, nEq,
>  					      &pVal);
>  			if (rc == 0 && p1 != NULL) {
> -				int res = sqlMemCompare(p1, pVal, coll);
> +				int res;
> +				mem_cmp_scalar(p1, pVal, &res, coll);
>  				if (res >= 0)
>  					nLower++;
>  			}
>  			if (rc == 0 && p2 != NULL) {
> -				int res = sqlMemCompare(p2, pVal, coll);
> +				int res;
> +				mem_cmp_scalar(p2, pVal, &res, coll);
>  				if (res >= 0)
>  					nUpper++;
>  			}