Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>
Cc: tarantool-patches@freelists.org,
	Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash
Date: Tue, 20 Aug 2019 22:12:56 +0300	[thread overview]
Message-ID: <20190820191256.GA25684@atlas> (raw)
In-Reply-To: <F9ADFEF1-75A7-403E-8A77-735B1EF123D4@tarantool.org>

* n.pettik <korablev@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

  reply	other threads:[~2019-08-20 19:12 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-16 13:26 [tarantool-patches] [PATCH v3 0/9] sql: uniform SQL and Lua functions subsystem Kirill Shcherbatov
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 1/9] sql: remove SQL_PreferBuiltin flag Kirill Shcherbatov
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 2/9] sql: GREATEST, LEAST instead of MIN/MAX overload Kirill Shcherbatov
2019-08-16 18:57   ` [tarantool-patches] " n.pettik
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 3/9] sql: wrap all trim functions in dispatcher Kirill Shcherbatov
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 4/9] sql: rework SQL_FUNC_COUNT flag semantics Kirill Shcherbatov
2019-08-16 18:55   ` [tarantool-patches] " n.pettik
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 5/9] sql: rename OP_Function to OP_BuiltinFunction Kirill Shcherbatov
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 6/9] sql: remove SQL_FUNC_SLOCHNG flag Kirill Shcherbatov
2019-08-16 18:54   ` [tarantool-patches] " n.pettik
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 7/9] sql: get rid of FuncDef function hash Kirill Shcherbatov
2019-08-16 14:09   ` [tarantool-patches] " Konstantin Osipov
2019-08-16 18:59     ` n.pettik
2019-08-19 15:51     ` Kirill Shcherbatov
2019-08-20 13:47       ` Konstantin Osipov
2019-08-20 19:04       ` n.pettik
2019-08-20 19:12         ` Konstantin Osipov [this message]
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 8/9] sql: get rid of box.internal.sql_function_create Kirill Shcherbatov
2019-08-16 13:26 ` [tarantool-patches] [PATCH v3 9/9] sql: better error messages on invalid arguments Kirill Shcherbatov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190820191256.GA25684@atlas \
    --to=kostja@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox