[tarantool-patches] Re: [PATCH v5 3/3] sql: space_def* instead of Table* in Expr

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 1. Please, do not use SQLITE prefix in Tarantool. Lets name it with
> AFFINITY prefix instead of SQLITE_AFF.
 enum affinity_type {
-       SQLITE_AFF_UNDEFINED = 0,
-       SQLITE_AFF_BLOB = 'A',
-       SQLITE_AFF_TEXT = 'B',
-       SQLITE_AFF_NUMERIC = 'C',
-       SQLITE_AFF_INTEGER = 'D',
-       SQLITE_AFF_REAL = 'E',
+       AFFINITY_UNDEFINED = 0,
+       AFFINITY_BLOB = 'A',
+       AFFINITY_TEXT = 'B',
+       AFFINITY_NUMERIC = 'C',
+       AFFINITY_INTEGER = 'D',
+       AFFINITY_REAL = 'E',
 };


> 2. Fix the comment as well> 3. Now this function is useless. You always can get the collation from
> coll_by_id(def->fields[column].coll_id).
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index f2d325c..33815b8 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1032,7 +1032,7 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 			assert(pIdx->nColumn == 1);
 			if (pIdx->aiColumn[0] == i) {
 				pIdx->coll_array[0] =
-					sql_column_collation(p->def, i);
+					coll_by_id(p->def->fields[i].coll_id);
 			}
 		}
 	}
@@ -1040,39 +1040,6 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 }
 
 /**
- * Return collation of given column from space_def	.
- *
- * @param table Table which is used to fetch column.
- * @param column Number of column.
- * @retval Pointer to collation.
- */
-struct coll *
-sql_column_collation(struct space_def *def, uint32_t column)
-{
-	assert(def != NULL);
-	uint32_t space_id = def->id;
-	struct space *space = space_by_id(space_id);
-	/*
-	 * It is not always possible to fetch collation directly
-	 * from struct space. To be more precise when:
-	 * 1. space is ephemeral. Thus, its id is zero and
-	 *    it can't be found in space cache.
-	 * 2. space is a view. Hence, it lacks any functional
-	 *    parts such as indexes or fields.
-	 * 3. space is under construction. So, the same as p.1
-	 *    it can't be found in space cache.
-	 * In cases mentioned above collation is fetched from
-	 * SQL specific structures.
-	 */
-	if (space == NULL || space_index(space, 0) == NULL) {
-		assert(column < (uint32_t)def->field_count);
-		return coll_by_id(def->fields[column].coll_id);
-	}
-
-	return space->format->fields[column].coll;
-}
-
-/**
  * Return name of given column collation from index.
  *
  * @param idx Index which is used to fetch column.
@@ -3107,7 +3074,7 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
 				goto exit_create_index;
 			}
 		} else if (j >= 0) {
-			coll = sql_column_collation(pTab->def, j);
+			coll = coll_by_id(pTab->def->fields[j].coll_id);
 		} else {
 			coll = NULL;
 		}
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 51c50e0..59f239d 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -34,6 +34,7 @@
  * for generating VDBE code that evaluates expressions in SQLite.
  */
 #include <box/coll.h>
+#include "box/coll_cache.h"
 #include "sqliteInt.h"
 #include "box/session.h"
 
@@ -187,7 +188,8 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found)
 			 */
 			int j = p->iColumn;
 			if (j >= 0) {
-				coll = sql_column_collation(p->space_def, j);
+				coll = coll_by_id(
+					p->space_def->fields[j].coll_id);
 				*is_found = true;
 			}
 			break;
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 7c83924..c7b1cda 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -34,6 +34,7 @@
  * support to compiled SQL statements.
  */
 #include <box/coll.h>
+#include "box/coll_cache.h"
 #include "sqliteInt.h"
 #include "box/session.h"
 #include "tarantoolInt.h"
