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 patch!

See 3 comments below.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index aa565277c..dc99bd390 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -144,11 +144,10 @@ minmaxFunc(sql_context * context, int argc, sql_value ** argv)
>  	for (i = 1; i < argc; i++) {
>  		if (mem_is_null(argv[i]))
>  			return;
> -		if ((sqlMemCompare(argv[iBest], argv[i], pColl) ^ mask) >=
> -		    0) {
> -			testcase(mask == 0);
> +		int res;
> +		mem_cmp_scalar(argv[iBest], argv[i], &res, pColl);
> +		if ((res ^ mask) >= 0)

1. It seems that under certain conditions if cmp_scalar fails, res remains
not initialized. Which can lead to behaviour changing from run to run. The
same in the other places below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 2595e2fd4..576596c9f 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -59,6 +59,40 @@ enum {
>  	BUF_SIZE = 32,
>  };
>  
> +enum mem_class {
> +	MEM_CLASS_NULL,
> +	MEM_CLASS_BOOL,
> +	MEM_CLASS_NUMBER,
> +	MEM_CLASS_STR,
> +	MEM_CLASS_BIN,
> +	MEM_CLASS_UUID,
> +	mem_class_max,
> +};

2. It might make sense to add a comment that these must be sorted
exactly like enum mp_class.

> +
> +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.