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 1096826D07 for ; Tue, 20 Aug 2019 15:12:59 -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 aOHyteo2v2Q7 for ; Tue, 20 Aug 2019 15:12:58 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 7AAD626CC6 for ; Tue, 20 Aug 2019 15:12:58 -0400 (EDT) Date: Tue, 20 Aug 2019 22:12:56 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash Message-ID: <20190820191256.GA25684@atlas> References: <20190816140902.GC18643@atlas> <52e011b5-fe1c-6731-2726-c7d2d3f65e05@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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: "n.pettik" Cc: tarantool-patches@freelists.org, Kirill Shcherbatov * n.pettik [19/08/20 22:05]: Turns out check_count is not such a brilliant idea, eh?-) I don't care much either way, but obviously I don't see a good reason to move something that can be checked at prepare stage to execution stage. With a function in func there is at least a chance to do the check at prepare stage. > > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > > index 5a3e8f1c1..91a640816 100644 > > --- a/src/box/sql/sqlInt.h > > +++ b/src/box/sql/sqlInt.h > > > > @@ -4487,9 +4330,64 @@ Expr *sqlExprForVectorField(Parse *, Expr *, int); > > */ > > extern int sqlSubProgramsRemaining; > > > > -/** Register built-in functions to work with ANALYZE data. */ > > -void > > -sql_register_analyze_builtins(void); > > +struct func_sql_builtin { > > + /** Function object base class. */ > > + struct func base; > > + /** A bitmask of SQL flags. */ > > + uint16_t flags; > > + /** User data to pass in call method. */ > > + void *user_data; > > ‘user_data' is not used anymore, please remove. > > > + /** > > + * A VDBE-memory-compatible call method for a function. > > + * SQL Builting functions doesn't use base class call > > + * method to gain the best possible performance for SQL > > + * requests. As builtin functions are predefined and > > + * non of them modifie schema, access checks are > > + * redundant, functions have the same execution > > + * privileges as SQL. > > + */ > > + void (*call)(sql_context *ctx, int argc, sql_value **argv); > > + /** > > + * Check whether do the function support a specified > > + * number of input arguments. > > -> Check whether function supports given number of arguments. > For instance, trim() function can accept one, two or three arguments. > > > + */ > > + int (*check_param_count)(struct func_sql_builtin *func, int argc); > > > > > > diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h > > index f77c019fb..d0c15d1fe 100644 > > --- a/src/box/sql/vdbeInt.h > > +++ b/src/box/sql/vdbeInt.h > > @@ -46,6 +46,8 @@ > > */ > > typedef struct VdbeOp Op; > > > > +struct func; > > + > > /* > > * Boolean values > > */ > > > > @@ -516,7 +523,17 @@ int sqlVdbeMemNumerify(Mem *); > > int sqlVdbeMemCast(Mem *, enum field_type type); > > int sqlVdbeMemFromBtree(BtCursor *, u32, u32, Mem *); > > void sqlVdbeMemRelease(Mem * p); > > -int sqlVdbeMemFinalize(Mem *, FuncDef *); > > + > > +/** > > + * Memory cell pMem contains the context of an aggregate function. > > Again: pMem -> mem. > > > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > > index 07c019db9..b583b60c9 100644 > > --- a/src/box/sql/func.c > > +++ b/src/box/sql/func.c > > > > @@ -498,6 +501,13 @@ substrFunc(sql_context * context, int argc, sql_value ** argv) > > } > > } > > > > +static int > > +sql_builtin_substr_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc != 2 && argc != 3 ? -1 : 0; > > +} > > + > > /* > > * Implementation of the round() function > > */ > > @@ -538,6 +548,13 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) > > sql_result_double(context, r); > > } > > > > +static int > > +sql_builtin_round_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc != 1 && argc != 2 ? -1 : 0; > > +} > > + > > /* > > * Allocate nByte bytes of space using sqlMalloc(). If the > > * allocation fails, return NULL. If nByte is larger than the > > @@ -968,6 +985,13 @@ likeFunc(sql_context *context, int argc, sql_value **argv) > > sql_result_bool(context, res == MATCH); > > } > > > > +static int > > +sql_builtin_like_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc != 2 && argc != 3 ? -1 : 0; > > +} > > + > > /* > > * Implementation of the NULLIF(x,y) function. The result is the first > > * argument if the arguments are different. The result is NULL if the > > @@ -1514,6 +1538,13 @@ trim_func(struct sql_context *context, int argc, sql_value **argv) > > } > > } > > > > +static int > > +sql_builtin_trim_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc != 1 && argc != 2 && argc != 3 ? -1 : 0; > > +} > > + > > /* > > * Compute the soundex encoding of a word. > > * > > @@ -1703,6 +1734,13 @@ countFinalize(sql_context * context) > > sql_result_uint(context, p ? p->n : 0); > > } > > > > +static int > > +sql_builtin_count_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc != 0 && argc != 1 ? -1 : 0; > > +} > > + > > To be honest, all these stubs and idea with count_check > function at all now seems to be not as good as it used to be. > I’ve removed this check and instead added several runtime > ones and IMHO it looks way much better: > > np/sql-builtins > > diff --git a/src/box/errcode.h b/src/box/errcode.h > index 817275b97..d4aca2caa 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -253,6 +253,7 @@ struct errcode_record { > /*198 */_(ER_FUNC_INDEX_FUNC, "Failed to build a key for functional index '%s' of space '%s': %s") \ > /*199 */_(ER_FUNC_INDEX_FORMAT, "Key format doesn't match one defined in functional index '%s' of space '%s': %s") \ > /*200 */_(ER_FUNC_INDEX_PARTS, "Wrong functional index definition: %s") \ > + /*200 */_(ER_FUNC_WRONG_ARG_COUNT, "Wrong number of arguments is passed to %s: expected %s, got %d") > > /* > * !IMPORTANT! Please follow instructions at start of the file > diff --git a/src/box/sql/func.c b/src/box/sql/func.c > index 74f434346..9f5dffd73 100644 > --- a/src/box/sql/func.c > +++ b/src/box/sql/func.c > @@ -691,7 +691,12 @@ substrFunc(sql_context * context, int argc, sql_value ** argv) > i64 p1, p2; > int negP2 = 0; > > - assert(argc == 3 || argc == 2); > + if (argc != 2 && argc != 3) { > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "SUBSTR", > + "1 or 2", argc); > + context->is_aborted = true; > + return; > + } > if (sql_value_is_null(argv[1]) > || (argc == 3 && sql_value_is_null(argv[2])) > ) { > @@ -779,13 +784,6 @@ substrFunc(sql_context * context, int argc, sql_value ** argv) > } > } > > -static int > -sql_builtin_substr_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc != 2 && argc != 3 ? -1 : 0; > -} > - > /* > * Implementation of the round() function > */ > @@ -794,7 +792,12 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) > { > int n = 0; > double r; > - assert(argc == 1 || argc == 2); > + if (argc != 1 && argc != 2) { > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "ROUND", > + "1 or 2", argc); > + context->is_aborted = true; > + return; > + } > if (argc == 2) { > if (sql_value_is_null(argv[1])) > return; > @@ -826,13 +829,6 @@ roundFunc(sql_context * context, int argc, sql_value ** argv) > sql_result_double(context, r); > } > > -static int > -sql_builtin_round_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc != 1 && argc != 2 ? -1 : 0; > -} > - > /* > * Allocate nByte bytes of space using sqlMalloc(). If the > * allocation fails, return NULL. If nByte is larger than the > @@ -1193,6 +1189,12 @@ likeFunc(sql_context *context, int argc, sql_value **argv) > { > u32 escape = SQL_END_OF_STRING; > int nPat; > + if (argc != 2 && argc != 3) { > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, > + "LIKE", "2 or 3", argc); > + context->is_aborted = true; > + return; > + } > sql *db = sql_context_db_handle(context); > int rhs_type = sql_value_type(argv[0]); > int lhs_type = sql_value_type(argv[1]); > @@ -1263,13 +1265,6 @@ likeFunc(sql_context *context, int argc, sql_value **argv) > sql_result_bool(context, res == MATCH); > } > > -static int > -sql_builtin_like_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc != 2 && argc != 3 ? -1 : 0; > -} > - > /* > * Implementation of the NULLIF(x,y) function. The result is the first > * argument if the arguments are different. The result is NULL if the > @@ -1812,17 +1807,12 @@ trim_func(struct sql_context *context, int argc, sql_value **argv) > trim_func_three_args(context, argv[0], argv[1], argv[2]); > break; > default: > - unreachable(); > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "TRIM", > + "1 or 2 or 3", argc); > + context->is_aborted = true; > } > } > > -static int > -sql_builtin_trim_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc != 1 && argc != 2 && argc != 3 ? -1 : 0; > -} > - > /* > * Compute the soundex encoding of a word. > * > @@ -1998,6 +1988,12 @@ static void > countStep(sql_context * context, int argc, sql_value ** argv) > { > CountCtx *p; > + if (argc != 0 && argc != 1) { > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, > + "COUNT", "0 or 1", argc); > + context->is_aborted = true; > + return; > + } > p = sql_aggregate_context(context, sizeof(*p)); > if ((argc == 0 || ! sql_value_is_null(argv[0])) && p) { > p->n++; > @@ -2012,13 +2008,6 @@ countFinalize(sql_context * context) > sql_result_uint(context, p ? p->n : 0); > } > > -static int > -sql_builtin_count_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc != 0 && argc != 1 ? -1 : 0; > -} > - > /* > * Routines to implement min() and max() aggregate functions. > */ > @@ -2083,7 +2072,12 @@ groupConcatStep(sql_context * context, int argc, sql_value ** argv) > StrAccum *pAccum; > const char *zSep; > int nVal, nSep; > - assert(argc == 1 || argc == 2); > + if (argc != 1 && argc != 2) { > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, > + "GROUP_CONCAT", "1 or 2", argc); > + context->is_aborted = true; > + return; > + } > if (sql_value_is_null(argv[0])) > return; > pAccum = > @@ -2131,14 +2125,6 @@ groupConcatFinalize(sql_context * context) > } > } > > -static int > -sql_builtin_group_concat_check_param_count(struct func_sql_builtin *func, > - int argc) > -{ > - (void) func; > - return argc != 1 && argc != 2 ? -1 : 0; > -} > - > int > sql_is_like_func(struct Expr *expr) > { > @@ -2158,16 +2144,9 @@ sql_func_by_signature(const char *name, int argc) > struct func *base = func_by_name(name, strlen(name)); > if (base == NULL || !base->def->exports.sql) > return NULL; > - if (base->def->language == FUNC_LANGUAGE_SQL_BUILTIN) { > - struct func_sql_builtin *func = > - (struct func_sql_builtin *) base; > - if (func->check_param_count(func, argc) != 0) > - return NULL; > - } else { > - if (base->def->param_count != -1 && > - base->def->param_count != argc) > - return NULL; > - } > + > + if (base->def->param_count != -1 && base->def->param_count != argc) > + return NULL; > return base; > } > > @@ -2190,34 +2169,25 @@ sql_builtin_stub(sql_context *ctx, int argc, sql_value **argv) > ctx->is_aborted = true; > } > > -static int > -sql_builtin_default_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - return (func->base.def->param_count != -1 && > - argc != func->base.def->param_count) ? -1 : 0; > -} > - > -static int > -sql_builtin_coalesce_check_param_count(struct func_sql_builtin *func, int argc) > -{ > - (void) func; > - return argc == 0 || argc == 1 ? -1 : 0; > -} > - > /** > * A sequence of SQL builtins definitions in > * lexicographic order. > */ > static struct { > + /** > + * Name is used to find corresponding entry in array > + * sql_builtins applying binary search. > + */ > const char *name; > - int param_count; > - enum field_type returns; > - enum func_aggregate aggregate; > - bool is_deterministic; > + /** Members below are related to struct func_sql_builtin. */ > uint16_t flags; > void (*call)(sql_context *ctx, int argc, sql_value **argv); > - int (*check_param_count)(struct func_sql_builtin *func, int argc); > void (*finalize)(sql_context *ctx); > + /** Members below are related to struct func_def. */ > + bool is_deterministic; > + int param_count; > + enum field_type returns; > + enum func_aggregate aggregate; > } sql_builtins[] = { > {.name = "ABS", > .param_count = 1, > @@ -2226,7 +2196,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = absFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "AVG", > .param_count = 1, > @@ -2235,7 +2204,6 @@ static struct { > .aggregate = FUNC_AGGREGATE_GROUP, > .flags = 0, > .call = sum_step, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = avgFinalize}, > {.name = "CHAR", > .param_count = -1, > @@ -2244,7 +2212,6 @@ static struct { > .aggregate = FUNC_AGGREGATE_NONE, > .flags = 0, > .call = charFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "CHARACTER_LENGTH", > .param_count = 1, > @@ -2253,7 +2220,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = lengthFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "CHAR_LENGTH", > .param_count = 1, > @@ -2262,7 +2228,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = lengthFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "COALESCE", > .param_count = -1, > @@ -2271,7 +2236,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_COALESCE, > .call = sql_builtin_stub, > - .check_param_count = sql_builtin_coalesce_check_param_count, > .finalize = NULL}, > {.name = "COUNT", > .param_count = -1, > @@ -2280,7 +2244,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = countStep, > - .check_param_count = sql_builtin_count_check_param_count, > .finalize = countFinalize}, > {.name = "GREATEST", > .param_count = -1, > @@ -2289,7 +2252,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_NEEDCOLL | SQL_FUNC_MAX, > .call = minmaxFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "GROUP_CONCAT", > .param_count = -1, > @@ -2298,7 +2260,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = groupConcatStep, > - .check_param_count = sql_builtin_group_concat_check_param_count, > .finalize = groupConcatFinalize}, > {.name = "HEX", > .param_count = 1, > @@ -2307,7 +2268,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = hexFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "IFNULL", > .param_count = 2, > @@ -2316,7 +2276,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_COALESCE, > .call = sql_builtin_stub, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "LEAST", > .param_count = -1, > @@ -2325,7 +2284,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_NEEDCOLL | SQL_FUNC_MIN, > .call = minmaxFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "LENGTH", > .param_count = 1, > @@ -2334,7 +2292,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_LENGTH, > .call = lengthFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "LIKE", > .param_count = -1, > @@ -2343,7 +2300,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_NEEDCOLL | SQL_FUNC_LIKE, > .call = likeFunc, > - .check_param_count = sql_builtin_like_check_param_count, > .finalize = NULL}, > {.name = "LIKELIHOOD", > .param_count = 2, > @@ -2352,7 +2308,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_UNLIKELY, > .call = sql_builtin_stub, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "LIKELY", > .param_count = 1, > @@ -2361,7 +2316,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_UNLIKELY, > .call = sql_builtin_stub, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "LOWER", > .param_count = 1, > @@ -2370,7 +2324,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_DERIVEDCOLL | SQL_FUNC_NEEDCOLL, > .call = LowerICUFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "MAX", > .param_count = 1, > @@ -2379,7 +2332,6 @@ static struct { > .is_deterministic = false, > .flags = SQL_FUNC_NEEDCOLL | SQL_FUNC_MAX, > .call = minmaxStep, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = minMaxFinalize}, > {.name = "MIN", > .param_count = 1, > @@ -2388,7 +2340,6 @@ static struct { > .is_deterministic = false, > .flags = SQL_FUNC_NEEDCOLL | SQL_FUNC_MIN, > .call = minmaxStep, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = minMaxFinalize}, > {.name = "NULLIF", > .param_count = 2, > @@ -2397,7 +2348,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_NEEDCOLL, > .call = nullifFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "POSITION", > .param_count = 2, > @@ -2406,7 +2356,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_NEEDCOLL, > .call = position_func, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "PRINTF", > .param_count = -1, > @@ -2415,7 +2364,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = printfFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "QUOTE", > .param_count = 1, > @@ -2424,7 +2372,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = quoteFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "RANDOM", > .param_count = 0, > @@ -2433,7 +2380,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = randomFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "RANDOMBLOB", > .param_count = 1, > @@ -2442,7 +2388,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = randomBlob, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "REPLACE", > .param_count = 3, > @@ -2451,7 +2396,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_DERIVEDCOLL, > .call = replaceFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "ROUND", > .param_count = -1, > @@ -2460,7 +2404,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = roundFunc, > - .check_param_count = sql_builtin_round_check_param_count, > .finalize = NULL}, > {.name = "ROW_COUNT", > .param_count = 0, > @@ -2469,7 +2412,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = sql_row_count, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "SOUNDEX", > .param_count = 1, > @@ -2478,7 +2420,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = soundexFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "SUBSTR", > .param_count = -1, > @@ -2487,7 +2428,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_DERIVEDCOLL, > .call = substrFunc, > - .check_param_count = sql_builtin_substr_check_param_count, > .finalize = NULL}, > {.name = "SUM", > .param_count = 1, > @@ -2496,7 +2436,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = sum_step, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = sumFinalize}, > {.name = "TOTAL", > .param_count = 1, > @@ -2505,7 +2444,6 @@ static struct { > .is_deterministic = false, > .flags = 0, > .call = sum_step, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = totalFinalize}, > {.name = "TRIM", > .param_count = -1, > @@ -2514,7 +2452,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_DERIVEDCOLL, > .call = trim_func, > - .check_param_count = sql_builtin_trim_check_param_count, > .finalize = NULL}, > {.name = "TYPEOF", > .param_count = 1, > @@ -2523,7 +2460,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_TYPEOF, > .call = typeofFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "UNICODE", > .param_count = 1, > @@ -2532,7 +2468,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = unicodeFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "UNLIKELY", > .param_count = 1, > @@ -2541,7 +2476,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_UNLIKELY, > .call = sql_builtin_stub, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "UPPER", > .param_count = 1, > @@ -2550,7 +2484,6 @@ static struct { > .is_deterministic = true, > .flags = SQL_FUNC_DERIVEDCOLL | SQL_FUNC_NEEDCOLL, > .call = UpperICUFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "VERSION", > .param_count = 0, > @@ -2559,7 +2492,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = sql_func_version, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > {.name = "ZEROBLOB", > .param_count = 1, > @@ -2568,7 +2500,6 @@ static struct { > .is_deterministic = true, > .flags = 0, > .call = zeroblobFunc, > - .check_param_count = sql_builtin_default_check_param_count, > .finalize = NULL}, > }; > > @@ -2607,8 +2538,8 @@ func_sql_builtin_new(struct func_def *def) > goto end; > func->flags = sql_builtins[idx].flags; > func->call = sql_builtins[idx].call; > - func->check_param_count = sql_builtins[idx].check_param_count; > func->finalize = sql_builtins[idx].finalize; > + > def->param_count = sql_builtins[idx].param_count; > def->is_deterministic = sql_builtins[idx].is_deterministic; > def->returns = sql_builtins[idx].returns; > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index 2c037a5a4..cd4a303dd 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -38,8 +38,6 @@ > #include "sqlInt.h" > #include > #include > -#include "box/func.h" > -#include "box/func_def.h" > #include "box/schema.h" > > /* > @@ -678,18 +676,10 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > pParse->is_aborted = true; > pNC->nErr++; > } else if (wrong_num_args) { > - const char *err; > - if (func->def->param_count >= 0) { > - err = "invalid number of arguments is " > - "passed to %.*s(): expected %d, " > - "got %d"; > - } else { > - err = "invalid number of arguments is " > - "passed to %.*s()"; > - } > - diag_set(ClientError, ER_SQL_PARSER_GENERIC, > - tt_sprintf(err, nId, zId, > - func->def->param_count, n)); > + > + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, > + tt_sprintf("%d", func->def->param_count), > + nId); > pParse->is_aborted = true; > pNC->nErr++; > } > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 91a640816..c27eb1022 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -4335,8 +4335,6 @@ struct func_sql_builtin { > struct func base; > /** A bitmask of SQL flags. */ > uint16_t flags; > - /** User data to pass in call method. */ > - void *user_data; > /** > * A VDBE-memory-compatible call method for a function. > * SQL Builting functions doesn't use base class call > @@ -4347,11 +4345,6 @@ struct func_sql_builtin { > * privileges as SQL. > */ > void (*call)(sql_context *ctx, int argc, sql_value **argv); > - /** > - * Check whether do the function support a specified > - * number of input arguments. > - */ > - int (*check_param_count)(struct func_sql_builtin *func, int argc); > /** > * A VDBE-memory-compatible finalize method > * (is valid only for aggregate function). > > > /* > > * Routines to implement min() and max() aggregate functions. > > */ > > @@ -1713,6 +1751,8 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) > > Mem *pBest; > > UNUSED_PARAMETER(NotUsed); > > > > + struct func_sql_builtin *func = > > + (struct func_sql_builtin *)context->func; > > pBest = (Mem *) sql_aggregate_context(context, sizeof(*pBest)); > > if (!pBest) > > return; > > @@ -1721,20 +1761,17 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv) > > if (pBest->flags) > > sqlSkipAccumulatorLoad(context); > > } else if (pBest->flags) { > > - int max; > > int cmp; > > struct coll *pColl = sqlGetFuncCollSeq(context); > > - /* This step function is used for both the min() and max() aggregates, > > - * the only difference between the two being that the sense of the > > - * comparison is inverted. For the max() aggregate, the > > - * sql_user_data() function returns (void *)-1. For min() it > > - * returns (void *)db, where db is the sql* database pointer. > > - * Therefore the next statement sets variable 'max' to 1 for the max() > > - * aggregate, or 0 for min(). > > + /* > > + * This step function is used for both the min() > > + * and max() aggregates, the only difference > > + * between the two being that the sense of the > > + * comparison is inverted. > > */ > > - max = sql_user_data(context) != 0; > > + bool is_max = (func->flags & SQL_FUNC_MAX) != 0; > > I’d rather move introduction of SQL_FUNC_MAX flag to a separate patch. > > > cmp = sqlMemCompare(pBest, pArg, pColl); > > - if ((max && cmp < 0) || (!max && cmp > 0)) { > > + if ((is_max && cmp < 0) || (!is_max && cmp > 0)) { > > sqlVdbeMemCopy(pBest, pArg); > > } else { > > sqlSkipAccumulatorLoad(context); > > @@ -1816,128 +1853,503 @@ groupConcatFinalize(sql_context * context) > > } > > } > > > > +static int > > +sql_builtin_group_concat_check_param_count(struct func_sql_builtin *func, > > + int argc) > > +{ > > + (void) func; > > + return argc != 1 && argc != 2 ? -1 : 0; > > +} > > > > -/* > > - * All of the FuncDef structures in the aBuiltinFunc[] array above > > - * to the global function hash table. This occurs at start-time (as > > - * a consequence of calling sql_initialize()). > > - * > > - * After this routine runs > > +struct func * > > +sql_func_by_signature(const char *name, int argc) > > +{ > > + struct func *base = func_by_name(name, strlen(name)); > > + if (base == NULL || !base->def->exports.sql) > > + return NULL; > > + if (base->def->language == FUNC_LANGUAGE_SQL_BUILTIN) { > > + struct func_sql_builtin *func = > > + (struct func_sql_builtin *) base; > > + if (func->check_param_count(func, argc) != 0) > > + return NULL; > > + } else { > > + if (base->def->param_count != -1 && > > + base->def->param_count != argc) > > + return NULL; > > + } > > + return base; > > +} > > + > > + > > Nit: one extra empty line. > > > +static int > > +sql_builtin_call_stub(struct func *func, struct port *args, struct port *ret) > > I’d rename it to func_sql_builtin_call_stub() > > > +{ > > + (void) func; (void) args; (void) ret; > > + diag_set(ClientError, ER_UNSUPPORTED, > > + "sql builtin function", "Lua frontend"); > > + return -1; > > +} > > + > > +static void > > +sql_builtin_stub(sql_context *ctx, int argc, sql_value **argv) > > +{ > > + (void) argc; (void) argv; > > + diag_set(ClientError, ER_SQL_EXECUTE, > > + tt_sprintf("function '%s' is not implemented", > > + ctx->func->def->name)); > > + ctx->is_aborted = true; > > +} > > + > > +static int > > +sql_builtin_default_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + return (func->base.def->param_count != -1 && > > + argc != func->base.def->param_count) ? -1 : 0; > > +} > > + > > +static int > > +sql_builtin_coalesce_check_param_count(struct func_sql_builtin *func, int argc) > > +{ > > + (void) func; > > + return argc == 0 || argc == 1 ? -1 : 0; > > +} > > + > > +/** > > + * A sequence of SQL builtins definitions in > > + * lexicographic order. > > */ > > -void > > -sqlRegisterBuiltinFunctions(void) > > +static struct { > > + const char *name; > > + int param_count; > > + enum field_type returns; > > + enum func_aggregate aggregate; > > + bool is_deterministic; > > + uint16_t flags; > > + void (*call)(sql_context *ctx, int argc, sql_value **argv); > > + int (*check_param_count)(struct func_sql_builtin *func, int argc); > > + void (*finalize)(sql_context *ctx); > > Let’s reorganise a bit this struct: > > @@ -2209,17 +2209,24 @@ sql_builtin_coalesce_check_param_count(struct func_sql_builtin *func, int argc) > * lexicographic order. > */ > static struct { > + /** > + * Name is used to find corresponding entry in array > + * sql_builtins applying binary search. > + */ > const char *name; > - int param_count; > - enum field_type returns; > - enum func_aggregate aggregate; > - bool is_deterministic; > + /** Members below are related to struct func_sql_builtin. */ > uint16_t flags; > void (*call)(sql_context *ctx, int argc, sql_value **argv); > int (*check_param_count)(struct func_sql_builtin *func, int argc); > void (*finalize)(sql_context *ctx); > + /** Members below are related to struct func_def. */ > + bool is_deterministic; > + int param_count; > + enum field_type returns; > + enum func_aggregate aggregate; > > Now you can use memcpy() to copy corresponding parts > to struct func_def and struct func_sql_builtin (it is up to you). > > > +} sql_builtins[] = { > > + {.name = "ABS", > > + .param_count = 1, > > + .returns = FIELD_TYPE_NUMBER, > > + .aggregate = FUNC_AGGREGATE_NONE, > > + .is_deterministic = true, > > + .flags = 0, > > + .call = absFunc, > > + .check_param_count = sql_builtin_default_check_param_count, > > + .finalize = NULL}, > > + {.name = "AVG”, > > Please, leave one empty line between structs, so that > one can visually separate them: > > - {.name = "ABS", > + { > + .name = "ABS", > .param_count = 1, > .returns = FIELD_TYPE_NUMBER, > .aggregate = FUNC_AGGREGATE_NONE, > @@ -2227,8 +2234,9 @@ static struct { > .flags = 0, > .call = absFunc, > .check_param_count = sql_builtin_default_check_param_count, > - .finalize = NULL}, > - {.name = "AVG", > + .finalize = NULL > + }, { > + .name = "AVG", > .param_count = 1, > .returns = FIELD_TYPE_NUMBER, > .is_deterministic = false, > @@ -2236,8 +2244,9 @@ static struct { > .flags = 0, > .call = sum_step, > .check_param_count = sql_builtin_default_check_param_count, > - .finalize = avgFinalize}, > - {.name = "CHAR", > + .finalize = avgFinalize > + }, { > + .name = "CHAR", > > etc > > > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > > index 207f57ba6..682bb9c8e 100644 > > --- a/src/box/sql/resolve.c > > +++ b/src/box/sql/resolve.c > > @@ -38,6 +38,9 @@ > > #include "sqlInt.h" > > #include > > #include > > +#include "box/func.h" > > +#include "box/func_def.h” > > func.h and func_def.h seem to be redundant - > I’ve removed them and project has compiled. > > > +#include "box/schema.h" > > > > /* > > * Walk the expression tree pExpr and increase the aggregate function > > @@ -596,27 +599,28 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > > int is_agg = 0; /* True if is an aggregate function */ > > int nId; /* Number of characters in function name */ > > const char *zId; /* The function name. */ > > - FuncDef *pDef; /* Information about the function */ > > > > assert(!ExprHasProperty(pExpr, EP_xIsSelect)); > > zId = pExpr->u.zToken; > > nId = sqlStrlen30(zId); > > - pDef = sqlFindFunction(pParse->db, zId, n, 0); > > - if (pDef == 0) { > > - pDef = > > - sqlFindFunction(pParse->db, zId, -2,0); > > - if (pDef == 0) { > > + struct func *func = sql_func_by_signature(zId, n); > > + if (func == NULL) { > > + func = func_by_name(zId, nId); > > This makes you include box/schema.h > > > + if (func == NULL || !func->def->exports.sql) > > no_such_func = 1; > > "No such function” and “not available in SQL” are different errors. > Please, handle both cases with proper error messages and add > test cases. > > > - } else { > > + else > > wrong_num_args = 1; > > - } > > } else { > > - is_agg = pDef->xFinalize != 0; > > - pExpr->type = pDef->ret_type; > > + is_agg = func->def->language == > > + FUNC_LANGUAGE_SQL_BUILTIN && > > + func->def->aggregate == > > + FUNC_AGGREGATE_GROUP; > > == FUNC_AGGREGATE — check on language is redundant. > Instead, add an assertion. > > > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > > index ddb5de87e..b2ce868d6 100644 > > --- a/src/box/sql/select.c > > +++ b/src/box/sql/select.c > > @@ -4381,7 +4381,9 @@ is_simple_count(struct Select *select, struct AggInfo *agg_info) > > return NULL; > > if (NEVER(agg_info->nFunc == 0)) > > return NULL; > > - if ((agg_info->aFunc->pFunc->funcFlags & SQL_FUNC_COUNT) == 0 || > > + assert(agg_info->aFunc->func->def->language == > > + FUNC_LANGUAGE_SQL_BUILTIN); > > + if (!sql_func_flag_is_set(agg_info->aFunc->func, SQL_FUNC_COUNT) == 0 || > > Nit: sql_func_flag_is_set() returns boolean, no need to check > it on equality to zero. > > > diff --git a/src/box/alter.cc b/src/box/alter.cc > > index 4f2e34bf0..22d3040ef 100644 > > --- a/src/box/alter.cc > > +++ b/src/box/alter.cc > > @@ -2926,6 +2926,12 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) > > (unsigned) old_func->def->uid, > > "function has references"); > > } > > + /* Can't' drop a builtin function. */ > > + if (old_func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) { > > + tnt_raise(ClientError, ER_DROP_FUNCTION, > > + (unsigned) old_func->def->uid, > > + "function is SQL builtin”); > > Nit: …SQL built-in. > > -- Konstantin Osipov, Moscow, Russia