[tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov at 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