[tarantool-patches] [PATCH v2 0/2] Introduce 'view' space option

[tarantool-patches] [PATCH v2 0/2] Introduce 'view' space option

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/gh-3268-introduce-view
Issue: https://github.com/tarantool/tarantool/issues/3268                          
                                                                                   
Changelog:                                                                         
 - Added tests to check that view can't be created via Lua 'create_space'
   function, and to check that view can be created by direct insertion
   to _space.                                                                      
 - Implemented SQL follow-up patch to remove TF_View flag and to fetch
   'view' property from SQL.     

Nikita Pettik (2):
  Introduce 'view' space option
  sql: use 'view' opts from space

 src/box/alter.cc             |  7 ++++
 src/box/errcode.h            |  3 +-
 src/box/space_def.c          |  7 ++++
 src/box/space_def.h          |  6 +++
 src/box/sql.c                |  9 +++-
 src/box/sql/alter.c          |  4 +-
 src/box/sql/build.c          | 27 ++++++++----
 src/box/sql/delete.c         |  2 +-
 src/box/sql/fkey.c           |  3 +-
 src/box/sql/insert.c         | 10 +++--
 src/box/sql/parse.c          |  4 +-
 src/box/sql/parse.y          |  5 +--
 src/box/sql/select.c         |  4 +-
 src/box/sql/sqliteInt.h      |  5 ++-
 src/box/sql/trigger.c        |  4 +-
 src/box/sql/update.c         |  2 +-
 src/box/sql/vdbeaux.c        |  2 +-
 src/box/sql/where.c          |  3 +-
 test/box/misc.result         |  1 +
 test/sql/transition.result   |  7 ----
 test/sql/transition.test.lua |  4 --
 test/sql/view.result         | 98 ++++++++++++++++++++++++++++++++++++++++++++
 test/sql/view.test.lua       | 46 +++++++++++++++++++++
 23 files changed, 220 insertions(+), 43 deletions(-)
 create mode 100644 test/sql/view.result
 create mode 100644 test/sql/view.test.lua

-- 
2.15.1

[tarantool-patches] [PATCH v2 1/2] Introduce 'view' space option

[Nikita Pettik]

Now, space can feature SQL specific option 'view'. In SQL view is a
space without any functional parts except for 'SELECT' statement.
In this respect, creation of any indexes is prohibited on views;
views must have SQL statement; transofrmation from space to view is also
banned.
It is worth mentioning, that setting 'view' option isn't available via
Lua interface function 'space_create', but possible by straight
insertion in _space space.

Closes #3268
---
 src/box/alter.cc       |  7 ++++
 src/box/errcode.h      |  3 +-
 src/box/space_def.c    |  7 ++++
 src/box/space_def.h    |  6 ++++
 src/box/sql.c          |  9 ++++-
 test/box/misc.result   |  1 +
 test/sql/view.result   | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/view.test.lua | 46 ++++++++++++++++++++++++
 8 files changed, 175 insertions(+), 2 deletions(-)
 create mode 100644 test/sql/view.result
 create mode 100644 test/sql/view.test.lua

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bd87d2f54..a93ea5918 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -496,6 +496,9 @@ space_def_new_from_tuple(struct tuple *tuple, uint32_t errcode,
 	}
 	struct space_opts opts;
 	space_opts_decode(&opts, space_opts, region);
+	if (opts.view && opts.sql == NULL) {
+		tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
+	}
 	struct space_def *def =
 		space_def_new_xc(id, uid, exact_field_count, name, name_len,
 				 engine_name, engine_name_len, &opts, fields,
@@ -1590,6 +1593,10 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple,
 					  BOX_INDEX_FIELD_ID);
 	struct space *old_space = space_cache_find_xc(id);
+	if (old_space->def->opts.view) {
+		tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
+			  "can not alter indexes on view");
+	}
 	enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D;
 	if (old_tuple && new_tuple)
 		priv_type = PRIV_A;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index beb37aa2f..e5d882df8 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -212,7 +212,8 @@ struct errcode_record {
 	/*157 */_(ER_SQL_EXECUTE,               "Failed to execute SQL statement: %s") \
 	/*158 */_(ER_SQL,			"SQL error: %s") \
 	/*159 */_(ER_SQL_BIND_NOT_FOUND,	"Parameter %s was not found in the statement") \
-	/*160 */_(ER_ACTION_MISMATCH,		"Field %d contains %s on conflict action, but %s in index parts")
+	/*160 */_(ER_ACTION_MISMATCH,		"Field %d contains %s on conflict action, but %s in index parts") \
+	/*161 */_(ER_VIEW_MISSING_SQL,		"Space declared as a view must have SQL statement") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 340cf27f2..304f904a6 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -35,11 +35,13 @@
 
 const struct space_opts space_opts_default = {
 	/* .temporary = */ false,
+	/* .view = */ false,
 	/* .sql        = */ NULL,
 };
 
 const struct opt_def space_opts_reg[] = {
 	OPT_DEF("temporary", OPT_BOOL, struct space_opts, temporary),
+	OPT_DEF("view", OPT_BOOL, struct space_opts, view),
 	OPT_DEF("sql", OPT_STRPTR, struct space_opts, sql),
 	OPT_END,
 };
@@ -177,6 +179,11 @@ space_def_check_compatibility(const struct space_def *old_def,
 			 "space id is immutable");
 		return -1;
 	}