@@ -298,8 +299,8 @@ sqlite3FkLocateIndex(Parse * pParse,	/* Parse context to store any error in */
 					 * unusable. Bail out early in this case.
 					 */
 					struct coll *def_coll;
-					def_coll = sql_column_collation(pParent->def,
-									iCol);
+					def_coll = coll_by_id(
+						pParent->def->fields[iCol].coll_id);
 					struct coll *coll;
 					coll = sql_index_collation(pIdx, i);
 					if (def_coll != coll)
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b388735..710a388 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -37,6 +37,7 @@
 #include "tarantoolInt.h"
 #include "box/session.h"
 #include "box/schema.h"
+#include "box/coll_cache.h"
 #include "bit/bit.h"
 
 /*
@@ -1809,8 +1810,8 @@ xferOptimization(Parse * pParse,	/* Parser context */
 		 * Collating sequence must be the same on all
 		 * columns.
 		 */
-		if (sql_column_collation(pDest->def, i) !=
-		    sql_column_collation(pSrc->def, i))
+		if (coll_by_id(pDest->def->fields[i].coll_id) !=
+			coll_by_id(pSrc->def->fields[i].coll_id))
 			return 0;
 		/* The tab2 must be NOT NULL if tab1 is */
 		if (!space_def_column_is_nullable(pDest->def, i)
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 02489b7..7cca3d5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3499,8 +3499,6 @@ void sqlite3AddCollateType(Parse *, Token *);
 
 const char *
 column_collation_name(Table *, uint32_t);
-struct coll *
-sql_column_collation(struct space_def *, uint32_t);
 const char *
 index_collation_name(Index *, uint32_t);
 struct coll *

> 4. Lets do not inline affinity values: use AFFINITY_INTEGER.
@@ -3184,7 +3184,7 @@ sqlite3ExprCodeIN(Parse * pParse,	/* Parsing and code generating context */
-		    && affinity == 'D'
+		    && affinity == AFFINITY_INTEGER

@@ -1303,7 +1303,8 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
-				if (pTab->zColAff[pIdx->aiColumn[0]] == 'D') {
+				if (pTab->zColAff[pIdx->aiColumn[0]] ==
+				    AFFINITY_INTEGER) {

@@ -1392,7 +1392,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
-		if (nPkCol == 1 && affinity == 'D') {
+		if (nPkCol == 1 && affinity == AFFINITY_INTEGER) {


> 8. For flags use 'is_' prefix, please. I know, it is not your code, but if
> you modify it, then lets do it correctly.@@ -373,7 +373,7 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
-					bool nullable =
+					bool is_nullable =


> 9. Please, use /** prefix for comments out of function body.
> 10. Same.
-		/* Pointer to space definition. */
+		/** Pointer to space definition. */

-	/* Pointer for table relative definition. */
+	/** Pointer for table relative definition. */


> 12. Perfect. Now this function is useless. It can be inlined and removed.
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 9057301..9a8f045 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2139,8 +2139,7 @@ sqlite3ExprCanBeNull(const Expr * p)
 		assert(p->space_def != NULL);
 		return ExprHasProperty(p, EP_CanBeNull) ||
 		       (p->iColumn >= 0
-		        && space_def_column_is_nullable(p->space_def,
-							p->iColumn));
+		        && p->space_def->fields[p->iColumn].is_nullable);
 	default:
 		return 1;
 	}
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 71567cf..1a883e2 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1120,7 +1120,7 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 			/* Don't bother checking for NOT NULL on columns that do not change */
 			continue;
 		}
-		if (space_def_column_is_nullable(pTab->def, i)
+		if (pTab->def->fields[i].is_nullable
 		    || (pTab->tabFlags & TF_Autoincrement
 			&& pTab->iAutoIncPKey == i))
 			continue;	/* This column is allowed to be NULL */
@@ -1815,8 +1815,8 @@ xferOptimization(Parse * pParse,	/* Parser context */
 			coll_by_id(pSrc->def->fields[i].coll_id))
 			return 0;
 		/* The tab2 must be NOT NULL if tab1 is */
-		if (!space_def_column_is_nullable(pDest->def, i)
-		    && space_def_column_is_nullable(pSrc->def, i))
+		if (!pDest->def->fields[i].is_nullable
+		    && pSrc->def->fields[i].is_nullable)
 			return 0;
 		/* Default values for second and subsequent columns need to match. */
 		if (i > 0) {
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 4d7b834..c5ae5cb 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -374,8 +374,8 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 						}
 					}
 					bool is_nullable =
-						space_def_column_is_nullable(
-							pTab->def, i);
+						pTab->def->fields[i].
+							is_nullable;
 					uint32_t space_id =
 						SQLITE_PAGENO_TO_SPACEID(
 							pTab->tnum);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 55a1a87..75699ae 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4117,9 +4117,6 @@ extern int sqlite3InitDatabase(sqlite3 * db);
 enum on_conflict_action
 table_column_nullable_action(struct Table *tab, uint32_t column);
 
-bool
-space_def_column_is_nullable(struct space_def *def, uint32_t column);
-
 /**
  * Initialize a new parser object.
  * @param parser object to initialize.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 807e017..4e3b236 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -4712,22 +4712,3 @@ table_column_nullable_action(struct Table *tab, uint32_t column)
 
 	return field.nullable_action;
 }
-
-/**
- * Return nullable flag value of given column in given table.
- * FIXME: This is implemented in expensive way. For each invocation table lookup
- * is performed. In future, first param will be replaced with pointer to struct
- * space.
- *
- * @param tab  pointer to the table
- * @param column column number for which action to be returned
- * @return return nullability flag value
- */
-bool
-space_def_column_is_nullable(struct space_def *def, uint32_t column)
-{
-	assert(def->fields[column].is_nullable ==
-	       nullable_action_is_nullable(
-		       def->fields[column].nullable_action));
-	return def->fields[column].is_nullable;
-}
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 8e77e3e..f3a10c0 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -493,7 +493,7 @@ indexColumnNotNull(Index * pIdx, int iCol)
 	assert(iCol >= 0 && iCol < (int)index_column_count(pIdx));
 	j = pIdx->aiColumn[iCol];
 	if (j >= 0) {
-		return !space_def_column_is_nullable(pIdx->pTable->def, j);
+		return !pIdx->pTable->def->fields[j].is_nullable;
 	} else if (j == (-1)) {
 		return 1;
 	} else {
@@ -3333,8 +3333,8 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo,	/* The WHERE clause */
 				if (isOrderDistinct
 				    && iColumn >= 0
 				    && j >= pLoop->nEq
-				    && space_def_column_is_nullable(
-					pIndex->pTable->def, iColumn)) {
+				    && pIndex->pTable->def->fields[iColumn].
+					is_nullable) {
 					isOrderDistinct = 0;
 				}
 
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 0bc6b39..a3db23b 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1261,8 +1261,7 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 			 *      NULL, ... NULL, min_value, ...
 			 */
 			if ((j >= 0 &&
-				space_def_column_is_nullable(pIdx->pTable->def,
-							     j)) ||
+				pIdx->pTable->def->fields[j].is_nullable) ||
 			    j == XN_EXPR) {
 				assert(pLoop->nSkip == 0);
 				bSeekPastNull = 1;
@@ -1308,9 +1307,8 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 #endif
 			if (pRangeStart == 0) {
 				j = pIdx->aiColumn[nEq];
-				if ((j >= 0
-				     && space_def_column_is_nullable(
-					pIdx->pTable->def, j)) ||
+				if ((j >= 0 &&
+				     pIdx->pTable->def->fields[j].is_nullable) ||
 				    j == XN_EXPR) {
 					bSeekPastNull = 1;
 				}