From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3AF3A247A4
	for <tarantool-patches@freelists.org>; Fri, 16 Aug 2019 10:09:06 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
	by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
	with ESMTP id S2Ne6vI_W9Rw for <tarantool-patches@freelists.org>;
	Fri, 16 Aug 2019 10:09:06 -0400 (EDT)
Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 83E482469D
	for <tarantool-patches@freelists.org>; Fri, 16 Aug 2019 10:09:05 -0400 (EDT)
Date: Fri, 16 Aug 2019 17:09:02 +0300
From: Konstantin Osipov <kostja@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef
 function hash
Message-ID: <20190816140902.GC18643@atlas>
References: <cover.1565961886.git.kshcherbatov@tarantool.org>
 <f2c5281d2cdf2d5d7644521684f744313b99c6e2.1565961887.git.kshcherbatov@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f2c5281d2cdf2d5d7644521684f744313b99c6e2.1565961887.git.kshcherbatov@tarantool.org>
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-Help: <mailto:ecartis@freelists.org?Subject=help>
List-Unsubscribe: <mailto:tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-Subscribe: <mailto:tarantool-patches-request@freelists.org?Subject=subscribe>
List-Owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-Archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org
Cc: korablev@tarantool.org, Kirill Shcherbatov <kshcherbatov@tarantool.org>

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/08/16 16:29]:
> --- a/src/box/func.c
> +++ b/src/box/func.c
> @@ -39,6 +39,7 @@
>  #include "port.h"
>  #include "schema.h"
>  #include "session.h"
> +#include "sql.h"
>  #include <dlfcn.h>

Why?

> +struct func *
> +sql_func_by_signature(const char *name, int argc)
> +{
> +	struct func *func = func_by_name(name, strlen(name));
> +	if (func == NULL || !func->def->exports.sql)
> +		return NULL;
> +	if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> +		if (strcmp(func->def->name, "COALESCE") == 0) {
> +			if (argc == 0 || argc == 1)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "COUNT") == 0) {
> +			if (argc != 0 && argc != 1)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "GROUP_CONCAT") == 0) {
> +			if (argc != 1 && argc != 2)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "LIKE") == 0) {
> +			if (argc != 2 && argc != 3)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "ROUND") == 0) {
> +			if (argc != 1 && argc != 2)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "SUBSTR") == 0) {
> +			if (argc != 2 && argc != 3)
> +				return NULL;
> +			argc = func->def->param_count;
> +		} else if (strcmp(func->def->name, "TRIM") == 0) {
> +			if (argc != 1 && argc != 2 && argc != 3)
> +				return NULL;
> +			argc = func->def->param_count;
> +		}
> +	}
> +	if (func->def->param_count != -1 && argc != func->def->param_count)
> +		return NULL;
> +	return func;
> +}

Why can't you have this as a check() method of sql_builtin_func, set for the specific functions which nee
this check, why do you have to have a switch + strcmp during runtime?


overall, this patch is huge. It is frustrating that the previous
patches on the stack are tacking so long to push. They should have
been finished & pushed so that this patch could be split into
pieces and worked on faster.

Nikita, could you please finish your review of all the patches up
to this one?

-- 
Konstantin Osipov, Moscow, Russia