+	if (new_def->opts.view != old_def->opts.view) {
+		diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
+			 "can not transform space to view and visa versa");
+		return -1;
+	}
 	if (is_space_empty)
 		return 0;
 
diff --git a/src/box/space_def.h b/src/box/space_def.h
index 8425ff6d3..bdf697437 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -49,6 +49,12 @@ struct space_opts {
 	 * - changes are not part of a snapshot
 	 */
 	bool temporary;
+	/**
+	 * If the space is a view, then it can't feature any
+	 * indexes, and must have SQL statement. Moreover,
+	 * this flag can't be changed after space creation.
+	 */
+	bool view;
 	/**
 	 * SQL statement that produced this space.
 	 */
diff --git a/src/box/sql.c b/src/box/sql.c
index 224747157..1e5f551ae 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1592,9 +1592,16 @@ int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
 	const struct Enc *enc = get_enc(buf);
 	char *base = buf, *p;
 
-	p = enc->encode_map(base, 1);
+	bool is_view = false;
+	if (pTable != NULL)
+		is_view = pTable->pSelect != NULL;
+	p = enc->encode_map(base, is_view ? 2 : 1);
 	p = enc->encode_str(p, "sql", 3);
 	p = enc->encode_str(p, zSql, strlen(zSql));
+	if (is_view) {
+		p = enc->encode_str(p, "view", 4);
+		p = enc->encode_bool(p, true);
+	}
 	return (int)(p - base);
 }
 
diff --git a/test/box/misc.result b/test/box/misc.result
index aeee42930..298e6a9ba 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -329,6 +329,7 @@ t;
   - 'box.error.ROLE_LOOP : 87'
   - 'box.error.TUPLE_NOT_FOUND : 4'
   - 'box.error.LOADING : 116'
+  - 'box.error.VIEW_MISSING_SQL : 161'
   - 'box.error.BACKUP_IN_PROGRESS : 129'
   - 'box.error.DROP_USER : 44'
   - 'box.error.MODIFY_INDEX : 14'
