[tarantool-patches] Re: [PATCH v2 6/7] sql: refactor sql_expr_create to set diag
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Mar 18 22:33:55 MSK 2019
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.
More information about the Tarantool-patches
mailing list