[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server
n.pettik
korablev at tarantool.org
Tue May 29 14:49:09 MSK 2018
>sql: remove Checks to server
I guess, it would be better to use word ‘move’...
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 9e0e7e3..7bff86f 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -33,17 +33,33 @@
> #include "diag.h"
> #include "error.h"
> #include "sql.h"
> +#include "msgpuck.h"
> +
> +/**
> + * Make checks from msgpack.
> + * @param str pointer to array of maps
> + * e.g. [{"expr_str": "x < y", "name": "ONE"}, ..].
> + * @param len array items count.
> + * @param opt pointer to store parsing result.
You can specify that ‘opt’ is output param by [out] tag:
@param[out] opt ...
>+++ b/src/box/space_def.h
>@@ -40,6 +40,8 @@
> extern "C" {
> #endif /* defined(__cplusplus) */
>
>+struct ExprList;
>+
Mb it would be better to rename this struct according to our codestyle?
>@@ -79,12 +81,8 @@ space_opts_create(struct space_opts *opts)
> /**
> * Destroy space options
> */
>-static inline void
>-space_opts_destroy(struct space_opts *opts)
>-{
>- free(opts->sql);
>- TRASH(opts);
>-}
>+void
>+space_opts_destroy(struct space_opts *opts);
Seems like this function lacks some meaningful comment.
>+++ b/src/box/sql.c
>@@ -1500,22 +1500,47 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
> *
> * Ex: {"temporary": true, "sql": "CREATE TABLE student (name, grade)"}
> */
>-int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
>+int
>+tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf)
> {
> const struct Enc *enc = get_enc(buf);
>- char *base = buf, *p;
>+ bool is_view = pTable != NULL && pTable->def->opts.is_view;
>+ bool has_checks = pTable != NULL && pTable->def->opts.checks != NULL;
In fact, it is incorrect since opts.checks may also contain aliases for VIEW
columns: CREATE VIEW V1(a, b, c) AS SELECT * FROM t1;
In this case, checks contain NULL exprs with names ‘A’, ‘B’ and ‘C’.
So you should add another one condition: && is_view.
>+ if (a[i].zName != NULL) {
>+ p = enc->encode_str(p, "name", 4);
>+ p = enc->encode_str(p, a[i].zName, strlen(a[i].zName));
>+ }
The same is here: you add checks for VIEW which are really aliases.
>+int
>+sql_checks_resolve_space_def_reference(ExprList *expr_list,
>+ struct space_def *def)
>+{
>+ Parse parser;
>+ sql_parser_create(&parser, sql_get());
>+ parser.parse_only = true;
>+
>+ Table dummy_table;
>+ memset(&dummy_table, 0, sizeof(dummy_table));
>+ dummy_table.def = def;
>+
>+ sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+ expr_list);
>+
>+ sql_parser_destroy(&parser);
>+ if (parser.rc != SQLITE_OK) {
>+ /* Error is already set with diag. */
>+ if (parser.rc != SQL_TARANTOOL_ERROR)
>+ diag_set(ClientError, ER_SQL, parser.zErrMsg);
Comment isn’t consistent with code: if error is already
set, why you set it again?
>@@ -232,6 +240,42 @@ void
> sql_resolve_self_reference(struct Parse *parser, struct Table *table, int type,
> struct Expr *expr, struct ExprList *expr_list);
>
>+/**
>+ * Initialize check_list_item.
>+ * @param expr_list ExprList with item.
>+ * @param idx item index.
I would somehow rename this var, since it is easy to
confuse one with things related to database indexes.
>+int
>+sql_checks_resolve_space_def_reference(ExprList *expr_list,
>+ struct space_def *def)
>+{
>+ Parse parser;
>+ sql_parser_create(&parser, sql_get());
>+ parser.parse_only = true;
>+
>+ Table dummy_table;
>+ memset(&dummy_table, 0, sizeof(dummy_table));
>+ dummy_table.def = def;
>+
>+ sql_resolve_self_reference(&parser, &dummy_table, NC_IsCheck, NULL,
>+ expr_list);
Why do we still need to create table wrappers?
Is it possible to change signatures and use straight space_def instead of fake tables?
More information about the Tarantool-patches
mailing list