diff --git a/test/sql/view.result b/test/sql/view.result
new file mode 100644
index 000000000..aa65dfb28
--- /dev/null
+++ b/test/sql/view.result
@@ -0,0 +1,98 @@
+test_run = require('test_run').new()
+---
+...
+-- Verify that constraints on 'view' option are working.
+-- box.cfg()
+-- Create space and view.
+box.sql.execute("CREATE TABLE t1(a, b, PRIMARY KEY(a, b));");
+---
+...
+box.sql.execute("CREATE VIEW v1 AS SELECT a+b FROM t1;");
+---
+...
+-- View can't have any indexes.
+box.sql.execute("CREATE INDEX i1 on v1(a);");
+---
+- error: views may not be indexed
+...
+v1 = box.space.V1;
+---
+...
+v1:create_index('primary', {parts = {1, 'string'}})
+---
+- error: 'Can''t modify space ''V1'': can not alter indexes on view'
+...
+v1:create_index('secondary', {parts = {1, 'string'}})
+---
+- error: 'Can''t modify space ''V1'': can not alter indexes on view'
+...
+-- View option can't be changed.
+v1 = box.space._space.index[2]:select('V1')[1]:totable();
+---
+...
+v1[6]['view'] = false;
+---
+...
+box.space._space:replace(v1);
+---
+- error: 'Can''t modify space ''V1'': can not transform space to view and visa versa'
+...
+t1 = box.space._space.index[2]:select('T1')[1]:totable();
+---
+...
+t1[6]['view'] = true;
+---
+...
+box.space._space:replace(t1);
+---
+- error: 'Can''t modify space ''T1'': can not transform space to view and visa versa'
+...
+-- View can't exist without SQL statement.
+v1[6] = {};
+---
+...
+v1[6]['view'] = true;
+---
+...
+box.space._space:replace(v1);
+---
+- error: Space declared as a view must have SQL statement
+...
+-- Views can't be created via space_create().
+box.schema.create_space('view', {view = true})
+---
+- error: Illegal parameters, unexpected option 'view'
+...
+-- View can be created via straight insertion into _space.
+sp = box.schema.create_space('test');
+---
+...
+raw_sp = box.space._space:get(sp.id):totable();
+---
+...
+sp:drop();
+---
+...
+raw_sp[6].sql = 'fake';
+---
+...
+raw_sp[6].view = true;
+---
+...
+sp = box.space._space:replace(raw_sp);
+---
+...
+box.space._space:select(sp['id'])[1]['name']
+---
+- test
+...
+-- Cleanup
+box.space.test:drop();
+---
+...
+box.sql.execute("DROP TABLE t1;");
+---
+...
+box.sql.execute("DROP VIEW v1;");
+---
+...
diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua
new file mode 100644
index 000000000..ab2843cb6
--- /dev/null
+++ b/test/sql/view.test.lua
@@ -0,0 +1,46 @@
+test_run = require('test_run').new()
+
+-- Verify that constraints on 'view' option are working.
+
+-- box.cfg()
+
+-- Create space and view.
+box.sql.execute("CREATE TABLE t1(a, b, PRIMARY KEY(a, b));");
+box.sql.execute("CREATE VIEW v1 AS SELECT a+b FROM t1;");
+
+-- View can't have any indexes.
+box.sql.execute("CREATE INDEX i1 on v1(a);");
+v1 = box.space.V1;
+v1:create_index('primary', {parts = {1, 'string'}})
+v1:create_index('secondary', {parts = {1, 'string'}})
+
+-- View option can't be changed.
+v1 = box.space._space.index[2]:select('V1')[1]:totable();
+v1[6]['view'] = false;
+box.space._space:replace(v1);
+
+t1 = box.space._space.index[2]:select('T1')[1]:totable();
+t1[6]['view'] = true;
+box.space._space:replace(t1);
+
+-- View can't exist without SQL statement.
+v1[6] = {};
+v1[6]['view'] = true;
+box.space._space:replace(v1);
+
+-- Views can't be created via space_create().
+box.schema.create_space('view', {view = true})
+
+-- View can be created via straight insertion into _space.
+sp = box.schema.create_space('test');
+raw_sp = box.space._space:get(sp.id):totable();
+sp:drop();
+raw_sp[6].sql = 'fake';
+raw_sp[6].view = true;
+sp = box.space._space:replace(raw_sp);
+box.space._space:select(sp['id'])[1]['name']
+
+-- Cleanup
+box.space.test:drop();
+box.sql.execute("DROP TABLE t1;");
+box.sql.execute("DROP VIEW v1;");
-- 
2.15.1

[tarantool-patches] [PATCH v2 2/2] sql: use 'view' opts from space

[Nikita Pettik]

Originally in SQLite, table which represented view had not null pointer
to struct Select. However, after 'view' space option is introduced, it
has become possible to fetch 'view' property directly from struct space.
This patch introduces simple wrapper to get this option.
Moreover, TF_View flag is no more needed, so it is also removed.
---
 src/box/sql/alter.c          |  4 ++--
 src/box/sql/build.c          | 27 +++++++++++++++++++--------
 src/box/sql/delete.c         |  2 +-
 src/box/sql/fkey.c           |  3 ++-
 src/box/sql/insert.c         | 10 ++++++----
 src/box/sql/parse.c          |  4 ++--
 src/box/sql/parse.y          |  5 ++---
 src/box/sql/select.c         |  4 ++--
 src/box/sql/sqliteInt.h      |  5 +++--
 src/box/sql/trigger.c        |  4 ++--
 src/box/sql/update.c         |  2 +-
 src/box/sql/vdbeaux.c        |  2 +-
 src/box/sql/where.c          |  3 ++-
 test/sql/transition.result   |  7 -------
 test/sql/transition.test.lua |  4 ----
 15 files changed, 45 insertions(+), 41 deletions(-)

diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index a52ba8d7a..054c0856c 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -107,7 +107,7 @@ sqlite3AlterRenameTable(Parse * pParse,	/* Parser context. */
 	}
 
 #ifndef SQLITE_OMIT_VIEW
