[tarantool-patches] Re: [PATCH v6 2/4] sql: remove SQL fields from Table and Column

["n.pettik" <korablev@tarantool.org>]

Disclaimer:
Most of comments don’t seem to be vital.
You may skip them, if you disagree with them.
=======================================================================

> 1. Removed zName, is_nullable, collation, type
> from SQL Column.
> 2. Removed zColumns, zName from SQL Table.
> 3. Refactored Parser to use def_expression directly.
> 4. Introduced is_view flag.

Where did you introduce this flag? AFAIR It has already been introduced
in space opts several patches ago.

> 4. Introduced sql_table_def_rebuild intended for collect
> fragmented with sql_field_retrieve space_def into memory
> located in one allocation.

Duplicate: in fact, it is fifth point.

> +static inline bool
> +nullable_action_is_nullable(enum on_conflict_action nullable_action)

Sounds confusing to me. Mb better call it just ‘action_is_nullable’?
It is just a suggestion.

> @@ -116,10 +104,10 @@ space_def_dup(const struct space_def *src)
> 			if (src->fields[i].default_value != NULL) {
> 				ret->fields[i].default_value = strs_pos;
> 				strs_pos += strlen(strs_pos) + 1;
> -
> -				struct Expr *e =
> -					src->fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				src->fields[i].default_value_expr;

You don’t need carrying this line: it fits into one.

> +			if (e != NULL) {
> 				char *expr_pos_old = expr_pos;
> 				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> 				assert(e != NULL);
> @@ -201,10 +189,10 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
> 				       fields[i].default_value, len);
> 				def->fields[i].default_value[len] = 0;
> 				strs_pos += len + 1;
> -
> -				struct Expr *e =
> -					fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				fields[i].default_value_expr;

Either this.

> @@ -1441,47 +1443,53 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
> 	 * treat it as strict type, not affinity.  */
> 	if (pk_idx && pk_idx->nColumn == 1) {
> 		int pk = pk_idx->aiColumn[0];
> -		if (pTable->aCol[pk].type == FIELD_TYPE_INTEGER)
> +		if (def->fields[pk].type == FIELD_TYPE_INTEGER)
> 			pk_forced_int = pk;
> 	}
> 
> 	for (i = 0; i < n; i++) {
> 		const char *t;
> -		struct coll *coll = aCol[i].coll;
> -		struct field_def *field = &pTable->def->fields[i];
> -		struct Expr *def = field->default_value_expr;
> +		struct coll *coll =
> +			coll_by_id(pTable->def->fields[i].coll_id);

And this one.

> +
> +		struct field_def *field = &def->fields[i];
> +		const char *zToken = field->default_value;

Lest use Tarantool naming conventions, i.e. don’t use Hungarian notation.

> 		int base_len = 4;
> 		if (coll != NULL)
> 			base_len += 1;
> -		if (def != NULL)
> +		if (zToken != NULL)
> 			base_len += 1;
> 		p = enc->encode_map(p, base_len);
> 		p = enc->encode_str(p, "name", 4);
> -		p = enc->encode_str(p, aCol[i].zName, strlen(aCol[i].zName));
> +		p = enc->encode_str(p, field->name, strlen(field->name));
> 		p = enc->encode_str(p, "type", 4);
> +
> +		assert(def->fields[i].is_nullable ==
> +			       nullable_action_is_nullable(
> +				       def->fields[i].nullable_action));

Here smth wrong with indentations (and in other places where you use this assertion).

> @@ -1496,13 +1504,12 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>  */
> int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
> {
> -	(void)pTable;
> 	const struct Enc *enc = get_enc(buf);
> 	char *base = buf, *p;
> 
> 	bool is_view = false;
> 	if (pTable != NULL)
> -		is_view = pTable->pSelect != NULL;
> +		is_view = pTable->def->opts.is_view;
> 	p = enc->encode_map(base, is_view ? 2 : 1);
> 	p = enc->encode_str(p, "sql", 3);
> 	p = enc->encode_str(p, zSql, strlen(zSql));
> @@ -1523,6 +1530,9 @@ int tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, void *buf)
> int tarantoolSqlite3MakeIdxParts(SqliteIndex *pIndex, void *buf)
> {
> 	struct Column *aCol = pIndex->pTable->aCol;
> +	struct space_def *def = pIndex->pTable->def;

In tarantoolSqlite3MakeTableFormat() you declare such var as const.
Lets use everywhere const, or don’t use it at all.

> +
> +struct space_def *
> +sql_ephemeral_space_def_new(Parse *parser, const char *name)

Use ‘struct’ prefix for Parse struct: you did it in declaration, tho. (*)

Please, provide explanation, why you call it ‘ephemeral’.
I mean, this function is used both for real tables and those,
which, for instance, represent result set of selects.
Furthermore, there is some mess between sql_table_new and
sql_ephemeral_table_new(). I strongly recommend you to write
comments in code concerning these points.

> +{
> +	struct space_def *def = NULL;

Why do you need to declare it beforehand?
You can write this way:
struct space_def *def = (struct space_def *)region_alloc(region, size);

> +	struct region *region = &fiber()->gc;
> +	size_t name_len = name != NULL ? strlen(name) : 0;
> +	uint32_t dummy;
> +	size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy);
> +	def = (struct space_def *)region_alloc(region, size);
> +	if (def == NULL) {
> +		diag_set(OutOfMemory, sizeof(struct tuple_dictionary),
> +			"region_alloc", "sql_ephemeral_space_def_new");
> +		parser->rc = SQL_TARANTOOL_ERROR;
> +		parser->nErr++;
> +		return NULL;

You can avoid passing parse context as an argument,
and check return value. Wouldn’t it be better?

> +	}
> +
> +	memset(def, 0, size);
> +	memcpy(def->name, name, name_len);
> +	def->name[name_len] = '\0';
> +	def->opts.temporary = true;
> +	return def;
> +}
> +
> +Table *
> +sql_ephemeral_table_new(Parse *parser, const char *name)

The same as (*).

> diff --git a/src/box/sql.h b/src/box/sql.h
> index db92d80..3c26492 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -65,6 +65,7 @@ sql_get();
> struct Expr;
> struct Parse;
> struct Select;
> +struct Table;
> 
> /**
>  * Perform parsing of provided expression. This is done by
> @@ -143,6 +144,37 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
> void
> sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
> 
> +/**
> + * Create and initialize a new ephemeral SQL Table object.
> + * @param parser SQL Parser object.
> + * @param name Table to create name.

Did you mean ’Name of table to be created’?

> + * @retval NULL on memory allocation error, Parser state changed.
> + * @retval not NULL on success.
> + */

I may already mention, but don’t write comments
'just for comments’. I think it is a good place to explain,
for example, difference between ephemeral (btw, I don’t like this name)
table and ‘ordinary’ one.

> +struct Table *
> +sql_ephemeral_table_new(struct Parse *parser, const char *name);
> +
> +/**
> + * Create and initialize a new ephemeral space_def object.
> + * @param parser SQL Parser object.
> + * @param name Table to create name.

The same.

> @@ -388,17 +370,14 @@ deleteTable(sqlite3 * db, Table * pTable)
> 	/* Delete the Table structure itself.
> 	 */
> 	sqlite3HashClear(&pTable->idxHash);
> -	sqlite3DeleteColumnNames(db, pTable);
> -	sqlite3DbFree(db, pTable->zName);
> +	sqlite3DbFree(db, pTable->aCol);
> 	sqlite3DbFree(db, pTable->zColAff);
> 	sqlite3SelectDelete(db, pTable->pSelect);
> 	sqlite3ExprListDelete(db, pTable->pCheck);
> -	if (pTable->def != NULL) {
> -		/* Fields has been allocated independently. */
> -		struct field_def *fields = pTable->def->fields;
> +	/* Do not delete pTable->def allocated not on region. */
> +	assert(pTable->def != NULL);
> +	if (!pTable->def->opts.temporary)

You would better describe somewhere that you are using
this field as a sign of def allocated on region.
It can be misleading since it is used for ephemeral spaces
which def if allocated on regular malloc.

> @@ -675,42 +646,50 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
> 	sqlite3 *db = pParse->db;
> 	if ((p = pParse->pNewTable) == 0)
> 		return;
> +	assert(p->def->opts.temporary);

The same is here: add comment explaining this assert.

> @@ -1937,7 +1920,9 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
> 		if (pSelect) {
> 			zStmt = createTableStmt(db, p);
> 		} else {
> -			Token *pEnd2 = p->pSelect ? &pParse->sLastToken : pEnd;
> +			assert(p->def->opts.is_view == (p->pSelect != NULL));

Too many same assertions in one function. Do you really need them all?

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 0c86761..119940c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -48,7 +48,7 @@ static int exprCodeVector(Parse * pParse, Expr * p, int *piToFree);
> char
> sqlite3TableColumnAffinity(Table * pTab, int iCol)
> {
> -	assert(iCol < pTab->nCol);
> +	assert(iCol < (int)pTab->def->field_count);
> 	return iCol >= 0 ? pTab->aCol[iCol].affinity : SQLITE_AFF_INTEGER;
> }
> 
> @@ -2233,7 +2233,8 @@ isCandidateForInOpt(Expr * pX)
> 		return 0;	/* FROM is not a subquery or view */
> 	pTab = pSrc->a[0].pTab;
> 	assert(pTab != 0);
> -	assert(pTab->pSelect == 0);	/* FROM clause is not a view */
> +	assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
> +	assert(!pTab->def->opts.is_view);	/* FROM clause is not a view */

Don’t put comment right to code: move it up.

> 
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 2ab8751..f949709 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -204,26 +204,6 @@ sqlite3InitDatabase(sqlite3 * db)
> 	return rc;
> }
> 
> -
> -/*
> - * Free all memory allocations in the pParse object
> - */
> -void
> -sqlite3ParserReset(Parse * pParse)
> -{
> -	if (pParse) {
> -		sqlite3 *db = pParse->db;
> -		sqlite3DbFree(db, pParse->aLabel);
> -		sqlite3ExprListDelete(db, pParse->pConstExpr);
> -		if (db) {
> -			assert(db->lookaside.bDisable >=
> -			       pParse->disableLookaside);
> -			db->lookaside.bDisable -= pParse->disableLookaside;
> -		}
> -		pParse->disableLookaside = 0;
> -	}
> -}

I would mention in commit message about the fact that
now almost within parsing context is allocated on region,
and at the end of parsing region is truncated. Or, at least,
in comments to sql_parser_destroy()/sql_parser_create()

> -
> /*
>  * Compile the UTF-8 encoded SQL statement zSql into a statement handle.
>  */
> @@ -241,9 +221,7 @@ sqlite3Prepare(sqlite3 * db,	/* Database handle. */
> 	int rc = SQLITE_OK;	/* Result code */
> 	int i;			/* Loop counter */
> 	Parse sParse;		/* Parsing context */
> -
> -	memset(&sParse, 0, PARSE_HDR_SZ);
> -	memset(PARSE_TAIL(&sParse), 0, PARSE_TAIL_SZ);
> +	sql_parser_create(&sParse);
> 	sParse.pReprepare = pReprepare;
> 	assert(ppStmt && *ppStmt == 0);
> 	/* assert( !db->mallocFailed ); // not true with SQLITE_USE_ALLOCA */
> @@ -265,6 +243,7 @@ sqlite3Prepare(sqlite3 * db,	/* Database handle. */
> 	 * works even if READ_UNCOMMITTED is set.
> 	 */
> 	sParse.db = db;
> +

Redundant empty line.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5a50413..e08d709 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -318,9 +318,9 @@ sqlite3JoinType(Parse * pParse, Token * pA, Token * pB, Token * pC)
> static int
> columnIndex(Table * pTab, const char *zCol)
> {
> -	int i;
> -	for (i = 0; i < pTab->nCol; i++) {
> -		if (strcmp(pTab->aCol[i].zName, zCol) == 0)
> +	uint32_t i;
> +	for (i = 0; i < pTab->def->field_count; i++) {

You can move definition of i inside ‘for' cycle:
for (uint32_t i = 0; ...)

> @@ -1819,13 +1818,31 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
> 		testcase(aCol == 0);
> 	} else {
> 		nCol = 0;
> -		aCol = 0;
> +		aCol = NULL;
> 	}
> 	assert(nCol == (i16) nCol);
> -	*pnCol = nCol;
> -	*paCol = aCol;
> 
> -	for (i = 0, pCol = aCol; i < nCol && !db->mallocFailed; i++, pCol++) {
> +	/*
> +	 * This should be a table without resolved columns.
> +	 * sqlite3ViewGetColumnNames could use it to resolve
> +	 * names for existing table.
> +	 */
> +	assert(pTable->def->fields == NULL);
> +	struct region *region = &fiber()->gc;
> +	pTable->def->fields =
> +		region_alloc(region, nCol * sizeof(pTable->def->fields[0]));
> +	if (pTable->def->fields == NULL) {
> +		sqlite3OomFault(db);
> +		goto cleanup;
> +	}
> +	memset(pTable->def->fields, 0, nCol * sizeof(pTable->def->fields[0]));
> +	/* NULL nullable_action should math is_nullable flag. */

Math? I can’t understand this comment. Rephrase it pls.

> +	for (int i = 0; i < nCol; i++)
> +		pTable->def->fields[i].is_nullable = true;
> +	pTable->def->field_count = (uint32_t)nCol;
> +	pTable->aCol = aCol;
> +
> +	for (i = 0, pCol = aCol; i < nCol; i++, pCol++) {
> 		/* Get an appropriate name for the column
> 		 */
> 		p = sqlite3ExprSkipCollate(pEList->a[i].pExpr);
> @@ -1845,7 +1862,7 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
> 				pTab = pColExpr->pTab;
> 				if (iCol < 0)
> 					iCol = pTab->iPKey;
> -				zName = pTab->aCol[iCol].zName;
> +				zName = pTab->def->fields[iCol].name;
> 			} else if (pColExpr->op == TK_ID) {
> 				assert(!ExprHasProperty(pColExpr, EP_IntValue));
> 				zName = pColExpr->u.zToken;
> @@ -1874,22 +1891,34 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
> 			if (cnt > 3)
> 				sqlite3_randomness(sizeof(cnt), &cnt);
> 		}
> -		pCol->zName = zName;
> -		if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) {
> +		uint32_t name_len = (uint32_t)strlen(zName);

Why do you declare it as uin32_t and do cast?
region_alloc and memcpy take it as size_t arg, btw.

> @@ -4683,18 +4711,29 @@ selectExpander(Walker * pWalker, Select * p)
> 			assert(pFrom->pTab == 0);
> 			if (sqlite3WalkSelect(pWalker, pSel))
> 				return WRC_Abort;
> +			/*
> +			 * Will be overwritten with pointer as
> +			 * unique identifier.
> +			 */
> +			const char *name = "sqlite_sq_DEADBEAFDEADBEAF”;

Lest don’t use sqlite prefixes. Just call it ‘unused’.

> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index c77aa9b..279c3af 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -661,15 +661,16 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
> 	sprintf(stmt, "%s%s", outer, expr);
> 
> 	struct Parse parser;
> -	memset(&parser, 0, sizeof(parser));
> +	sql_parser_create(&parser);
> 	parser.db = db;
> 	parser.parse_only = true;
> +

Redundant empty line.