From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 308B42F9B4 for ; Fri, 17 May 2019 11:20:23 -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 McXrE6nGcfkz for ; Fri, 17 May 2019 11:20:23 -0400 (EDT) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6F92C2F9B2 for ; Fri, 17 May 2019 11:20:22 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v1 2/3] sql: ban sql functions coinciding with builtins From: "n.pettik" In-Reply-To: Date: Fri, 17 May 2019 18:20:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <9B856D4D-0FA4-40DA-8756-1DBB63CFF0EC@tarantool.org> References: <068d058c4b8354f332e67e95761f722ab72cbae0.1557941573.git.kshcherbatov@tarantool.org> <0356C5B8-C1B1-4986-BBC7-5202E877F656@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > On 17 May 2019, at 11:22, Kirill Shcherbatov = wrote: >=20 > 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, >=20 > 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 !=3D 0) > - return luaL_error(L, sqlErrStr(rc)); > + if (rc !=3D 0) { > + const char *err =3D rc =3D=3D 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; > } >=20 > +/** > + * 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 >=20 > +struct FuncDef * > +sql_find_function(struct sql *db, const char *func_name, int = arg_count, > + bool is_create) > { > - FuncDef *p; /* Iterator variable */ > - FuncDef *pBest =3D 0; /* Best match found so far */ > - int bestScore =3D 0; /* Score of best match */ > - int h; /* Hash value */ > - int nName; /* Length of the name */ > - struct session *user_session =3D current_session(); > + assert(arg_count >=3D -2); > + assert(arg_count >=3D -1 || !is_create); > + uint32_t func_name_len =3D strlen(func_name); > + /* > + * The 'score' of the best match is calculated with > + * matchQuality estimator. > + */ > + int func_score =3D 0; > + /* Best match found so far. */ > + struct FuncDef *func =3D NULL; >=20 > - assert(nArg >=3D (-2)); > - assert(nArg >=3D (-1) || createFlag =3D=3D 0); > - nName =3D sqlStrlen30(zName); > + struct session *user_session =3D current_session(); > + bool is_builtin =3D (user_session->sql_flags & = SQL_PreferBuiltin) !=3D 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=E2=80=99s 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=E2=80=99t 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. =20 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.