Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: define format of ephemeral spaces
@ 2021-07-15 14:14 Mergen Imeev via Tarantool-patches
  2021-07-23 23:06 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 1 reply; 2+ messages in thread
From: Mergen Imeev via Tarantool-patches @ 2021-07-15 14:14 UTC (permalink / raw)
  To: v.shpilevoy, kyukhin; +Cc: tarantool-patches

After this patch, most of the created ephemeral spaces will have defined
field types in their format. Previously, for some fields, in some cases,
the type was defined, but mostly fields were of SCALAR type by default.

There are two cases where the old approach is used:
1) In the IN operator, since this operator accepts values of any scalar
types as valid right values.
2) In some cases during ORDER BY, as some optimizations use the old
approach. This case should be fixed in its own patch.

Part of #6213
---
https://github.com/tarantool/tarantool/issues/6213
https://github.com/tarantool/tarantool/tree/imeevma/gh-6213-field-types-in-ephemeral-spaces

 src/box/sql.c              | 119 ++++++++++++++++-
 src/box/sql/delete.c       |  41 +++---
 src/box/sql/expr.c         |  21 ++-
 src/box/sql/insert.c       |  49 ++++---
 src/box/sql/select.c       | 261 ++++++++++++++++++++++++++++++-------
 src/box/sql/sqlInt.h       |  25 ++++
 src/box/sql/tarantoolInt.h |  14 +-
 src/box/sql/update.c       |  20 ++-
 src/box/sql/vdbe.c         |  25 +++-
 src/box/sql/vdbe.h         |  15 +--
 src/box/sql/vdbeaux.c      |  15 +--
 src/box/sql/where.c        |  11 +-
 src/box/sql/wherecode.c    |  15 ++-
 13 files changed, 494 insertions(+), 137 deletions(-)

diff --git a/src/box/sql.c b/src/box/sql.c
index 790ca7f70..f5d0d6ce6 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -298,7 +298,8 @@ tarantoolsqlCount(struct BtCursor *pCur)
 }
 
 struct space *
-sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
+sql_ephemeral_space_create_key_info(uint32_t field_count,
+				    struct sql_key_info *key_info)
 {
 	struct key_def *def = NULL;
 	uint32_t part_count = field_count;
@@ -407,6 +408,122 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
 	return ephemer_new_space;
 }
 
+enum {
+	/*
+	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2" and so on. Due to
+	 * this, length of each name is no more than strlen("_COLUMN_") plus
+	 * length of UINT32_MAX turned to string, which is 10 and plus 1 for \0.
+	 */
+	NAME_LEN = 19,
+};
+
+static struct space_def *
+sql_space_def_new_ephemeral(uint32_t field_count, enum field_type *types,
+			    uint32_t *coll_ids)
+{
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	uint32_t size = field_count * sizeof(struct field_def);
+	struct field_def *fields = region_aligned_alloc(region, size,
+							alignof(fields[0]));
+	if (fields == NULL) {
+		diag_set(OutOfMemory, size, "region_aligned_alloc", "fields");
+		return NULL;
+	}
+	char *names = region_alloc(region, field_count * NAME_LEN);
+	if (names == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "names");
+		return NULL;
+	}
+	for (uint32_t i = 0; i < field_count; ++i) {
+		struct field_def *field = &fields[i];
+		field->name = &names[i * NAME_LEN];
+		sprintf(field->name, "_COLUMN_%d", i);
+		field->is_nullable = true;
+		field->nullable_action = ON_CONFLICT_ACTION_NONE;
+		field->default_value = NULL;
+		field->default_value_expr = NULL;
+		field->type = types[i];
+		field->coll_id = coll_ids[i];
+	}
+	struct space_def *def = space_def_new_ephemeral(field_count, fields);
+	region_truncate(region, svp);
+	return def;
+}
+
+static struct index_def *
+sql_index_def_new_ephemeral(uint32_t field_count, enum field_type *types,
+			    uint32_t *coll_ids, bool is_pk_rowid)
+{
+	uint32_t part_count = is_pk_rowid ? 1 : field_count;
+	struct region *region = &fiber()->gc;
+	size_t svp = region_used(region);
+	uint32_t size = part_count * sizeof(struct key_part_def);
+	struct key_part_def *parts = region_aligned_alloc(region, size,
+							  alignof(parts[0]));
+	if (parts == NULL) {
+		diag_set(OutOfMemory, size, "region_aligned_alloc", "parts");
+		return NULL;
+	}
+	if (is_pk_rowid) {
+		uint32_t j = field_count - 1;
+		parts[0].fieldno = j;
+		parts[0].nullable_action = ON_CONFLICT_ACTION_NONE;
+		parts[0].is_nullable = true;
+		parts[0].exclude_null = false;
+		parts[0].sort_order = SORT_ORDER_ASC;
+		parts[0].path = NULL;
+		parts[0].type = types[j];
+		parts[0].coll_id = coll_ids[j];
+	} else {
+		for (uint32_t i = 0; i < part_count; ++i) {
+			struct key_part_def *part = &parts[i];
+			part->fieldno = i;
+			part->nullable_action = ON_CONFLICT_ACTION_NONE;
+			part->is_nullable = true;
+			part->exclude_null = false;
+			part->sort_order = SORT_ORDER_ASC;
+			part->path = NULL;
+			part->type = types[i];
+			part->coll_id = coll_ids[i];
+		}
+	}
+	struct key_def *key_def = key_def_new(parts, part_count, false);
+	if (key_def == NULL)
+		return NULL;
+	const char *name = "ephemer_idx";
+	struct index_def *def = index_def_new(0, 0, name, strlen(name), TREE,
+					      &index_opts_default, key_def,
+					      NULL);
+	key_def_delete(key_def);
+	region_truncate(region, svp);
+	return def;
+}
+
+struct space *
+sql_ephemeral_space_create(uint32_t field_count, enum field_type *types,
+			   uint32_t *coll_ids, bool is_pk_rowid)
+{
+	struct space_def *space_def;
+	space_def = sql_space_def_new_ephemeral(field_count, types, coll_ids);
+	if (space_def == NULL)
+		return NULL;
+	struct index_def *index_def;
+	index_def = sql_index_def_new_ephemeral(field_count, types, coll_ids,
+						is_pk_rowid);
+	if (index_def == NULL) {
+		space_def_delete(space_def);
+		return NULL;
+	}
+	struct rlist key_list;
+	rlist_create(&key_list);
+	rlist_add_entry(&key_list, index_def, link);
+	struct space *space = space_new_ephemeral(space_def, &key_list);
+	index_def_delete(index_def);
+	space_def_delete(space_def);
+	return space;
+}
+
 int tarantoolsqlEphemeralInsert(struct space *space, const char *tuple,
 				    const char *tuple_end)
 {
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 62a726fdd..b35545296 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -224,12 +224,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		 * is held in ephemeral table, there is no PK for
 		 * it, so columns should be loaded manually.
 		 */
-		struct sql_key_info *pk_info = NULL;
 		int reg_eph = ++parse->nMem;
 		int reg_pk = parse->nMem + 1;
-		int pk_len;
+		int pk_len = is_view ? space->def->field_count + 1 :
+			     space->index[0]->def->key_def->part_count;
 		int eph_cursor = parse->nTab++;
 		int addr_eph_open = sqlVdbeCurrentAddr(v);
+		struct fields_info *info = fields_info_new(pk_len);
+		if (info == NULL) {
+			parse->is_aborted = true;
+			goto delete_from_cleanup;
+		}
 		if (is_view) {
 			/*
 			 * At this stage SELECT is already materialized
@@ -249,22 +254,20 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			 * account that id field as well. That's why pk_len
 			 * has one field more than view format.
 			 */
-			pk_len = space->def->field_count + 1;
-			parse->nMem += pk_len;
-			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
-					  pk_len);
+			uint32_t count = space->def->field_count;
+			fields_info_from_space_def(info->types, info->coll_ids,
+						   count, space->def);
+			info->types[count] = FIELD_TYPE_UNSIGNED;
+			info->coll_ids[count] = COLL_NONE;
 		} else {
-                        assert(space->index_count > 0);
-                        pk_info = sql_key_info_new_from_key_def(db,
-					space->index[0]->def->key_def);
-                        if (pk_info == NULL)
-                                goto delete_from_cleanup;
-                        pk_len = pk_info->part_count;
-                        parse->nMem += pk_len;
-			sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
-					  pk_len, 0,
-					  (char *)pk_info, P4_KEYINFO);
+			assert(space->index_count > 0);
+			fields_info_from_index_def(info->types, info->coll_ids,
+						   pk_len,
+						   space->index[0]->def);
 		}
