[tarantool-patches] Re: [PATCH v1 3/3] sql: get rid of Column structure

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Again: why did this patch trapped in patch-set? All three patches seem to be independent.
You and Vlad sometimes ask me to do some unrelated to patch work; This is the same situation.
And it is also convenient to ask Kirill to merge a big patch set than to noise a lot for each small fix.

> Typo: ‘becomes’.
Yep, fixed.

> Why do you need to move collation ptr to field_def? It already features collation id,
> so you can always get pointer to it by simple lookup. It would make sense if it was
> utilised everywhere. But I see assignment only in sqlite3SelectAddColumnTypeAndCollation()
> and no real usages..Lets remove it at all.
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 444bc48..7b6bd1a 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -407,9 +407,6 @@ field_def_decode(struct field_def *field, const char **data,
 			  tt_sprintf("collation is reasonable only for "
 				     "string, scalar and any fields"));
 	}
-	struct coll_id *collation = coll_by_id(field->coll_id);
-	if (collation != NULL)
-		field->coll = collation->coll;
 
 	const char *dv = field->default_value;
 	if (dv != NULL) {
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 12cc3b2..8dbead6 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -106,7 +106,6 @@ const struct field_def field_def_default = {
 	.is_nullable = false,
 	.nullable_action = ON_CONFLICT_ACTION_DEFAULT,
 	.coll_id = COLL_NONE,
-	.coll = NULL,
 	.default_value = NULL,
 	.default_value_expr = NULL
 };
diff --git a/src/box/field_def.h b/src/box/field_def.h
index c8f158c..05f80d4 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -123,8 +123,6 @@ struct field_def {
 	enum on_conflict_action nullable_action;
 	/** Collation ID for string comparison. */
 	uint32_t coll_id;
-	/** Collating sequence for string comparison. */
-	struct coll *coll;
 	/** 0-terminated SQL expression for DEFAULT value. */
 	char *default_value;
 	/** AST for parsed default value. */
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 6f803cf..2615624 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -992,9 +992,8 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 	char *zColl = sqlite3NameFromToken(db, pToken);
 	if (!zColl)
 		return;
-	uint32_t *id = &p->def->fields[i].coll_id;
-	p->def->fields[i].coll = sql_get_coll_seq(pParse, zColl, id);
-	if (p->def->fields[i].coll != NULL) {
+	uint32_t *coll_id = &p->def->fields[i].coll_id;
+	if (sql_get_coll_seq(pParse, zColl, coll_id) != NULL) {
 		/* If the column is declared as "<name> PRIMARY KEY COLLATE <type>",
 		 * then an index may have been created on this column before the
 		 * collation type was added. Correct this if it is the case.
@@ -1003,9 +1002,8 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
 		     pIdx = pIdx->pNext) {
 			assert(pIdx->def->key_def->part_count == 1);
 			if (pIdx->def->key_def->parts[0].fieldno == i) {
-				id = &pIdx->def->key_def->parts[0].coll_id;
-				pIdx->def->key_def->parts[0].coll =
-					sql_column_collation(p->def, i, id);
+				coll_id = &pIdx->def->key_def->parts[0].coll_id;
+				(void)sql_column_collation(p->def, i, coll_id);
 			}
 		}
 	}
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index d577648..ab91bd0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1962,12 +1962,8 @@ sqlite3SelectAddColumnTypeAndCollation(Parse * pParse,		/* Parsing contexts */
 			affinity = AFFINITY_BLOB;
 		pTab->def->fields[i].affinity = affinity;
 		bool unused;
-		uint32_t id;
-		struct coll *coll = sql_expr_coll(pParse, p, &unused, &id);
-		if (coll != NULL && pTab->def->fields[i].coll == NULL) {
-			pTab->def->fields[i].coll = coll;
-			pTab->def->fields[i].coll_id = id;
-		}
+		(void)sql_expr_coll(pParse, p, &unused,
+				    &pTab->def->fields[i].coll_id);
 	}
 }
 
> Here you are simply fixing changes made in first patch, so mb it is better to
> move them to first patch?
No, I rethink thous changes taking into account new situation. It is better to keep it as is, except you think
to merge this patch with first one. But it seems to be a bad idea.