[tarantool-patches] Re: [PATCH v1 2/3] sql: ban sql functions coinciding with builtins

[n.pettik]

> On 17 May 2019, at 11:22, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
> The code of the original sqlFindFunction function does not match
> our codestyle and is difficult to maintain and extend. This patch
> does not make any functional changes in Tarantool, only optimizes
> the sqlFindFunction function,
> 
> Needed #3691
> ---
> diff --git a/src/box/lua/lua_sql.c b/src/box/lua/lua_sql.c
> index 36b75ff08..561353520 100644
> --- a/src/box/lua/lua_sql.c
> +++ b/src/box/lua/lua_sql.c
> @@ -195,7 +195,11 @@ lbox_sql_create_function(struct lua_State *L)
> 					   is_deterministic ? SQL_DETERMINISTIC : 0,
> 					   func_info, lua_sql_call, NULL, NULL,
> 					   lua_sql_destroy);
> -	if (rc != 0)
> -		return luaL_error(L, sqlErrStr(rc));
> +	if (rc != 0) {
> +		const char *err = rc == SQL_TARANTOOL_ERROR ?
> +				  box_error_message(box_error_last()) :
> +				  sqlErrStr(rc);
> +		return luaL_error(L, err);
> +	}

TBO, this looks more like functional change requiring separate test.

> 	return 0;
> }
> diff --git a/src/box/sql/callback.c b/src/box/sql/callback.c
> index 49197532e..54d59bbd3 100644
> --- a/src/box/sql/callback.c
> +++ b/src/box/sql/callback.c
> @@ -131,6 +131,19 @@ functionSearch(int h,		/* Hash of the name */
> 	return 0;
> }
> 
> +/**
> + * Cache function is used to organise sqlBuiltinFunctions table.
> + * @param func_name The name of builtin function.
> + * @param func_name_len The length of the @a name.
> + * @retval Hash value is calculated for given name.
> + */
> +static int

Why not inline?

> +sql_builtin_func_name_hash(const char *func_name, uint32_t func_name_len)
> +{
> +	return (sqlUpperToLower[(u8) func_name[0]] +
> +		func_name_len) % SQL_FUNC_HASH_SZ;
> +}
> +
> @@ -160,110 +171,98 @@ sqlInsertBuiltinFuncs(FuncDef * aDef,	/* List of global functions to be inserted
> 
> +struct FuncDef *
> +sql_find_function(struct sql *db, const char *func_name, int arg_count,
> +		  bool is_create)
> {
> -	FuncDef *p;		/* Iterator variable */
> -	FuncDef *pBest = 0;	/* Best match found so far */
> -	int bestScore = 0;	/* Score of best match */
> -	int h;			/* Hash value */
> -	int nName;		/* Length of the name */
> -	struct session *user_session = current_session();
> +	assert(arg_count >= -2);
> +	assert(arg_count >= -1 || !is_create);
> +	uint32_t func_name_len = strlen(func_name);
> +	/*
> +	 * The 'score' of the best match is calculated with
> +	 * matchQuality estimator.
> +	 */
> +	int func_score = 0;
> +	/* Best match found so far. */
> +	struct FuncDef *func = NULL;
> 
> -	assert(nArg >= (-2));
> -	assert(nArg >= (-1) || createFlag == 0);
> -	nName = sqlStrlen30(zName);
> +	struct session *user_session = current_session();
> +	bool is_builtin = (user_session->sql_flags & SQL_PreferBuiltin) != 0;
> +	if (is_builtin)
> +		goto lookup_for_builtin;

Could you please explain why do we need separate lookups for
built-in functions and user-defined one? Patch states that now
we share one name-space for both function types. Let’s utilise
the same algorithm for searching functions in hash.
The only problem I see is that there are several built-in functions
with the same name, but different number of arguments. And
depending on number of arguments we one or another function
is used. For example, when max() takes more than one arg, it
becomes non-aggregate function returning maximum from its
arguments. However, ANSI doesn’t declare such behaviour.
As an alternative, several DBs use greatest() and least()
names. So, I suggest to remove ambiguous and make
name hash contains only unique values.   

What is more, lets separate lookup from adding function to hash:
introduce sql_function_by_name() and sql_function_hash_add()

These steps would make work with functions way much more
cleaner and simpler.