[tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash
Konstantin Osipov
kostja at tarantool.org
Tue Aug 20 22:12:56 MSK 2019
* n.pettik <korablev at tarantool.org> [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 <stdlib.h>
> #include <string.h>
> -#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 <stdlib.h>
> > #include <string.h>
> > +#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
More information about the Tarantool-patches
mailing list