Re: [Tarantool-patches] [PATCH v2 10/10] sql: refactor sql/func.c

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

Thanks for the patch!

See 3 comments below.

On 14.08.2020 17:05, imeevma@tarantool.org wrote:
> After changing the way of checking the types of arguments, some of the
> code in sql/func.c is no longer used. This patch removes this code.
> 
> Follow-up of #4159
> ---
>  src/box/sql/func.c | 841 +++++----------------------------------------
>  1 file changed, 87 insertions(+), 754 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index c289f2de0..de1e4d13d 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c

1. Why didn't you simplify some funtions?

- lengthFunc(). Is it allowed to pass numbers into it legally?
  Perhaps it is, I just don't remember.

- position_func(). It has a some complicated check of argument
  types. I think now you can make it a bit simpler.

- lower/upper(). They still check for non-blob argument.

- like(). Also still checks for non-str types.

> @@ -504,44 +504,21 @@ absFunc(sql_context * context, int argc, sql_value ** argv)
>  {
>  	assert(argc == 1);
>  	UNUSED_PARAMETER(argc);
> -	switch (sql_value_type(argv[0])) {
> -	case MP_UINT: {
> -		sql_result_uint(context, sql_value_uint64(argv[0]));
> -		break;
> -	}
> -	case MP_INT: {
> +	enum mp_type type = sql_value_type(argv[0]);
> +	if (type == MP_NIL)
> +		return sql_result_null(context);
> +	if (type == MP_UINT)
> +		return sql_result_uint(context, sql_value_uint64(argv[0]));
> +	if (type == MP_INT) {

2. Would look better as a switch.

>  		int64_t value = sql_value_int64(argv[0]);
>  		assert(value < 0);
> -		sql_result_uint(context, -value);
> -		break;
> -	}
> @@ -2229,703 +2189,76 @@ static struct {
>  	uint16_t flags;
>  	void (*call)(sql_context *ctx, int argc, sql_value **argv);
>  	void (*finalize)(sql_context *ctx);
> -	/** Members below are related to struct func_def. */
> -	bool is_deterministic;
> -	int param_count;
> -	enum field_type returns;
> -	enum func_aggregate aggregate;
> -	bool export_to_sql;
>  } sql_builtins[] = {
> -	{.name = "ABS",
> -	 .param_count = 1,
> -	 .returns = FIELD_TYPE_NUMBER,
> -	 .aggregate = FUNC_AGGREGATE_NONE,
> -	 .is_deterministic = true,
> -	 .flags = 0,
> -	 .call = absFunc,
> -	 .finalize = NULL,
> -	 .export_to_sql = true,
> -	}, {

3. These removals I don't like. I think we don't need to expose anything
into _func. All could be done in there instead, and would look simpler,
IMO. As I explained in the previous emails in this thread.