From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 6341E430409 for ; Sat, 22 Aug 2020 17:31:08 +0300 (MSK) References: <847d1f4dd9b5b582f178e23361bbb746d2f12c04.1597417321.git.imeevma@gmail.com> From: Vladislav Shpilevoy Message-ID: <59a52e9c-c405-55e2-55a5-22bac51471e5@tarantool.org> Date: Sat, 22 Aug 2020 16:31:06 +0200 MIME-Version: 1.0 In-Reply-To: <847d1f4dd9b5b582f178e23361bbb746d2f12c04.1597417321.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 10/10] sql: refactor sql/func.c 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! 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.