From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 04DC0430409 for ; Sat, 22 Aug 2020 17:26:23 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <8e1ae2cb-5bcb-16cd-7086-2cd6e8dbeca4@tarantool.org> Date: Sat, 22 Aug 2020 16:26:21 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 03/10] sql: change signature of trim() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.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( FROM )" 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.