Re: [Tarantool-patches] [PATCH v1 02/10] sql: rework SQL built-in functions hash table

[Vladimir Davydov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

On Fri, Aug 13, 2021 at 06:17:07AM +0300, imeevma@tarantool.org wrote:
> @@ -2615,23 +2099,30 @@ built_in_func_put(struct func *func)
>  	}
>  }
>  
> +static struct func *
> +find_built_in_func(struct Expr *expr, struct sql_func_dictionary *dict)
> +{
> +	const char *name = expr->u.zToken;
> +	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
> +	struct func *func = &dict->functions[0]->base;
> +	assert(func->def->exports.sql);
> +	int param_count = func->def->param_count;
> +	if (param_count != -1 && param_count != n) {
> +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> +			 tt_sprintf("%d", func->def->param_count), n);
> +		return NULL;
> +	}
> +	return func;
> +}
> +
>  struct func *
>  sql_func_find(struct Expr *expr)
>  {
>  	const char *name = expr->u.zToken;
> -	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
> -	struct func *func = built_in_func_get(name);
> -	if (func != NULL) {
> -		assert(func->def->exports.sql);
> -		int param_count = func->def->param_count;
> -		if (param_count != -1 && param_count != n) {
> -			diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> -				 tt_sprintf("%d", func->def->param_count), n);
> -			return NULL;
> -		}
> -		return func;
> -	}
> -	func = func_by_name(name, strlen(name));
> +	struct sql_func_dictionary *dict = built_in_func_get(name);
> +	if (dict != NULL)
> +		return find_built_in_func(expr, dict);
> +	struct func *func = func_by_name(name, strlen(name));
>  	if (func == NULL) {
>  		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
>  		return NULL;

Having a separate (from _func) two-level (name -> dictionary -> func)
hash for built-in functions looks ugly. Treating built-in functions and
user-defined functions differently here, but using the same struct func
for both kinds looks ugly as well. The old code, where both built-in
functions and user-defined functions lived in the same hash (and in
_func system space), looked consistent and neat.

If our goal is to enable static type checking of function arguments, we
can instead add a callback to struct func ('check'), which would raise
an error if supplied arguments are incorrect without invoking the
function. We wouldn't have a separate 'execute' callback for each
variant of the same function then, because all variants would share the
same struct func, like they do now, but do we really need it?

Another idea is to move built-in function name resolution completely to
the parser. Then we wouldn't need a hash for built-in functions at all
(nor would we need to have them in _func). I find this idea particularly
appealing, because after all built-in functions are like operators -
their names and argument types are known at the parse time - so
theoretically we could generate a separate opcode for them with all the
necessary information prepared by the paser. Please check out if this
is doable.