Re: [Tarantool-patches] [PATCH v2 03/10] sql: change signature of trim()

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

Thanks for the patch!

On 14.08.2020 17:04, imeevma@tarantool.org wrote:
> This patch changes the signature of the SQL built-in trim() function.
> This makes it easier to define a function in _func and fixes a bug where
> the function loses collation when the BOTH, LEADING, or TRAILING
> keywords are specified.

I am not sure I understand. Did you break the backward compatibility by
changing the public function? Or did you change only internal implementation?
What exactly has changed? Where is refactoring, and where is the bugfix?
What is so hard about its signature now, that it does not allow to define it
in _func?

If this is a bugfix, it should be on a separate branch, with changelog,
so as it could be pushed to the older versions, according to our policy.

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index affb285aa..e5da21191 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1776,32 +1772,30 @@ static void
>  trim_func_two_args(struct sql_context *context, sql_value *arg1,
>  		   sql_value *arg2)
>  {
> -	const unsigned char *input_str, *trim_set;
> -	if ((input_str = sql_value_text(arg2)) == NULL)
> -		return;
> -
> -	int input_str_sz = sql_value_bytes(arg2);
> -	if (sql_value_type(arg1) == MP_INT || sql_value_type(arg1) == MP_UINT) {
> -		uint8_t len_one = 1;
> -		trim_procedure(context, sql_value_int(arg1),
> -			       (const unsigned char *) " ", &len_one, 1,
> -			       input_str, input_str_sz);
> -	} else if ((trim_set = sql_value_text(arg1)) != NULL) {
> -		int trim_set_sz = sql_value_bytes(arg1);
> -		uint8_t *char_len;
> -		int char_cnt = trim_prepare_char_len(context, trim_set,
> -						     trim_set_sz, &char_len);
> -		if (char_cnt == -1)
> -			return;
> -		trim_procedure(context, TRIM_BOTH, trim_set, char_len, char_cnt,
> -			       input_str, input_str_sz);
> -		sql_free(char_len);
> -	}
> +	assert(sql_value_type(arg2) == MP_UINT);
> +	enum mp_type type = sql_value_type(arg1);
> +	if (type == MP_NIL)
> +		return sql_result_null(context);
> +	const unsigned char *input_str = sql_value_text(arg1);
> +	const unsigned char *trim_set;
> +
> +	int input_str_sz = sql_value_bytes(arg1);
> +	uint8_t len_one = 1;
> +	if (type == MP_BIN)
> +		trim_set = (const unsigned char *) "\0";
> +	else
> +		trim_set = (const unsigned char *) " ";
> +	trim_procedure(context, sql_value_int(arg2), trim_set,  &len_one, 1,
> +		       input_str, input_str_sz);

Why did you move handling of the "TRIM(<character_set> FROM <str>)"
case to another function? That breaks the trim functions idea of having
trim_func_one_arg, trim_func_two_args, and trim_func_three_args separated.
After your patch trim_func_three_args handles both 2 and 3 arguments,
and trim_func_two_args handles not both options with 2 arguments.

You either need to keep the different argument count handling inside the
existing functions, or change their names somehow to reflect the new
behaviour.