[tarantool-patches] Re: [PATCH v2 6/7] sql: refactor sql_expr_create to set diag

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

Thanks for the fixes! See 6 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 44b6ce11f..f5b2926c8 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -149,16 +149,17 @@ sqlExprAddCollateToken(Parse * pParse,	/* Parsing context */
>   			   int dequote	/* True to dequote pCollName */
>       )
>   {
> -	if (pCollName->n > 0) {
> -		Expr *pNew =
> -		    sqlExprAlloc(pParse->db, TK_COLLATE, pCollName,
> -				     dequote);
> -		if (pNew) {
> -			pNew->pLeft = pExpr;
> -			pNew->flags |= EP_Collate | EP_Skip;
> -			pExpr = pNew;
> -		}
> +	if (pCollName->n == 0)
> +		return pExpr;
> +	struct Expr *new_expr =
> +		sql_expr_new(pParse->db, TK_COLLATE, pCollName, dequote);
> +	if (new_expr == NULL) {
> +		sql_parser_error(pParse);
> +		return NULL;

1. Now you have a leak - parse.y:935 expects that sqlExprAddCollateToken on
an error returns the old value, that is freed later, but after your patch
the old value is lost.

> @@ -854,113 +855,62 @@ sqlExprSetHeightAndFlags(Parse * pParse, Expr * p)
> -/* Allocate a new expression and initialize it as integer.
> - * @param db sql engine.
> - * @param value Value to initialize by.
> - *
> - * @retval not NULL Allocated and initialized expr.
> - * @retval     NULL Memory error.
> - */
> -Expr *
> -sqlExprInteger(sql * db, int value)
> +struct Expr *
> +sql_op_expr_new(struct sql *db, int op, const char *name)
>   {
> -	Expr *ret = sqlExpr(db, TK_INTEGER, NULL);
> -	if (ret != NULL) {
> -		ret->flags = EP_IntValue;
> -		ret->u.iValue = value;
> -	}
> -	return ret;
> +	struct Token name_token;
> +	name_token.z = name;
> +	name_token.n = name != NULL ? strlen(name) : 0;

2. sqlTokenInit.

> diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
> index d1eb6670a..ceba2db9f 100644
> --- a/src/box/sql/fk_constraint.c
> +++ b/src/box/sql/fk_constraint.c
> @@ -309,31 +309,30 @@ fk_constraint_lookup_parent(struct Parse *parse_context, struct space *parent,
>    * the data for given space.  regBase+1 holds the first column.
>    * regBase+2 holds the second column, and so forth.
>    *

3. Now the names above are outdated.

> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index f0388b481..1a5882dda 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -529,12 +529,16 @@ selcollist(A) ::= sclp(A) expr(X) as(Y).     {
>      sqlExprListSetSpan(pParse,A,&X);
>   }
>   selcollist(A) ::= sclp(A) STAR. {
> -  Expr *p = sqlExpr(pParse->db, TK_ASTERISK, 0);
> +  struct Expr *p = sql_op_expr_new(pParse->db, TK_ASTERISK, NULL);
> +  if (p == NULL)
> +    sql_parser_error(pParse);
>     A = sql_expr_list_append(pParse->db, A, p);
>   }
>   selcollist(A) ::= sclp(A) nm(X) DOT STAR. {
>     Expr *pRight = sqlPExpr(pParse, TK_ASTERISK, 0, 0);
> -  Expr *pLeft = sqlExprAlloc(pParse->db, TK_ID, &X, 1);
> +  struct Expr *pLeft = sql_expr_new(pParse->db, TK_ID, &X, true);
> +  if (pLeft == NULL)
> +    sql_parser_error(pParse);

4. The same as about other similar places. Why not 'return'?

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index fda4296cc..5434a2ab0 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c> @@ -506,7 +511,12 @@ addWhereTerm(Parse * pParse,	/* Parsing context */
>   		ExprSetVVAProperty(pEq, EP_NoReduce);
>   		pEq->iRightJoinTable = (i16) pE2->iTable;
>   	}
> -	*ppWhere = sqlExprAnd(db, *ppWhere, pEq);
> +	if (*ppWhere != NULL || pEq != NULL) {
> +		*ppWhere = sql_and_expr_new(pParse->db, *ppWhere, pEq);
> +		if (*ppWhere == NULL)
> +			sql_parser_error(pParse);
> +	}
> +	return;

5. Why do you need a return at the end of a 'void' function?

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 4e6c5717b..4401ea8b7 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -3218,13 +3218,73 @@ void sqlClearTempRegCache(Parse *);
>   #ifdef SQL_DEBUG
>   int sqlNoTempsInRange(Parse *, int, int);
>   #endif
> -Expr *sqlExprAlloc(sql *, int, const Token *, int);
> -Expr *sqlExpr(sql *, int, const char *);
> -Expr *sqlExprInteger(sql *, int);
> +
> +/**
> + * This routine is the core allocator for Expr nodes.
> + * Construct a new expression node and return a pointer to it.
> + * Memory for this node and for the token argument is a single
> + * allocation obtained from sqlDbMalloc(). The calling function
> + * is responsible for making sure the node eventually gets freed.
> + *
> + * If dequote is true, then the token (if it exists) is dequoted.
> + * If dequote is false, no dequoting is performed.  The dequote
> + * parameter is ignored if token is NULL or if the token does
> + * not appear to be quoted. If the quotes were of the form "..."
> + * (double-quotes) then the EP_DblQuoted flag is set on the
> + * expression node.
> + *
> + * Special case: If op==TK_INTEGER and token points to a string
> + * that can be translated into a 32-bit integer, then the token is
> + * not stored in u.zToken. Instead, the integer values is written
> + * into u.iValue and the EP_IntValue flag is set. No extra storage
> + * is allocated to hold the integer text and the dequote flag is
> + * ignored.
> + *
> + * @param db The database connection.
> + * @param op Expression opcode (TK_*).
> + * @param token Source token. Might be NULL.
> + * @param dequote True to dequote string.
> + * @retval Not NULL new expression object on success.
> + * @retval NULL otherwise. The diag message is set.
> + */
> +struct Expr *
> +sql_expr_new(struct sql *db, int op, const Token *token, bool dequote);
> +
> +/**
> + * Allocate a new expression node from a zero-terminated token
> + * that has already been dequoted.
> + *
> + * @param db The database connection.
> + * @param op Expression opcode.
> + * @param name The object name string.
> + * @retval Not NULL expression pointer on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct Expr *
> +sql_op_expr_new(struct sql *db, int op, const char *name);
> +
>   void sqlExprAttachSubtrees(sql *, Expr *, Expr *, Expr *);
>   Expr *sqlPExpr(Parse *, int, Expr *, Expr *);
>   void sqlPExprAddSelect(Parse *, Expr *, Select *);
> -Expr *sqlExprAnd(sql *, Expr *, Expr *);
> +
> +/**
> + * Join two expressions using an AND operator.  If either
> + * expression is NULL, then just return the other expression.
> + *
> + * If one side or the other of the AND is known to be false, then
> + * instead of returning an AND expression, just return a constant
> + * expression with a value of false.
> + *
> + * @param db The database connection.
> + * @param left_expr The left-branch expresion to join.
> + * @param right_expr The right-branch expression to join.
> + * @retval Not NULL new expression root node pointer on success.
> + * @retval NULL Otherwise. The diag message is set.
> + */
> +struct Expr *
> +sql_and_expr_new(struct sql *db, struct Expr *left_expr,
> +		 struct Expr *right_expr);
> +
>   Expr *sqlExprFunction(Parse *, ExprList *, Token *);
>   void sqlExprAssignVarNumber(Parse *, Expr *, u32);
>   ExprList *sqlExprListAppendVector(Parse *, ExprList *, IdList *, Expr *);
> @@ -4732,7 +4792,21 @@ void sqlAppendChar(StrAccum *, int, char);
>   char *sqlStrAccumFinish(StrAccum *);
>   void sqlStrAccumReset(StrAccum *);
>   void sqlSelectDestInit(SelectDest *, int, int, int);
> -Expr *sqlCreateColumnExpr(sql *, SrcList *, int, int);
> +
> +/*
> + * Allocate and return a pointer to an expression to load the
> + * column from datasource src_idx in SrcList src_list.
> + *
> + * @param db The database connection.
> + * @param src_list The source list described with FROM clause.
> + * @param src_idx The resource index to use in src_list.
> + * @param column The column index.
> + * @retval Not NULL expression pointer on success.

6. I am still not sure, if I said that earlier, but start
sentences with a capital letter. It is a simple rule. Here
and in all other places below and above.