[tarantool-patches] Re: [PATCH] sql: use collation pointers instead of names

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. See below 6 comments.

1. Looks that you did not pushed this patch, because on the
branch I see 3 commits:
[WIP] sql: use collation pointers instead of names
sql: RAW1
sql: RAW2

git diff head~3 --stat shows not the same as the info in this
letter:
18 files changed, 270 insertions(+), 295 deletions(-)
instead of
18 files changed, 272 insertions(+), 295 deletions(-)

Please, push the latest version.

2. Please, specify links to branch and issue after '---' in
the letter.

On 13/04/2018 11:05, Kirill Yukhin wrote:
> Before the change SQL FE used collation names to refer to
> collations. Main data structures were using names as well.
> The patch fixes that and uses explicit pointers to Tarantool's
> `struct coll` during code gen and inside data structures.
> 
> Closes #3205
> ---
>   src/box/sql.c           |  21 ++------
>   src/box/sql/alter.c     |   2 +-
>   src/box/sql/analyze.c   |   9 +---
>   src/box/sql/build.c     | 133 ++++++++++++++++++++++--------------------------
>   src/box/sql/callback.c  |  53 ++-----------------
>   src/box/sql/expr.c      |  54 +++++++++++---------
>   src/box/sql/fkey.c      |  23 +++++----
>   src/box/sql/func.c      |   3 +-
>   src/box/sql/insert.c    |  12 ++---
>   src/box/sql/pragma.c    |   9 +++-
>   src/box/sql/select.c    |  84 +++++++++++++++++-------------
>   src/box/sql/sqliteInt.h |  22 +++++---
>   src/box/sql/vdbe.c      |   2 +-
>   src/box/sql/vdbeaux.c   |   7 +++
>   src/box/sql/vdbesort.c  |   4 +-
>   src/box/sql/where.c     |  98 ++++++++++++++++-------------------
>   src/box/sql/whereInt.h  |   7 ++-
>   src/box/sql/whereexpr.c |  24 ++++++---
>   18 files changed, 272 insertions(+), 295 deletions(-)
> 
> @@ -952,6 +951,7 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
>   	Table *p;
>   	int i;
>   	char *zColl;		/* Dequoted name of collation sequence */
> +	struct coll *coll;
>   	sqlite3 *db;
>   
>   	if ((p = pParse->pNewTable) == 0)
> @@ -962,10 +962,10 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
>   	if (!zColl)
>   		return;
>   
> -	if (sqlite3LocateCollSeq(pParse, db, zColl)) {
> +	coll =  sqlite3LocateCollSeq(pParse, db, zColl);
> +	if (coll) {

3. Please, use explicit != NULL.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 59662cf..35c9dae 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1878,10 +1878,13 @@ struct Column {
>   	char *zName;		/* Name of this column */
>   	enum field_type type;	/* Column type. */
>   	Expr *pDflt;		/* Default value of this column */
> -	char *zColl;		/* Collating sequence.  If NULL, use the default */
> -	enum on_conflict_action notNull;  /* An ON_CONFLICT_ACTION code for
> -					   * handling a NOT NULL constraint
> -					   */
> +	/** Collating sequence. */
> +	struct coll *coll;
> +	/**
> +	 * An ON_CONFLICT_ACTION code for handling a NOT NULL
> +	 * constraint.
> +	 */
> +	enum on_conflict_action notNull;
>   	char affinity;		/* One of the SQLITE_AFF_... values */
>   	u8 szEst;		/* Estimated size of value in this column. sizeof(INT)==1 */
>   	u8 is_primkey;		/* Boolean propertie for being PK */
> @@ -2148,7 +2151,8 @@ struct Index {
>   	Index *pNext;		/* The next index associated with the same table */
>   	Schema *pSchema;	/* Schema containing this index */
>   	u8 *aSortOrder;		/* for each column: True==DESC, False==ASC */
> -	const char **azColl;	/* Array of collation sequence names for index */
> +	/**  Array of collation sequences for index. */
> +	struct coll **coll_array;
>   	Expr *pPartIdxWhere;	/* WHERE clause for partial indices */
>   	ExprList *aColExpr;	/* Column expressions */
>   	int tnum;		/* DB Page containing root of this index */
> @@ -3548,14 +3552,20 @@ void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, int);
>   void sqlite3AddCheckConstraint(Parse *, Expr *);
>   void sqlite3AddDefaultValue(Parse *, ExprSpan *);
>   void sqlite3AddCollateType(Parse *, Token *);
> + >   const char *
>   column_collation_name(Table *, uint32_t);
> +struct coll *
> +sql_column_collation(Table *, uint32_t);
>   const char *
>   index_collation_name(Index *, uint32_t);
>   struct coll *
> +sql_index_collation(Index *idx, uint32_t column);
> +struct coll *
>   sql_default_coll();
>   bool
>   space_is_view(Table *);
> +
>   void sqlite3EndTable(Parse *, Token *, Token *, Select *);
>   int
>   emit_open_cursor(Parse *, int, int);
> @@ -3845,7 +3855,7 @@ const char *sqlite3ErrName(int);
>   const char *sqlite3ErrStr(int);
>   struct coll *sqlite3FindCollSeq(const char *);
>   struct coll *sqlite3LocateCollSeq(Parse * pParse, sqlite3 * db, const char *zName);
> -struct coll *sqlite3ExprCollSeq(Parse * pParse, Expr * pExpr);
> +struct coll *sqlite3ExprCollSeq(Parse * pParse, Expr * pExpr, bool *found);

4. Please, move a comment into the header, and describe parameters. In
particular 'found' one.

> diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h
> index 381a1d2..b85cd73 100644
> --- a/src/box/sql/whereInt.h
> +++ b/src/box/sql/whereInt.h
> @@ -293,7 +293,12 @@ struct WhereTerm {
>   struct WhereScan {
>   	WhereClause *pOrigWC;	/* Original, innermost WhereClause */
>   	WhereClause *pWC;	/* WhereClause currently being scanned */
> -	const char *zCollName;	/* Required collating sequence, if not NULL */
> +	/** Required collating sequence. */
> +	struct coll *coll;
> +	/** Explicitly specified BINARY collation. */
> +	bool cool_binary;

5. is_col_binary.

> +	/** Flag is set if actual column was encountered. */
> +	bool column_seen;

6. is_column_seen.