+		parse->nMem += pk_len;
+		sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, pk_len, 0,
+			      (char *)info, P4_SPACEINFO);
 
 		/* Construct a query to find the primary key for
 		 * every row to be deleted, based on the WHERE
@@ -295,8 +298,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 
 		/* Extract the primary key for the current row */
 		if (!is_view) {
-			struct key_part_def *part = pk_info->parts;
-			for (int i = 0; i < pk_len; i++, part++) {
+			struct key_def *def = space->index[0]->def->key_def;
+			for (int i = 0; i < pk_len; i++) {
+				struct key_part *part = &def->parts[i];
 				sqlVdbeAddOp3(v, OP_Column, tab_cursor,
 					      part->fieldno, reg_pk + i);
 			}
@@ -377,7 +381,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		if (one_pass != ONEPASS_OFF) {
 			/* OP_Found will use an unpacked key. */
 			assert(key_len == pk_len);
-			assert(pk_info != NULL || space->def->opts.is_view);
 			sqlVdbeAddOp4Int(v, OP_NotFound, tab_cursor,
 					     addr_bypass, reg_key, key_len);
 
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3772596d6..cc7456371 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2773,7 +2773,6 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 
 	switch (pExpr->op) {
 	case TK_IN:{
-			int addr;	/* Address of OP_OpenEphemeral instruction */
 			Expr *pLeft = pExpr->pLeft;	/* the LHS of the IN operator */
 			int nVal;	/* Size of vector pLeft */
 
@@ -2794,13 +2793,13 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 			 */
 			pExpr->iTable = pParse->nTab++;
 			int reg_eph = ++pParse->nMem;
-			addr = sqlVdbeAddOp2(v, OP_OpenTEphemeral,
-						 reg_eph, nVal);
+			struct fields_info *info = fields_info_new(nVal);
+			if (info == NULL)
+				return 0;
+			sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, nVal, 0,
+				      (char *)info, P4_SPACEINFO);
 			sqlVdbeAddOp3(v, OP_IteratorOpen, pExpr->iTable, 0,
 					  reg_eph);
-			struct sql_key_info *key_info = sql_key_info_new(pParse->db, nVal);
-			if (key_info == NULL)
-				return 0;
 
 			if (ExprHasProperty(pExpr, EP_xIsSelect)) {
 				/* Case 1:     expr IN (SELECT ...)
@@ -2830,7 +2829,6 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 					    (pParse, pSelect, &dest)) {
 						sqlDbFree(pParse->db,
 							      dest.dest_type);
-						sql_key_info_unref(key_info);
 						return 0;
 					}
 					sqlDbFree(pParse->db,
@@ -2841,8 +2839,10 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 						Expr *p =
 						    sqlVectorFieldSubexpr
 						    (pLeft, i);
+						info->types[i] =
+							FIELD_TYPE_SCALAR;
 						if (sql_binary_compare_coll_seq(pParse, p, pEList->a[i].pExpr,
-										&key_info->parts[i].coll_id) != 0)
+										&info->coll_ids[i]) != 0)
 							return 0;
 					}
 				}
@@ -2863,8 +2863,9 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 					sql_expr_type(pLeft);
 				bool unused;
 				struct coll *unused_coll;
+				info->types[0] = FIELD_TYPE_SCALAR;
 				if (sql_expr_coll(pParse, pExpr->pLeft, &unused,
-						  &key_info->parts[0].coll_id,
+						  &info->coll_ids[0],
 						  &unused_coll) != 0)
 					return 0;
 
@@ -2899,8 +2900,6 @@ sqlCodeSubselect(Parse * pParse,	/* Parsing context */
 				sqlReleaseTempReg(pParse, r1);
 				sqlReleaseTempReg(pParse, r2);
 			}
-			sqlVdbeChangeP4(v, addr, (void *)key_info,
-					    P4_KEYINFO);
 			break;
 		}
 
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 02e9f9673..7d126f7e1 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -456,34 +456,41 @@ sqlInsert(Parse * pParse,	/* Parser context */
 			reg_eph = ++pParse->nMem;
 			regRec = sqlGetTempReg(pParse);
 			regCopy = sqlGetTempRange(pParse, nColumn + 1);
-			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
-					  nColumn + 1);
+			struct fields_info *info = fields_info_new(nColumn + 1);
+			if (info == NULL) {
+				pParse->is_aborted = true;
+				goto insert_cleanup;
+			}
+			if (pColumn != NULL) {
+				struct field_def *fields = space_def->fields;
+				for (int i = 0; i < nColumn; ++i) {
+					int j = pColumn->a[i].idx;
+					info->types[i] = fields[j].type;
+					info->coll_ids[i] = fields[j].coll_id;
+				}
+			} else {
+				fields_info_from_space_def(info->types,
+							   info->coll_ids,
+							   nColumn, space->def);
+			}
+			info->types[nColumn] = FIELD_TYPE_UNSIGNED;
+			info->coll_ids[nColumn] = COLL_NONE;
+			sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
+				      nColumn + 1, 0, (char *)info,
+				      P4_SPACEINFO);
 			/*
-			 * This key_info is used to show that
-			 * rowid should be the first part of PK in
-			 * case we used AUTOINCREMENT feature.
-			 * This way we will save initial order of
-			 * the inserted values. The order is
-			 * important if we use the AUTOINCREMENT
-			 * feature, since changing the order can
-			 * change the number inserted instead of
-			 * NULL.
+			 * Order of inserted values is important since it is
+			 * possible, that NULL will be inserted in field with
+			 * AUTOINCREMENT.
 			 */
-			if (space->sequence != NULL) {
-				struct sql_key_info *key_info =
-					sql_key_info_new(pParse->db,
-							 nColumn + 1);
-				key_info->parts[nColumn].type =
-					FIELD_TYPE_UNSIGNED;
-				key_info->is_pk_rowid = true;
-				sqlVdbeChangeP4(v, -1, (void *)key_info,
-					        P4_KEYINFO);
-			}
+			sqlVdbeChangeP5(v, OPFLAG_ROWID_PK);
 			addrL = sqlVdbeAddOp1(v, OP_Yield, dest.iSDParm);
 			VdbeCoverage(v);
 			sqlVdbeAddOp2(v, OP_NextIdEphemeral, reg_eph,
 					  regCopy + nColumn);
 			sqlVdbeAddOp3(v, OP_Copy, regFromSelect, regCopy, nColumn-1);
+			sqlVdbeAddOp4(v, OP_ApplyType, regCopy, nColumn + 1, 0,
+				      (char *)info->types, P4_STATIC);
 			sqlVdbeAddOp3(v, OP_MakeRecord, regCopy,
 					  nColumn + 1, regRec);
 			/* Set flag to save memory allocating one by malloc. */
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b9107fccc..c59d7b4b4 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -99,6 +99,103 @@ struct SortCtx {
 #define SORTFLAG_UseSorter  0x01	/* Use SorterOpen instead of OpenEphemeral */
 #define SORTFLAG_DESC 0xF0
 
+static inline uint32_t
+multi_select_coll_seq(struct Parse *parser, struct Select *p, int n);
+
+struct fields_info *
+fields_info_new(uint32_t len)
+{
+	uint32_t size_info = sizeof(struct fields_info);
+	uint32_t size_types = len * sizeof(enum field_type);
+	uint32_t size = size_info + size_types + len * (sizeof(uint32_t));
+	char *buf = sqlDbMallocRawNN(sql_get(), size);
+	struct fields_info *info = (struct fields_info *)buf;
+	if (info == NULL)
+		return NULL;
+	info->key_info = NULL;
+	buf += size_info;
+	info->types = (enum field_type *)buf;
+	buf += size_types;
+	info->coll_ids = (uint32_t *)buf;
+	return info;
+}
+
+void
+fields_info_delete(struct fields_info *info)
+{
+	if (info->key_info != NULL)
+		sql_key_info_unref(info->key_info);
+	return sqlDbFree(sql_get(), info);
+}
+
+void
+fields_info_from_space_def(enum field_type *types, uint32_t *coll_ids,
+			   uint32_t count, struct space_def *def)
+{
+	for (uint32_t i = 0; i < count; ++i) {
+		types[i] = def->fields[i].type;
+		coll_ids[i] = def->fields[i].coll_id;
+	}
+}
+
+void
+fields_info_from_index_def(enum field_type *types, uint32_t *coll_ids,
+			   uint32_t count, struct index_def *def)
+{
+	for (uint32_t i = 0; i < count; ++i) {
+		types[i] = def->key_def->parts[i].type;
+		coll_ids[i] = def->key_def->parts[i].coll_id;
+	}
+}
+
+static int
+fields_info_from_expr_list(enum field_type *types, uint32_t *coll_ids,
+			   struct Parse *parser, struct ExprList *list)
+{
+	for (int i = 0; i < list->nExpr; ++i) {
+		bool b;
+		struct coll *coll;
+		struct Expr *expr = list->a[i].pExpr;
+		types[i] = sql_expr_type(expr);
+		if (types[i] == FIELD_TYPE_ANY)
+			types[i] = FIELD_TYPE_SCALAR;
+		if (sql_expr_coll(parser, expr, &b, &coll_ids[i], &coll) != 0)
+			return -1;
+	}
+	return 0;
+}
+
+static int
+fields_info_from_order_by(enum field_type *types, uint32_t *coll_ids,
+			  struct Parse *parser, struct Select *select,
+			  struct ExprList *order_by)
+{
+	for (int i = 0; i < order_by->nExpr; ++i) {
+		bool b;
+		struct Expr *expr = order_by->a[i].pExpr;
+		types[i] = sql_expr_type(expr);
+		if (types[i] == FIELD_TYPE_ANY)
+			types[i] = FIELD_TYPE_SCALAR;
+		uint32_t id = COLL_NONE;
+		if ((expr->flags & EP_Collate) != 0) {
+			struct coll *coll;
+			if (sql_expr_coll(parser, expr, &b, &id, &coll) != 0)
+				return -1;
+		} else {
+			uint32_t fieldno = order_by->a[i].u.x.iOrderByCol - 1;
+			id = multi_select_coll_seq(parser, select, fieldno);
+			if (id != COLL_NONE) {
+				const char *name = coll_by_id(id)->name;
+				order_by->a[i].pExpr =
+					sqlExprAddCollateString(parser, expr,
+								name);
+			}
+		}
+		coll_ids[i] = id;
+	}
+	return 0;
+}
+
 /*
  * Delete all the content of a Select structure.  Deallocate the structure
  * itself only if bFree is true.
@@ -819,13 +916,13 @@ pushOntoSorter(Parse * pParse,		/* Parser context */
 		if (pParse->db->mallocFailed)
 			return;
 		pOp->p2 = nKey + nData;
-		struct sql_key_info *key_info = pOp->p4.key_info;
+		struct sql_key_info *key_info = pOp->p4.fields->key_info;
 		for (uint32_t i = 0; i < key_info->part_count; i++)
 			key_info->parts[i].sort_order = SORT_ORDER_ASC;
 		sqlVdbeChangeP4(v, -1, (char *)key_info, P4_KEYINFO);
-		pOp->p4.key_info = sql_expr_list_to_key_info(pParse,
-							     pSort->pOrderBy,
-							     nOBSat);
+		pOp->p4.fields->key_info =
+			sql_expr_list_to_key_info(pParse, pSort->pOrderBy,
+						  nOBSat);
 		addrJmp = sqlVdbeCurrentAddr(v);
 		sqlVdbeAddOp3(v, OP_Jump, addrJmp + 1, 0, addrJmp + 1);
 		VdbeCoverage(v);
@@ -2453,16 +2550,35 @@ generateWithRecursiveQuery(Parse * pParse,	/* Parsing context */
 	/* Allocate cursors for Current, Queue, and Distinct. */
 	regCurrent = ++pParse->nMem;
 	sqlVdbeAddOp3(v, OP_OpenPseudo, iCurrent, regCurrent, nCol);
-	if (pOrderBy) {
-		struct sql_key_info *key_info =
-			sql_multiselect_orderby_to_key_info(pParse, p, 1);
-		sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_queue,
-				  pOrderBy->nExpr + 2, 0, (char *)key_info,
-				  P4_KEYINFO);
+
+	int field_count = pOrderBy != NULL ? pOrderBy->nExpr + 2 : nCol + 1;
+	struct fields_info *info = fields_info_new(field_count);
+	if (info == NULL) {
+		pParse->is_aborted = true;
+		goto end_of_recursive_query;
+	}
+	sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_queue, field_count, 0,
+		      (char *)info, P4_SPACEINFO);
+	if (pOrderBy != NULL) {
+		if (fields_info_from_order_by(info->types, info->coll_ids,
+					      pParse, p, pOrderBy) != 0) {
+			pParse->is_aborted = true;
+			goto end_of_recursive_query;
+		}
+		info->types[pOrderBy->nExpr] = FIELD_TYPE_UNSIGNED;
+		info->coll_ids[pOrderBy->nExpr] = COLL_NONE;
+		info->types[pOrderBy->nExpr + 1] = FIELD_TYPE_VARBINARY;
+		info->coll_ids[pOrderBy->nExpr + 1] = COLL_NONE;
 		VdbeComment((v, "Orderby table"));
 		destQueue.pOrderBy = pOrderBy;
 	} else {
-		sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_queue, nCol + 1);
+		if (fields_info_from_expr_list(info->types, info->coll_ids,
+					       pParse, p->pEList) != 0) {
+			pParse->is_aborted = true;
+			goto end_of_recursive_query;
+		}
+		info->types[nCol] = FIELD_TYPE_UNSIGNED;
+		info->coll_ids[nCol] = COLL_NONE;
 		VdbeComment((v, "Queue table"));
 	}
 	sqlVdbeAddOp3(v, OP_IteratorOpen, iQueue, 0, reg_queue);
@@ -2673,7 +2789,23 @@ multiSelect(Parse * pParse,	/* Parsing context */
 	if (dest.eDest == SRT_EphemTab) {
 		assert(p->pEList);
 		int nCols = p->pEList->nExpr;
-		sqlVdbeAddOp2(v, OP_OpenTEphemeral, dest.reg_eph, nCols + 1);
+		struct fields_info *info = fields_info_new(nCols + 1);
+		if (info == NULL) {
+			pParse->is_aborted = true;
+			rc = 1;
+			goto multi_select_end;
+		}
+		if (fields_info_from_expr_list(info->types, info->coll_ids,
+					       pParse, p->pEList) != 0) {
+			fields_info_delete(info);
+			pParse->is_aborted = true;
+			rc = 1;
+			goto multi_select_end;
+		}
+		info->types[nCols] = FIELD_TYPE_SCALAR;
+		info->coll_ids[nCols] = COLL_NONE;
+		sqlVdbeAddOp4(v, OP_OpenTEphemeral, dest.reg_eph, nCols + 1,
+			      0, (char *)info, P4_SPACEINFO);
 		sqlVdbeAddOp3(v, OP_IteratorOpen, dest.iSDParm, 0, dest.reg_eph);
 		VdbeComment((v, "Destination temp"));
 		dest.eDest = SRT_Table;
@@ -3001,14 +3133,17 @@ multiSelect(Parse * pParse,	/* Parsing context */
 	if (p->selFlags & SF_UsesEphemeral) {
 		assert(p->pNext == NULL);
 		int nCol = p->pEList->nExpr;
-		struct sql_key_info *key_info = sql_key_info_new(db, nCol);
-		if (key_info == NULL)
+		struct fields_info *info = fields_info_new(nCol);
+		if (info == NULL) {
+			pParse->is_aborted = true;
 			goto multi_select_end;
-		for (int i = 0; i < nCol; i++) {
-			key_info->parts[i].coll_id =
-				multi_select_coll_seq(pParse, p, i);
+		}
+		for (int i = 0; i < nCol; ++i) {
+			info->types[i] = FIELD_TYPE_SCALAR;
+			info->coll_ids[i] = multi_select_coll_seq(pParse, p, i);
 		}
 
+		bool is_info_used = false;
 		for (struct Select *pLoop = p; pLoop; pLoop = pLoop->pPrior) {
 			for (int i = 0; i < 2; i++) {
 				int addr = pLoop->addrOpenEphm[i];
@@ -3020,13 +3155,15 @@ multiSelect(Parse * pParse,	/* Parsing context */
 					break;
 				}
 				sqlVdbeChangeP2(v, addr, nCol);
-				sqlVdbeChangeP4(v, addr,
-						    (char *)sql_key_info_ref(key_info),
-						    P4_KEYINFO);
+				sqlVdbeChangeP4(v, addr, (char *)info,
+						is_info_used ?
+						P4_STATIC : P4_SPACEINFO);
+				is_info_used = true;
 				pLoop->addrOpenEphm[i] = -1;
 			}
 		}
-		sql_key_info_unref(key_info);
+		if (!is_info_used)
+			sqlDbFree(pParse->db, info);
 	}
 
  multi_select_end:
@@ -5347,17 +5484,27 @@ resetAccumulator(Parse * pParse, AggInfo * pAggInfo)
 					 "exactly one argument");
 				pParse->is_aborted = true;
 				pFunc->iDistinct = -1;
-			} else {
-				struct sql_key_info *key_info =
-					sql_expr_list_to_key_info(pParse,
-								  pE->x.pList,
-								  0);
-				sqlVdbeAddOp4(v, OP_OpenTEphemeral,
-						  pFunc->reg_eph, 1, 0,
-						  (char *)key_info, P4_KEYINFO);
-				sqlVdbeAddOp3(v, OP_IteratorOpen,
-						  pFunc->iDistinct, 0, pFunc->reg_eph);
+				return;
+			}
+			assert(pE->x.pList->nExpr == 1);
+			struct fields_info *info = fields_info_new(1);
+			if (info == NULL) {
+				pParse->is_aborted = true;
+				pFunc->iDistinct = -1;
+				return;
+			}
+			if (fields_info_from_expr_list(info->types,
+						       info->coll_ids,
+						       pParse,
+						       pE->x.pList) != 0) {
+				pParse->is_aborted = true;
+				pFunc->iDistinct = -1;
+				return;
 			}
+			sqlVdbeAddOp4(v, OP_OpenTEphemeral, pFunc->reg_eph, 1,
+				      0, (char *)info, P4_SPACEINFO);
+			sqlVdbeAddOp3(v, OP_IteratorOpen, pFunc->iDistinct, 0,
+				      pFunc->reg_eph);
 		}
 	}
 }
@@ -5874,11 +6021,15 @@ sqlSelect(Parse * pParse,		/* The parser context */
 		if (key_info->parts[0].sort_order == SORT_ORDER_DESC) {
 			sSort.sortFlags |= SORTFLAG_DESC;
 		}
-		sSort.addrSortIndex =
-		    sqlVdbeAddOp4(v, OP_OpenTEphemeral,
-				      sSort.reg_eph,
-				      nCols,
-				      0, (char *)key_info, P4_KEYINFO);
+		struct fields_info *info = fields_info_new(nCols);
+		if (info == NULL) {
+			pParse->is_aborted = true;
+			goto select_end;
+		}
+		info->key_info = key_info;
+		sSort.addrSortIndex = sqlVdbeAddOp4(v, OP_OpenTEphemeral,
+						    sSort.reg_eph, nCols, 0,
+						    (char *)info, P4_SPACEINFO);
 		sqlVdbeAddOp3(v, OP_IteratorOpen, sSort.iECursor, 0, sSort.reg_eph);
 		VdbeComment((v, "Sort table"));
 	} else {
@@ -5888,11 +6039,20 @@ sqlSelect(Parse * pParse,		/* The parser context */
 	/* If the output is destined for a temporary table, open that table.
 	 */
 	if (pDest->eDest == SRT_EphemTab) {
-		struct sql_key_info *key_info =
-			sql_expr_list_to_key_info(pParse, pEList, 0);
+		struct fields_info *info = fields_info_new(pEList->nExpr + 1);
+		if (info == NULL) {
+			pParse->is_aborted = true;
+			goto select_end;
+		}
 		sqlVdbeAddOp4(v, OP_OpenTEphemeral, pDest->reg_eph,
-				  pEList->nExpr + 1, 0, (char *)key_info,
-				  P4_KEYINFO);
+			      pEList->nExpr + 1, 0, (char *)info, P4_SPACEINFO);
+		if (fields_info_from_expr_list(info->types, info->coll_ids,
+					       pParse, pEList) != 0) {
+			pParse->is_aborted = true;
+			goto select_end;
+		}
+		info->types[pEList->nExpr] = FIELD_TYPE_UNSIGNED;
+		info->coll_ids[pEList->nExpr] = COLL_NONE;
 		sqlVdbeAddOp3(v, OP_IteratorOpen, pDest->iSDParm, 0,
 				  pDest->reg_eph);
 
@@ -5918,16 +6078,23 @@ sqlSelect(Parse * pParse,		/* The parser context */
 	if (p->selFlags & SF_Distinct) {
 		sDistinct.cur_eph = pParse->nTab++;
 		sDistinct.reg_eph = ++pParse->nMem;
-		struct sql_key_info *key_info =
-			sql_expr_list_to_key_info(pParse, p->pEList, 0);
+		struct fields_info *info = fields_info_new(pEList->nExpr);
+		if (info == NULL) {
+			pParse->is_aborted = true;
+			goto select_end;
+		}
 		sDistinct.addrTnct = sqlVdbeAddOp4(v, OP_OpenTEphemeral,
-						       sDistinct.reg_eph,
-						       key_info->part_count,
-						       0, (char *)key_info,
-						       P4_KEYINFO);
+						   sDistinct.reg_eph,
+						   pEList->nExpr, 0,
+						   (char *)info, P4_SPACEINFO);
+		VdbeComment((v, "Distinct table"));
+		if (fields_info_from_expr_list(info->types, info->coll_ids,
+					       pParse, pEList) != 0) {
+			pParse->is_aborted = true;
+			goto select_end;
+		}
 		sqlVdbeAddOp3(v, OP_IteratorOpen, sDistinct.cur_eph, 0,
 				  sDistinct.reg_eph);
-		VdbeComment((v, "Distinct table"));
 		sDistinct.eTnctType = WHERE_DISTINCT_UNORDERED;
 	} else {
 		sDistinct.eTnctType = WHERE_DISTINCT_NOOP;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index ef8dcd693..167e80057 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2248,6 +2248,11 @@ struct Parse {
 #define OPFLAG_SYSTEMSP      0x20	/* OP_Open**: set if space pointer
 					 * points to system space.
 					 */
+/**
+ * If this flag is set, than in OP_OpenTEphemeral we should use rowid as the
+ * only part of primary index.
+ */
+#define OPFLAG_ROWID_PK		0x01
 
 /**
  * Prepare vdbe P5 flags for OP_{IdxInsert, IdxReplace, Update}
@@ -4073,6 +4078,26 @@ sql_key_info_unref(struct sql_key_info *key_info);
 struct key_def *
 sql_key_info_to_key_def(struct sql_key_info *key_info);
 
+struct fields_info {
+	struct sql_key_info *key_info;
+	enum field_type *types;
+	uint32_t *coll_ids;
+};
+
+struct fields_info *
+fields_info_new(uint32_t len);
+
+void
+fields_info_delete(struct fields_info *info);
+
+void
+fields_info_from_space_def(enum field_type *types, uint32_t *coll_ids,
+			   uint32_t count, struct space_def *def);
+
+void
+fields_info_from_index_def(enum field_type *types, uint32_t *coll_ids,
+			   uint32_t count, struct index_def *def);
+
 /**
  * Check if the function implements LIKE-style comparison & if it
  * is appropriate to apply a LIKE query optimization.
diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
index 1ded6c709..a85c406d8 100644
--- a/src/box/sql/tarantoolInt.h
+++ b/src/box/sql/tarantoolInt.h
@@ -71,7 +71,19 @@ int tarantoolsqlRenameTrigger(const char *zTriggerName,
  * @retval Pointer to created space, NULL if error.
  */
 struct space *
-sql_ephemeral_space_create(uint32_t filed_count, struct sql_key_info *key_info);
+sql_ephemeral_space_create_key_info(uint32_t field_count,
+				    struct sql_key_info *key_info);
+
+/**
+ * Create an ephemeral space. The number of fields, their types and collations
+ * of the new ephemeral space will be determined using the given arguments. In
+ * case is_pk_rowid is TRUE, the primary index will contain only one field -
+ * rowid (which is assumed to be the last one). If it is FALSE, then the primary
+ * index must contain all fields.
+ */
+struct space *
+sql_ephemeral_space_create(uint32_t field_count, enum field_type *types,
+			   uint32_t *coll_ids, bool is_pk_rowid);
 
 /**
  * Insert tuple into ephemeral space.
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 24c7cfa27..81552f5f7 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -227,8 +227,24 @@ sqlUpdate(Parse * pParse,		/* The parser context */
 	sqlVdbeAddOp2(v, OP_Null, 0, iPk);
 
 	/* Address of the OpenEphemeral instruction. */
-	int addrOpen = sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
-					 pk_part_count);
+	struct fields_info *info = fields_info_new(pk_part_count);
+	if (info == NULL) {
+		pParse->is_aborted = true;
+		goto update_cleanup;
+	}
+	if (is_view) {
+		uint32_t n = space->def->field_count;
+		fields_info_from_space_def(info->types, info->coll_ids, n, def);
+		info->types[n] = FIELD_TYPE_UNSIGNED;
+		info->coll_ids[n] = COLL_NONE;
+	} else {
+		assert(space->index_count > 0);
+		fields_info_from_index_def(info->types, info->coll_ids,
+					   pk_part_count, pPk->def);
+	}
+	int addrOpen = sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph,
+				     pk_part_count, 0, (char *)info,
+				     P4_SPACEINFO);
 	pWInfo = sqlWhereBegin(pParse, pTabList, pWhere, 0, 0,
 				   WHERE_ONEPASS_DESIRED, pk_cursor);
 	if (pWInfo == 0)
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index cc698b715..03138e2c3 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2145,10 +2145,9 @@ case OP_Fetch: {
 case OP_ApplyType: {
 	enum field_type *types = pOp->p4.types;
 	assert(types != NULL);
-	assert(types[pOp->p2] == field_type_MAX);
 	pIn1 = &aMem[pOp->p1];
-	enum field_type type;
-	while((type = *(types++)) != field_type_MAX) {
+	for (int i = 0; i < pOp->p2; ++i) {
+		enum field_type type = types[i];
 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
 		assert(memIsValid(pIn1));
 		if (mem_cast_implicit(pIn1, type) != 0) {
@@ -2521,10 +2520,18 @@ open_cursor_set_hints:
 case OP_OpenTEphemeral: {
 	assert(pOp->p1 >= 0);
 	assert(pOp->p2 > 0);
-	assert(pOp->p4type != P4_KEYINFO || pOp->p4.key_info != NULL);
+	assert(pOp->p4type == P4_SPACEINFO || pOp->p4type == P4_STATIC);
 
-	struct space *space = sql_ephemeral_space_create(pOp->p2,
-							 pOp->p4.key_info);
+	struct space *space;
+	if (pOp->p4.fields->key_info == NULL) {
+		enum field_type *types = pOp->p4.fields->types;
+		uint32_t *coll_ids = pOp->p4.fields->coll_ids;
+		space = sql_ephemeral_space_create(pOp->p2, types, coll_ids,
+						   pOp->p5 == OPFLAG_ROWID_PK);
+	} else {
+		struct sql_key_info *key_info = pOp->p4.fields->key_info;
+		space = sql_ephemeral_space_create_key_info(pOp->p2, key_info);
+	}
 
 	if (space == NULL)
 		goto abort_due_to_error;
@@ -2547,7 +2554,11 @@ case OP_SorterOpen: {
 
 	assert(pOp->p1>=0);
 	assert(pOp->p2>=0);
-	struct key_def *def = sql_key_info_to_key_def(pOp->p4.key_info);
+	struct key_def *def;
+	if (pOp->p4type == P4_SPACEINFO)
+		def = sql_key_info_to_key_def(pOp->p4.fields->key_info);
+	else
+		def = sql_key_info_to_key_def(pOp->p4.key_info);
 	if (def == NULL) goto no_mem;
 	pCx = allocateCursor(p, pOp->p1, pOp->p2, CURTYPE_SORTER);
 	if (pCx==0) goto no_mem;
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 118f1cd83..a9d1d6a5e 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -93,6 +93,7 @@ struct VdbeOp {
 		 * doing a cast.
 		 */
 		enum field_type *types;
+		struct fields_info *fields;
 	} p4;
 #ifdef SQL_ENABLE_EXPLAIN_COMMENTS
 	char *zComment;		/* Comment to improve readability */
@@ -142,6 +143,11 @@ struct SubProgram {
 #define P4_PTR      (-18)	/* P4 is a generic pointer */
 #define P4_KEYINFO  (-19)       /* P4 is a pointer to sql_key_info structure. */
 #define P4_SPACEPTR (-20)       /* P4 is a space pointer */
+/**
+ * P4 is a structure that contains information about types and collations of
+ * fields.
+ */
+#define P4_SPACEINFO (-21)
 
 /* Error message codes for OP_Halt */
 #define P5_ConstraintNotNull 1
@@ -210,15 +216,6 @@ int sqlVdbeDeletePriorOpcode(Vdbe *, u8 op);
 void sqlVdbeChangeP4(Vdbe *, int addr, const char *zP4, int N);
 void sqlVdbeAppendP4(Vdbe *, void *pP4, int p4type);
 
-/**
- * Set the P4 on the most recently added opcode to the key_def for the
- * index given.
- * @param Parse context, for error reporting.
- * @param key_def Definition of a key to set.
- */
-void
-sql_vdbe_set_p4_key_def(struct Parse *parse, struct key_def *key_def);
-
 VdbeOp *sqlVdbeGetOp(Vdbe *, int);
 int sqlVdbeMakeLabel(Vdbe *);
 void sqlVdbeRunOnlyOnce(Vdbe *);
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 4a1fdb637..02a3ed878 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -613,6 +613,9 @@ freeP4(sql * db, int p4type, void *p4)
 			sqlDbFree(db, p4);
 			break;
 		}
+	case P4_SPACEINFO:
+		fields_info_delete(p4);
+		break;
 	case P4_KEYINFO:
 		sql_key_info_unref(p4);
 		break;
@@ -787,18 +790,6 @@ sqlVdbeAppendP4(Vdbe * p, void *pP4, int n)
 	}
 }
 
-void
-sql_vdbe_set_p4_key_def(struct Parse *parse, struct key_def *key_def)
-{
-	struct Vdbe *v = parse->pVdbe;
-	assert(v != NULL);
-	assert(key_def != NULL);
-	struct sql_key_info *key_info =
-		sql_key_info_new_from_key_def(parse->db, key_def);
-	if (key_info != NULL)
-		sqlVdbeAppendP4(v, key_info, P4_KEYINFO);
-}
-
 #ifdef SQL_ENABLE_EXPLAIN_COMMENTS
 /*
  * Change the comment on the most recently coded instruction.  Or
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e5f35fbf8..79c4a5b82 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -919,15 +919,18 @@ constructAutomaticIndex(Parse * pParse,			/* The parsing context */
 	/* Create the automatic index */
 	assert(pLevel->iIdxCur >= 0);
 	pLevel->iIdxCur = pParse->nTab++;
-	struct sql_key_info *pk_info =
-		sql_key_info_new_from_key_def(pParse->db, idx_def->key_def);
-	if (pk_info == NULL) {
+	struct fields_info *info = fields_info_new(nKeyCol + 1);
+	if (info == NULL) {
 		pParse->is_aborted = true;
 		return;
 	}
+	fields_info_from_index_def(info->types, info->coll_ids, nKeyCol,
+				   idx_def);
+	info->types[nKeyCol] = FIELD_TYPE_UNSIGNED;
+	info->coll_ids[nKeyCol] = COLL_NONE;
 	int reg_eph = sqlGetTempReg(pParse);
 	sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_eph, nKeyCol + 1, 0,
-		      (char *)pk_info, P4_KEYINFO);
+		      (char *)info, P4_SPACEINFO);
 	sqlVdbeAddOp3(v, OP_IteratorOpen, pLevel->iIdxCur, 0, reg_eph);
 	VdbeComment((v, "for %s", space->def->name));
 
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 96bcab110..5b689cfff 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1345,11 +1345,20 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about the W
 		if ((pWInfo->wctrlFlags & WHERE_DUPLICATES_OK) == 0) {
 			cur_row_set = pParse->nTab++;
 			reg_row_set = ++pParse->nMem;
-			sqlVdbeAddOp2(v, OP_OpenTEphemeral,
-					  reg_row_set, pk_part_count);
+			struct fields_info *info =
+				fields_info_new(pk_part_count);
+			if (info == NULL) {
+				pParse->is_aborted = true;
+				return notReady;
+			}
+			fields_info_from_index_def(info->types, info->coll_ids,
+						   pk_part_count,
+						   space_index(space, 0)->def);
+			sqlVdbeAddOp4(v, OP_OpenTEphemeral, reg_row_set,
+				      pk_part_count, 0, (char *)info,
+				      P4_SPACEINFO);
 			sqlVdbeAddOp3(v, OP_IteratorOpen, cur_row_set, 0,
 					  reg_row_set);
-			sql_vdbe_set_p4_key_def(pParse, pk_key_def);
 			regPk = ++pParse->nMem;
 		}
 		iRetInit = sqlVdbeAddOp2(v, OP_Integer, 0, regReturn);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: define format of ephemeral spaces
  2021-07-15 14:14 [Tarantool-patches] [PATCH v1 1/1] sql: define format of ephemeral spaces Mergen Imeev via Tarantool-patches
@ 2021-07-23 23:06 ` Vladislav Shpilevoy via Tarantool-patches
  0 siblings, 0 replies; 2+ messages in thread
From: Vladislav Shpilevoy via Tarantool-patches @ 2021-07-23 23:06 UTC (permalink / raw)
  To: imeevma, kyukhin; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 9 comments below.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 790ca7f70..f5d0d6ce6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -298,7 +298,8 @@ tarantoolsqlCount(struct BtCursor *pCur)
>  }
>  
>  struct space *
> -sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> +sql_ephemeral_space_create_key_info(uint32_t field_count,
> +				    struct sql_key_info *key_info)

1. The function creates not key_info. It creates a space by key_info.
Maybe call it sql_ephemeral_space_new_from_key_info()?

>  {
>  	struct key_def *def = NULL;
>  	uint32_t part_count = field_count;
> @@ -407,6 +408,122 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
>  	return ephemer_new_space;
>  }
>  
> +enum {
> +	/*
> +	 * Name of the fields will be "_COLUMN_1", "_COLUMN_2" and so on. Due to
> +	 * this, length of each name is no more than strlen("_COLUMN_") plus
> +	 * length of UINT32_MAX turned to string, which is 10 and plus 1 for \0.
> +	 */
> +	NAME_LEN = 19,
> +};
> +
> +static struct space_def *
> +sql_space_def_new_ephemeral(uint32_t field_count, enum field_type *types,
> +			    uint32_t *coll_ids)

2. The last 2 arguments can be made const. The same in the other functions
below.

> +{
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
> +	uint32_t size = field_count * sizeof(struct field_def);
> +	struct field_def *fields = region_aligned_alloc(region, size,
> +							alignof(fields[0]));

3. There is region_alloc_array(). The same in the next function.

> +	if (fields == NULL) {
> +		diag_set(OutOfMemory, size, "region_aligned_alloc", "fields");
> +		return NULL;
> +	}
> +	char *names = region_alloc(region, field_count * NAME_LEN);
> +	if (names == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "names");> +		return NULL;
> +	}
> +	for (uint32_t i = 0; i < field_count; ++i) {
> +		struct field_def *field = &fields[i];
> +		field->name = &names[i * NAME_LEN];
> +		sprintf(field->name, "_COLUMN_%d", i);
> +		field->is_nullable = true;
> +		field->nullable_action = ON_CONFLICT_ACTION_NONE;
> +		field->default_value = NULL;
> +		field->default_value_expr = NULL;
> +		field->type = types[i];
> +		field->coll_id = coll_ids[i];
> +	}
> +	struct space_def *def = space_def_new_ephemeral(field_count, fields);
> +	region_truncate(region, svp);
> +	return def;
> +}
> +
> +static struct index_def *
> +sql_index_def_new_ephemeral(uint32_t field_count, enum field_type *types,
> +			    uint32_t *coll_ids, bool is_pk_rowid)
> +{
> +	uint32_t part_count = is_pk_rowid ? 1 : field_count;
> +	struct region *region = &fiber()->gc;
> +	size_t svp = region_used(region);
> +	uint32_t size = part_count * sizeof(struct key_part_def);
> +	struct key_part_def *parts = region_aligned_alloc(region, size,
> +							  alignof(parts[0]));
> +	if (parts == NULL) {
> +		diag_set(OutOfMemory, size, "region_aligned_alloc", "parts");
> +		return NULL;
> +	}
> +	if (is_pk_rowid) {
> +		uint32_t j = field_count - 1;
> +		parts[0].fieldno = j;
> +		parts[0].nullable_action = ON_CONFLICT_ACTION_NONE;
> +		parts[0].is_nullable = true;
> +		parts[0].exclude_null = false;
> +		parts[0].sort_order = SORT_ORDER_ASC;
> +		parts[0].path = NULL;
> +		parts[0].type = types[j];
> +		parts[0].coll_id = coll_ids[j];
> +	} else {
> +		for (uint32_t i = 0; i < part_count; ++i) {
> +			struct key_part_def *part = &parts[i];
> +			part->fieldno = i;
> +			part->nullable_action = ON_CONFLICT_ACTION_NONE;
> +			part->is_nullable = true;
> +			part->exclude_null = false;
> +			part->sort_order = SORT_ORDER_ASC;
> +			part->path = NULL;
> +			part->type = types[i];
> +			part->coll_id = coll_ids[i];
> +		}
> +	}
> +	struct key_def *key_def = key_def_new(parts, part_count, false);
> +	if (key_def == NULL)
> +		return NULL;
> +	const char *name = "ephemer_idx";
> +	struct index_def *def = index_def_new(0, 0, name, strlen(name), TREE,
> +					      &index_opts_default, key_def,
> +					      NULL);
> +	key_def_delete(key_def);
> +	region_truncate(region, svp);
> +	return def;
> +}
> +
> +struct space *
> +sql_ephemeral_space_create(uint32_t field_count, enum field_type *types,
> +			   uint32_t *coll_ids, bool is_pk_rowid)

4. It is strange how there are functions

	sql_space_def_new_ephemeral
	sql_index_def_new_ephemeral

and at the same time these:

	sql_ephemeral_space_create_key_info
	sql_ephemeral_space_create

First ones use ephemeral in the end, the last ones in the beginning.
I would propose to unify them while you are here. One of the
options:

	sql_ephemeral_space_def_new
	sql_ephemeral_index_def_new
	sql_ephemeral_space_new_from_key_info
	sql_ephemeral_space_new

> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 62a726fdd..b35545296 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -249,22 +254,20 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>  			 * account that id field as well. That's why pk_len
>  			 * has one field more than view format.
>  			 */
> -			pk_len = space->def->field_count + 1;
> -			parse->nMem += pk_len;
> -			sqlVdbeAddOp2(v, OP_OpenTEphemeral, reg_eph,
> -					  pk_len);
> +			uint32_t count = space->def->field_count;
> +			fields_info_from_space_def(info->types, info->coll_ids,
> +						   count, space->def);

5. In all usages of fields_info_from_space_def you pass 2 fields of
field_info. I propose to make it take field_info pointer instead
of these 2 arrays. Also for individual fields I propose to add
field_info_set_field(type, coll_id). So as not to miss anything. Currently
it looks too much boilerplate code.

> +			info->types[count] = FIELD_TYPE_UNSIGNED;
> +			info->coll_ids[count] = COLL_NONE;
>  		} else {
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index b9107fccc..c59d7b4b4 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -99,6 +99,103 @@ struct SortCtx {
>  #define SORTFLAG_UseSorter  0x01	/* Use SorterOpen instead of OpenEphemeral */
>  #define SORTFLAG_DESC 0xF0
>  
> +static inline uint32_t
> +multi_select_coll_seq(struct Parse *parser, struct Select *p, int n);
> +
> +struct fields_info *
> +fields_info_new(uint32_t len)
> +{
> +	uint32_t size_info = sizeof(struct fields_info);
> +	uint32_t size_types = len * sizeof(enum field_type);
> +	uint32_t size = size_info + size_types + len * (sizeof(uint32_t));
> +	char *buf = sqlDbMallocRawNN(sql_get(), size);
> +	struct fields_info *info = (struct fields_info *)buf;
> +	if (info == NULL)
> +		return NULL;
> +	info->key_info = NULL;
> +	buf += size_info;
> +	info->types = (enum field_type *)buf;
> +	buf += size_types;
> +	info->coll_ids = (uint32_t *)buf;
> +	return info;
> +}
> +
> +void
> +fields_info_delete(struct fields_info *info)
> +{
> +	if (info->key_info != NULL)
> +		sql_key_info_unref(info->key_info);
> +	return sqlDbFree(sql_get(), info);

6. You do not need this explicit 'return'.

> +}
> +
> +void
> +fields_info_from_space_def(enum field_type *types, uint32_t *coll_ids,
> +			   uint32_t count, struct space_def *def)

7. def can be made const. The same for some other arguments below.

> +{
> +	for (uint32_t i = 0; i < count; ++i) {
> +		types[i] = def->fields[i].type;
> +		coll_ids[i] = def->fields[i].coll_id;
> +	}
> +}
> +
> +void
> +fields_info_from_index_def(enum field_type *types, uint32_t *coll_ids,
> +			   uint32_t count, struct index_def *def)
> +{
> +	for (uint32_t i = 0; i < count; ++i) {
> +		types[i] = def->key_def->parts[i].type;
> +		coll_ids[i] = def->key_def->parts[i].coll_id;
> +	}
> +}
> +
> +static int
> +fields_info_from_expr_list(enum field_type *types, uint32_t *coll_ids,
> +			   struct Parse *parser, struct ExprList *list)
> +{
> +	for (int i = 0; i < list->nExpr; ++i) {
> +		bool b;
> +		struct coll *coll;
> +		struct Expr *expr = list->a[i].pExpr;
> +		types[i] = sql_expr_type(expr);
> +		if (types[i] == FIELD_TYPE_ANY)
> +			types[i] = FIELD_TYPE_SCALAR;
> +		if (sql_expr_coll(parser, expr, &b, &coll_ids[i], &coll) != 0)
> +			return -1;
> +	}
> +	return 0;
> +}
> +
> +static int
> +fields_info_from_order_by(enum field_type *types, uint32_t *coll_ids,
> +			  struct Parse *parser, struct Select *select,
> +			  struct ExprList *order_by)
> +{
> +	for (int i = 0; i < order_by->nExpr; ++i) {
> +		bool b;
> +		struct Expr *expr = order_by->a[i].pExpr;
> +		types[i] = sql_expr_type(expr);
> +		if (types[i] == FIELD_TYPE_ANY)
> +			types[i] = FIELD_TYPE_SCALAR;
> +		uint32_t id = COLL_NONE;
> +		if ((expr->flags & EP_Collate) != 0) {
> +			struct coll *coll;
> +			if (sql_expr_coll(parser, expr, &b, &id, &coll) != 0)
> +				return -1;
> +		} else {
> +			uint32_t fieldno = order_by->a[i].u.x.iOrderByCol - 1;
> +			id = multi_select_coll_seq(parser, select, fieldno);
> +			if (id != COLL_NONE) {
> +				const char *name = coll_by_id(id)->name;
> +				order_by->a[i].pExpr =
> +					sqlExprAddCollateString(parser, expr,
> +								name);
> +			}
> +		}
> +		coll_ids[i] = id;
> +	}
> +	return 0;
> +}

8. It sligtly cuts the ears that a struct called fields_info also
defines a key via key_info. It is like specifying indexes inside of
space format.

Maybe it is worth renaming it to ephemeral_space_def? Or space_info?
Or shorten ephemeral to ephmen in all the new code and use
ephem_space_def? Or sql_space_info? sql_ephemeral_space_def?

And make sql_ephemeral_space_create() take this struct instead of
2 arrays. Besides, you could hide sql_ephemeral_space_create_key_info()
inside of it because you could check key_info != null in
sql_ephemeral_space_create(). It just does not seem like vdbe business
to check these things.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index ef8dcd693..167e80057 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2248,6 +2248,11 @@ struct Parse {
>  #define OPFLAG_SYSTEMSP      0x20	/* OP_Open**: set if space pointer
>  					 * points to system space.
>  					 */
> +/**
> + * If this flag is set, than in OP_OpenTEphemeral we should use rowid as the

9. than -> then.

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-07-23 23:06 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 14:14 [Tarantool-patches] [PATCH v1 1/1] sql: define format of ephemeral spaces Mergen Imeev via Tarantool-patches
2021-07-23 23:06 ` Vladislav Shpilevoy via Tarantool-patches

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git