[patches] [PATCH 0/7] iproto: send SQL column meta on SELECT

[n.pettik]

Hello. See few minor comments below.
(I don’t want to spam, so I put all comments together in one letter)

[PATCH 1/7] collation: introduce coll_is_case_sensitive function

>diff --git a/src/box/coll.h b/src/box/coll.h
>index aa0e4c01a..f171e582c 100644
>--- a/src/box/coll.h
>+++ b/src/box/coll.h
>@@ -78,6 +78,14 @@ 
>+/**
>+ * Return true, if a collation is case sensitive.
>+ * @param coll Collation to check.
>+ * @retval Case sensitivity.
>+ */
>+bool
>+coll_is_case_sensitive(const struct coll *coll);

Not sure that this comment is really needed. I mean, function's name completely
describes what it does.

///////////////////////////////////////////////////////////////////////

[PATCH 2/7] sql: remove SQLITE_ENABLE_COLUMN_METADATA and _OMIT_DECLTYPE

>/*
>- * Convert the N-th element of pStmt->pColName[] into a string using
>- * xFunc() then return that string.  If N is out of range, return 0.
>+ * Get the N-th element of pStmt->pColName[]. If N is out of
>+ * range, return NULL.
> *
> * There are up to 5 names for each column.  useType determines which
> * name is returned.  Here are the names:
> *
> *    0      The column name as it should be displayed for output
>- *    1      The datatype name for the column
>- *    2      The name of the database that the column derives from
>- *    3      The name of the table that the column derives from
>- *    4      The name of the table column that the result column derives from
>+ *    1      The name of the table that the column derives from
>+ *    2      The name of the table column that the result column derives from
> *
> * If the result is not a simple column reference (if it is an expression
>- * or a constant) then useTypes 2, 3, and 4 return NULL.
>+ * or a constant) then useTypes 3 and 4 return NULL.
> */
>-static const void *
>-columnName(sqlite3_stmt * pStmt,
>-	   int N, const void *(*xFunc) (Mem *), int useType)
>+static const char *
>+columnName(sqlite3_stmt *pStmt, int N, int useType)
>{
>-	const void *ret;
>+	const char *ret;
>	Vdbe *p;
>	int n;
>	sqlite3 *db;
>#ifdef SQLITE_ENABLE_API_ARMOR
>-	if (pStmt == 0) {
>+	if (pStmt == NULL) {
>		(void)SQLITE_MISUSE_BKPT;
>-		return 0;
>+		return NULL;
>	}
>#endif
>-	ret = 0;
>+	ret = NULL;
>	p = (Vdbe *) pStmt;
>	db = p->db;
>-	assert(db != 0);
>+	assert(db != NULL);
>	n = sqlite3_column_count(pStmt);
>	if (N < n && N >= 0) {
>		N += useType * n;
>		sqlite3_mutex_enter(db->mutex);
>		assert(db->mallocFailed == 0);
>-		ret = xFunc(&p->aColName[N]);
>+		ret = (const char *) sqlite3_value_text(&p->aColName[N]);
>		/* A malloc may have failed inside of the xFunc() call. If this
>		 * is the case, clear the mallocFailed flag and return NULL.
>		 */
>		if (db->mallocFailed) {
>			sqlite3OomClear(db);
>-			ret = 0;
>+			ret = NULL;

Here you are patching function which you anyway remove in next commit.
I think it is redundant diff, since these commits are placed in one patch-set.

///////////////////////////////////////////////////////////////////////

[PATCH 5/7] sql: store column meta in a special structure instead of Mem

>diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
>@@ -2370,51 +2371,85 @@ Cleanup(Vdbe * p)
> * be called on an SQL statement before sqlite3_step().
> */
>void
>-sqlite3VdbeSetNumCols(Vdbe * p, int nResColumn)
>+sqlite3VdbeSetNumCols(Vdbe *p, int n)
>{
>-	int n;
>	sqlite3 *db = p->db;
>-
>-	releaseMemArray(p->aColName, p->nResColumn * COLNAME_N);
>-	sqlite3DbFree(db, p->aColName);
>-	n = nResColumn * COLNAME_N;
>-	p->nResColumn = (u16) nResColumn;
>-	p->aColName = (Mem *) sqlite3DbMallocRawNN(db, sizeof(Mem) * n);
>-	if (p->aColName == 0)
>-		return;
>-	initMemArray(p->aColName, n, p->db, MEM_Null);
>+	struct sql_column_meta *col = p->columns;
>+	for (uint32_t i = 0; i < p->nResColumn; ++i, ++col) {
>+		if (col->need_free_alias)
>+			sqlite3DbFree(db, col->alias);
>+		sqlite3DbFree(db, col->name);
>+		col->alias = NULL;
>+		col->name = NULL;
>+	}
>+	if (n == 0) {
>+		sqlite3DbFree(db, p->columns);
>+	} else {
>+		uint32_t size = sizeof(p->columns[0]) * n;
>+		struct sql_column_meta *new_meta = (struct sql_column_meta *)
>+			sqlite3DbRealloc(db, p->columns, size);
>+		if (new_meta == NULL)
>+			return;

I suggest to indicate somehow that sqlite3DbRealloc has failed
(even if it occurs extremely rarely).
For instace, by setting sqlite3Error().

>diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
>index d7546ab43..9d13a6449 100644
>--- a/src/box/sql/vdbeaux.c
>+++ b/src/box/sql/vdbeaux.c
>@@ -2370,51 +2371,85 @@ Cleanup(Vdbe * p)
>+sqlite3VdbeSetColMeta(Vdbe *p, int idx,
>+	struct sql_column_meta *meta = &p->columns[idx];
>+	if (table != NULL) {
>+		assert(fieldno < (uint32_t) table->nCol);
>+		const struct Column *column = &table->aCol[fieldno];
>+		meta->name = sqlite3DbStrDup(p->db, column->zName);
>+		meta->is_nullable = column->notNull == ON_CONFLICT_ACTION_NONE;
>+		meta->is_primary_part =
>+			(column->colFlags & COLFLAG_PRIMKEY) != 0;
>+		meta->is_autoincrement =
>+			meta->is_primary_part &&
>+			(table->tabFlags & TF_Autoincrement) != 0;
>+		if (column->zColl != NULL) {
>+			struct coll *coll = coll_by_name(column->zColl,
>+							 strlen(column->zColl));
>+			if (coll != NULL) {
>+				meta->is_case_sensitive =
>+					coll_is_case_sensitive(coll);
>+			} else {
>+				meta->is_case_sensitive = true;
>+			}
>+		} else {

I would say it is too gently to silently skip situation when collation is missed.
I can't imagine reason when collation isn't found by name, since AFAIK in SQL every
column features collation (even if it wasn't set, by default it would be "BINARY").
And why do you set case sensitivity to be true if there is no found collation?
Yep, there are only two variants what to do (case/nocase), but anyway
this moment deserves to be mentioned/commented somewhere.

>+		if (column->zName == alias) {
>+			/*
>+			 * If a column is selected with no alias,
>+			 * then do not copy it twice. Store two
>+			 * pointers to the same name.
>+			 */
>+			destructor = SQLITE_STATIC;
>+			alias = meta->name;

I suppose it should be strcmp() instead of == ?

>@@ -1695,51 +1697,23 @@ columnType(NameContext *pNC, Expr *pExpr, const char **pzOrigTab,
>-	if (pzOrigTab) {
>-		assert(pzOrigTab && pzOrigCol);
>-		*pzOrigTab = zOrigTab;
>-		*pzOrigCol = zOrigCol;
>+	if (table != NULL) {
>+		assert(table != NULL && fieldno != NULL);

LHS condition in assert is redundant: if you get in this branch, table is already != NULL.

///////////////////////////////////////////////////////////////////////

[PATCH 6/7] iproto: send SQL column meta on SELECT

Also, I agree with Kirill that function’s name ‘sql_ephemeral_column_meta_encode’ can be confusing.
I can suggest synonyms: surrogate, pseudo, dummy, synthetic/artificial.