-	if (pTab->pSelect) {
+	if (space_is_view(pTab)) {
 		sqlite3ErrorMsg(pParse, "view %s may not be altered",
 				pTab->zName);
 		goto exit_rename_table;
@@ -262,7 +262,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 		goto exit_begin_add_column;
 
 	/* Make sure this is not an attempt to ALTER a view. */
-	if (pTab->pSelect) {
+	if (space_is_view(pTab)) {
 		sqlite3ErrorMsg(pParse, "Cannot add a column to a view");
 		goto exit_begin_add_column;
 	}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index dd0f45cf6..a9cb76717 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1160,6 +1160,20 @@ index_collation_name(Index *idx, uint32_t column)
 	return index->def->key_def->parts[column].coll->name;
 }
 
+/**
+ * Return true if space which corresponds to
+ * given table has view option.
+ */
+bool
+space_is_view(Table *table)
+{
+	assert(table != NULL);
+	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
+	struct space *space = space_by_id(space_id);
+	assert(space != NULL);
+	return space->def->opts.view;
+}
+
 /*
  * This function returns the collation sequence for database native text
  * encoding identified by the string zName, length nName.
@@ -1839,7 +1853,6 @@ void
 sqlite3EndTable(Parse * pParse,	/* Parse context */
 		Token * pCons,	/* The ',' token after the last column defn. */
 		Token * pEnd,	/* The ')' before options in the CREATE TABLE */
-		u8 tabOpts,	/* Extra table options. Usually 0. */
 		Select * pSelect	/* Select from a "CREATE ... AS SELECT" */
     )
 {
@@ -1864,9 +1877,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 	if (db->init.busy)
 		p->tnum = db->init.newTnum;
 
-	if (p->pSelect) {
-		tabOpts |= TF_View;
-	} else {
+	if (!p->pSelect) {
 		if ((p->tabFlags & TF_HasPrimaryKey) == 0) {
 			sqlite3ErrorMsg(pParse,
 					"PRIMARY KEY missing on table %s",
@@ -1957,7 +1968,7 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		if (pSelect) {
 			zStmt = createTableStmt(db, p);
 		} else {
-			Token *pEnd2 = tabOpts ? &pParse->sLastToken : pEnd;
+			Token *pEnd2 = p->pSelect ? &pParse->sLastToken : pEnd;
 			n = (int)(pEnd2->z - pParse->sNameToken.z);
 			if (pEnd2->z[0] != ';')
 				n += pEnd2->n;
@@ -2123,7 +2134,7 @@ sqlite3CreateView(Parse * pParse,	/* The parsing context */
 	sEnd.n = 1;
 
 	/* Use sqlite3EndTable() to add the view to the Tarantool.  */
-	sqlite3EndTable(pParse, 0, &sEnd, 0, 0);
+	sqlite3EndTable(pParse, 0, &sEnd, 0);
 
  create_view_fail:
 	sqlite3SelectDelete(db, pSelect);
@@ -2414,12 +2425,12 @@ sqlite3DropTable(Parse * pParse, SrcList * pName, int isView, int noErr)
 	/* Ensure DROP TABLE is not used on a view, and DROP VIEW is not used
 	 * on a table.
 	 */
-	if (isView && pTab->pSelect == 0) {
+	if (isView && !space_is_view(pTab)) {
 		sqlite3ErrorMsg(pParse, "use DROP TABLE to delete table %s",
 				pTab->zName);
 		goto exit_drop_table;
 	}
-	if (!isView && pTab->pSelect) {
+	if (!isView && space_is_view(pTab)) {
 		sqlite3ErrorMsg(pParse, "use DROP VIEW to delete view %s",
 				pTab->zName);
 		goto exit_drop_table;
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 61845aa27..2e09d926a 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -88,7 +88,7 @@ sqlite3IsReadOnly(Parse * pParse, Table * pTab, int viewOk)
 		return 1;
 	}
 #ifndef SQLITE_OMIT_VIEW
-	if (!viewOk && pTab->pSelect) {
+	if (!viewOk && space_is_view(pTab)) {
 		sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view",
 				pTab->zName);
 		return 1;
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 793f2f640..9286f4c5a 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -769,7 +769,8 @@ sqlite3FkDropTable(Parse * pParse, SrcList * pName, Table * pTab)
 	sqlite3 *db = pParse->db;
 	struct session *user_session = current_session();
 
-	if ((user_session->sql_flags & SQLITE_ForeignKeys) && !pTab->pSelect) {
+	if ((user_session->sql_flags & SQLITE_ForeignKeys) &&
+	    !space_is_view(pTab)) {
 		int iSkip = 0;
 		Vdbe *v = sqlite3GetVdbe(pParse);
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 28c01092d..42254ddf4 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -381,7 +381,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
 	 */
 #ifndef SQLITE_OMIT_TRIGGER
 	pTrigger = sqlite3TriggersExist(pTab, TK_INSERT, 0, &tmask);
-	isView = pTab->pSelect != 0;
+	isView = space_is_view(pTab);
 #else
 #define pTrigger 0
 #define tmask 0
@@ -1074,7 +1074,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 	db = pParse->db;
 	v = sqlite3GetVdbe(pParse);
 	assert(v != 0);
-	assert(pTab->pSelect == 0);	/* This table is not a VIEW */
+	/* This table is not a VIEW */
+	assert(!space_is_view(pTab));
 	nCol = pTab->nCol;
 
 	pPk = sqlite3PrimaryKeyIndex(pTab);
@@ -1482,7 +1483,8 @@ sqlite3CompleteInsertion(Parse * pParse,	/* The parser context */
 
 	v = sqlite3GetVdbe(pParse);
 	assert(v != 0);
-	assert(pTab->pSelect == 0);	/* This table is not a VIEW */
+	/* This table is not a VIEW */
+	assert(!space_is_view(pTab));
 	/*
 	 * The for loop which purpose in sqlite was to insert new
 	 * values to all indexes is replaced to inserting new
@@ -1802,7 +1804,7 @@ xferOptimization(Parse * pParse,	/* Parser context */
 	if (pSrc == pDest) {
 		return 0;	/* tab1 and tab2 may not be the same table */
 	}
-	if (pSrc->pSelect) {
+	if (space_is_view(pSrc)) {
 		return 0;	/* tab2 may not be a view */
 	}
 	if (pDest->nCol != pSrc->nCol) {
diff --git a/src/box/sql/parse.c b/src/box/sql/parse.c
index f938a9a3c..17ce309da 100644
--- a/src/box/sql/parse.c
+++ b/src/box/sql/parse.c
@@ -2262,14 +2262,14 @@ static void yy_reduce(
       case 17: /* create_table_args ::= LP columnlist conslist_opt RP */
 #line 185 "parse.y"
 {
-  sqlite3EndTable(pParse,&yymsp[-1].minor.yy0,&yymsp[0].minor.yy0,0,0);
+  sqlite3EndTable(pParse,&yymsp[-1].minor.yy0,&yymsp[0].minor.yy0,0);
 }
 #line 2268 "parse.c"
         break;
       case 18: /* create_table_args ::= AS select */
 #line 188 "parse.y"
 {
-  sqlite3EndTable(pParse,0,0,0,yymsp[0].minor.yy279);
+  sqlite3EndTable(pParse,0,0,yymsp[0].minor.yy279);
   sqlite3SelectDelete(pParse->db, yymsp[0].minor.yy279);
 }
 #line 2276 "parse.c"
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 11db58d94..e2acf2423 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -183,10 +183,10 @@ ifnotexists(A) ::= .              {A = 0;}
 ifnotexists(A) ::= IF NOT EXISTS. {A = 1;}
 
 create_table_args ::= LP columnlist conslist_opt(X) RP(E). {
-  sqlite3EndTable(pParse,&X,&E,0,0);
+  sqlite3EndTable(pParse,&X,&E,0);
 }
 create_table_args ::= AS select(S). {
-  sqlite3EndTable(pParse,0,0,0,S);
+  sqlite3EndTable(pParse,0,0,S);
   sqlite3SelectDelete(pParse->db, S);
 }
 columnlist ::= columnlist COMMA columnname carglist.
@@ -1357,7 +1357,6 @@ cmd ::= createkw trigger_decl(A) BEGIN trigger_cmd_list(S) END(Z). {
 trigger_decl(A) ::= TRIGGER ifnotexists(NOERR) nm(B)
                     trigger_time(C) trigger_event(D)
                     ON fullname(E) foreach_clause when_clause(G). {
-  pParse->initiateTTrans = false;
   sqlite3BeginTrigger(pParse, &B, C, D.a, D.b, E, G, NOERR);
   A = B; /*A-overwrites-T*/
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index a76286f23..c14bd7462 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -4295,7 +4295,7 @@ isSimpleCount(Select * p, AggInfo * pAggInfo)
 	}
 	pTab = p->pSrc->a[0].pTab;
 	pExpr = p->pEList->a[0].pExpr;
-	assert(pTab && !pTab->pSelect && pExpr);
+	assert(pTab && !space_is_view(pTab) && pExpr);
 	if (pExpr->op != TK_AGG_FUNCTION)
 		return 0;
 	if (NEVER(pAggInfo->nFunc == 0))
@@ -4760,7 +4760,7 @@ selectExpander(Walker * pWalker, Select * p)
 				return WRC_Abort;
 			}
 #if !defined(SQLITE_OMIT_VIEW)
-			if (pTab->pSelect) {
+			if (space_is_view(pTab)) {
 				i16 nCol;
 				if (sqlite3ViewGetColumnNames(pParse, pTab))
 					return WRC_Abort;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 0856bc729..effc0ffd5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1355,7 +1355,6 @@ struct Table {
 #define TF_Ephemeral       0x02	/* An ephemeral table */
 #define TF_HasPrimaryKey   0x04	/* Table has a primary key */
 #define TF_Autoincrement   0x08	/* Integer primary key is autoincrement */
-#define TF_View   	   0x20	/* A view */
 
 /*
  * Each foreign key constraint is an instance of the following structure.
@@ -2983,7 +2982,9 @@ const char *
 index_collation_name(Index *, uint32_t);
 struct coll *
 sql_default_coll();
-void sqlite3EndTable(Parse *, Token *, Token *, u8, Select *);
+bool
+space_is_view(Table *);
+void sqlite3EndTable(Parse *, Token *, Token *, Select *);
 int sqlite3ParseUri(const char *, const char *, unsigned int *,
 		    sqlite3_vfs **, char **, char **);
 
diff --git a/src/box/sql/trigger.c b/src/box/sql/trigger.c
index 08963cb79..5460d1a54 100644
--- a/src/box/sql/trigger.c
+++ b/src/box/sql/trigger.c
@@ -144,13 +144,13 @@ sqlite3BeginTrigger(Parse * pParse,	/* The parse context of the CREATE TRIGGER s
 	/* INSTEAD of triggers are only for views and views only support INSTEAD
 	 * of triggers.
 	 */
-	if (pTab->pSelect && tr_tm != TK_INSTEAD) {
+	if (space_is_view(pTab) && tr_tm != TK_INSTEAD) {
 		sqlite3ErrorMsg(pParse, "cannot create %s trigger on view: %S",
 				(tr_tm == TK_BEFORE) ? "BEFORE" : "AFTER",
 				pTableName, 0);
 		goto trigger_cleanup;
 	}
-	if (!pTab->pSelect && tr_tm == TK_INSTEAD) {
+	if (!space_is_view(pTab) && tr_tm == TK_INSTEAD) {
 		sqlite3ErrorMsg(pParse, "cannot create INSTEAD OF"
 				" trigger on table: %S", pTableName, 0);
 		goto trigger_cleanup;
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 47500ec9e..bf413252d 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -163,7 +163,7 @@ sqlite3Update(Parse * pParse,		/* The parser context */
 	 */
 #ifndef SQLITE_OMIT_TRIGGER
 	pTrigger = sqlite3TriggersExist(pTab, TK_UPDATE, pChanges, &tmask);
-	isView = pTab->pSelect != 0;
+	isView = space_is_view(pTab);
 	assert(pTrigger || tmask == 0);
 #else
 #define pTrigger 0
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 92bf9943b..773290b28 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -4717,7 +4717,7 @@ table_column_is_nullable(struct Table *tab, uint32_t column)
 {
 	/* Temporary hack: until Tarantoool's ephemeral spaces are on-boarded,
 	*  views are not handled properly in Tarantool as well. */
-	if (!(tab->tabFlags | TF_Ephemeral || tab->pSelect != NULL)) {
+	if (!(tab->tabFlags | TF_Ephemeral || space_is_view(tab))) {
 		uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(tab->tnum);
 		struct space *space = space_cache_find(space_id);
 
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index ae1ca6d9f..af87d52bd 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4511,7 +4511,8 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 		pTabItem = &pTabList->a[pLevel->iFrom];
 		pTab = pTabItem->pTab;
 		pLoop = pLevel->pWLoop;
-		if ((pTab->tabFlags & TF_Ephemeral) != 0 || pTab->pSelect) {
+		if ((pTab->tabFlags & TF_Ephemeral) != 0 ||
+		    space_is_view(pTab)) {
 			/* Do nothing */
 		} else if ((pLoop->wsFlags & WHERE_IDX_ONLY) == 0 &&
 			   (wctrlFlags & WHERE_OR_SUBCLAUSE) == 0) {
diff --git a/test/sql/transition.result b/test/sql/transition.result
index 500fe85c6..7c4a2c813 100644
--- a/test/sql/transition.result
+++ b/test/sql/transition.result
@@ -112,10 +112,6 @@ box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'")
 ---
 - - [0]
 ...
--- cleanup
-box.space.FOOBAR:drop()
----
-...
 -- multi-index
 -- create space
 box.sql.execute("CREATE TABLE barfoo (bar, foo NUM PRIMARY KEY)")
@@ -188,9 +184,6 @@ box.sql.execute("CREATE TABLE rowid(x)")
 box.sql.execute("CREATE TABLE implicit_indices(a PRIMARY KEY,b,c,d UNIQUE)")
 ---
 ...
-box.space.IMPLICIT_INDICES:drop()
----
-...
 box.sql.execute("DROP TABLE implicit_indices")
 ---
 ...
diff --git a/test/sql/transition.test.lua b/test/sql/transition.test.lua
index 996e634ec..50ee7f569 100644
--- a/test/sql/transition.test.lua
+++ b/test/sql/transition.test.lua
@@ -33,9 +33,6 @@ box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'")
 box.sql.execute("DELETE FROM foobar WHERE bar='cacodaemon'")
 box.sql.execute("SELECT COUNT(*) FROM foobar WHERE bar='cacodaemon'")
 
--- cleanup
-box.space.FOOBAR:drop()
-
 -- multi-index
 
 -- create space
@@ -69,5 +66,4 @@ box.sql.execute("CREATE TABLE rowid(x)")
 
 -- create a table with implicit indices (used to SEGFAULT)
 box.sql.execute("CREATE TABLE implicit_indices(a PRIMARY KEY,b,c,d UNIQUE)")
-box.space.IMPLICIT_INDICES:drop()
 box.sql.execute("DROP TABLE implicit_indices")
-- 
2.15.1

[tarantool-patches] Re: [PATCH v2 1/2] Introduce 'view' space option

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [18/03/27 02:06]:
> +	if (opts.view && opts.sql == NULL) {
> +		tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
> +	}

We usually do not add {} braces around one-line if bodies.

>  	struct space_def *def =
>  		space_def_new_xc(id, uid, exact_field_count, name, name_len,
>  				 engine_name, engine_name_len, &opts, fields,
> @@ -1590,6 +1593,10 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>  	uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple,
>  					  BOX_INDEX_FIELD_ID);
>  	struct space *old_space = space_cache_find_xc(id);
> +	if (old_space->def->opts.view) {
> +		tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
> +			  "can not alter indexes on view");
> +	}

can not add index on a view.

You know for sure it's add, since it can't be drop or modify,
so let's the error message more specific.

You can also introduce a separate error code for the purpose,
ER_ALTER_VIEW.

>  const struct space_opts space_opts_default = {
>  	/* .temporary = */ false,
> +	/* .view = */ false,
>  	/* .sql        = */ NULL,
>  };

is_view. Usually we add is_  or has_ prefix to boolean state
variables.

> +	if (new_def->opts.view != old_def->opts.view) {
> +		diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
> +			 "can not transform space to view and visa versa");
> +		return -1;
> +	}

can't convert a space to a view or vice vice versa.

In future, please check all error messages you add with Elena.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v2 2/2] sql: use 'view' opts from space

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [18/03/27 02:06]:
>  
> +/**
> + * Return true if space which corresponds to
> + * given table has view option.
> + */
> +bool
> +space_is_view(Table *table)
> +{
> +	assert(table != NULL);
> +	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
> +	struct space *space = space_by_id(space_id);
> +	assert(space != NULL);
> +	return space->def->opts.view;
> +}

I assume this implementation will change in a few weeks.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.org - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v2 1/2] Introduce 'view' space option

[n.pettik]

> On 27 Mar 2018, at 09:02, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * Nikita Pettik <korablev@tarantool.org> [18/03/27 02:06]:
>> +	if (opts.view && opts.sql == NULL) {
>> +		tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
>> +	}
> 
> We usually do not add {} braces around one-line if bodies.

Fixed:

-       if (opts.view && opts.sql == NULL) {
+       if (opts.is_view && opts.sql == NULL)
                tnt_raise(ClientError, ER_VIEW_MISSING_SQL);
-       }

> 
>> 	struct space_def *def =
>> 		space_def_new_xc(id, uid, exact_field_count, name, name_len,
>> 				 engine_name, engine_name_len, &opts, fields,
>> @@ -1590,6 +1593,10 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>> 	uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple,
>> 					  BOX_INDEX_FIELD_ID);
>> 	struct space *old_space = space_cache_find_xc(id);
>> +	if (old_space->def->opts.view) {
>> +		tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
>> +			  "can not alter indexes on view");
>> +	}
> 
> can not add index on a view.
> 
> You know for sure it's add, since it can't be drop or modify,
> so let's the error message more specific.
> 
> You can also introduce a separate error code for the purpose,
> ER_ALTER_VIEW.

Fixed:

-       if (old_space->def->opts.view) {
+       if (old_space->def->opts.is_view) {
                tnt_raise(ClientError, ER_ALTER_SPACE, space_name(old_space),
-                         "can not alter indexes on view");
+                         "can not add index on a view");


> 
>> const struct space_opts space_opts_default = {
>> 	/* .temporary = */ false,
>> +	/* .view = */ false,
>> 	/* .sql        = */ NULL,
>> };
> 
> is_view. Usually we add is_  or has_ prefix to boolean state
> variables.

But field ’temporary’ in the same struct isn’t called ‘is_temporary’,
so I just didn’t want to outline this field. Anyway, renamed to ‘is_view’.

> 
>> +	if (new_def->opts.view != old_def->opts.view) {
>> +		diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
>> +			 "can not transform space to view and visa versa");
>> +		return -1;
>> +	}
> 
> can't convert a space to a view or vice vice versa.
> 
> In future, please check all error messages you add with Elena.
> 

Fixed:

-       if (new_def->opts.view != old_def->opts.view) {
+       if (new_def->opts.is_view != old_def->opts.is_view) {
                diag_set(ClientError, ER_ALTER_SPACE, old_def->name,
-                        "can not transform space to view and visa versa");
+                        "can not convert a space to a view and vice versa");

[tarantool-patches] Re: [PATCH v2 2/2] sql: use 'view' opts from space

[n.pettik]

> On 27 Mar 2018, at 09:04, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * Nikita Pettik <korablev@tarantool.org> [18/03/27 02:06]:
>> 
>> +/**
>> + * Return true if space which corresponds to
>> + * given table has view option.
>> + */
>> +bool
>> +space_is_view(Table *table)
>> +{
>> +	assert(table != NULL);
>> +	uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
>> +	struct space *space = space_by_id(space_id);
>> +	assert(space != NULL);
>> +	return space->def->opts.view;
>> +}
> 
> I assume this implementation will change in a few weeks.

It’s true. Added FIXME clarification:

@@ -1163,6 +1163,10 @@ index_collation_name(Index *idx, uint32_t column)
 /**
  * Return true if space which corresponds to
  * given table has view option.
+ *
+ * FIXME: this is temporary wrapper around SQL specific struct.
+ * It will be removed after data dictionary integration
+ * into SQL is finished.

[tarantool-patches] Re: [PATCH v2 0/2] Introduce 'view' space option

[v.shpilevoy]

LGTM.

> 27 марта 2018 г., в 2:03, Nikita Pettik <korablev@tarantool.org> написал(а):
> 
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-3268-introduce-view
> Issue: https://github.com/tarantool/tarantool/issues/3268                          
> 
> Changelog:                                                                         
> - Added tests to check that view can't be created via Lua 'create_space'
>   function, and to check that view can be created by direct insertion
>   to _space.                                                                      
> - Implemented SQL follow-up patch to remove TF_View flag and to fetch
>   'view' property from SQL.     
> 
> Nikita Pettik (2):
>  Introduce 'view' space option
>  sql: use 'view' opts from space
> 
> src/box/alter.cc             |  7 ++++
> src/box/errcode.h            |  3 +-
> src/box/space_def.c          |  7 ++++
> src/box/space_def.h          |  6 +++
> src/box/sql.c                |  9 +++-
> src/box/sql/alter.c          |  4 +-
> src/box/sql/build.c          | 27 ++++++++----
> src/box/sql/delete.c         |  2 +-
> src/box/sql/fkey.c           |  3 +-
> src/box/sql/insert.c         | 10 +++--
> src/box/sql/parse.c          |  4 +-
> src/box/sql/parse.y          |  5 +--
> src/box/sql/select.c         |  4 +-
> src/box/sql/sqliteInt.h      |  5 ++-
> src/box/sql/trigger.c        |  4 +-
> src/box/sql/update.c         |  2 +-
> src/box/sql/vdbeaux.c        |  2 +-
> src/box/sql/where.c          |  3 +-
> test/box/misc.result         |  1 +
> test/sql/transition.result   |  7 ----
> test/sql/transition.test.lua |  4 --
> test/sql/view.result         | 98 ++++++++++++++++++++++++++++++++++++++++++++
> test/sql/view.test.lua       | 46 +++++++++++++++++++++
> 23 files changed, 220 insertions(+), 43 deletions(-)
> create mode 100644 test/sql/view.result
> create mode 100644 test/sql/view.test.lua
> 
> -- 
> 2.15.1
> 
>