[tarantool-patches] Re: [PATCH v3 7/9] sql: get rid of FuncDef function hash
n.pettik
korablev at tarantool.org
Tue Aug 20 22:04:33 MSK 2019
> 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.
More information about the Tarantool-patches
mailing list