[tarantool-patches] Re: [PATCH v3 4/9] sql: remove field rc of struct Parse

[n.pettik]

Nit: remove from.

> Currently, the field rc of the struct Parse can have only two
> values: SQL_OK or SQL_TARANTOOL_ERROR. Therefore, it is logical to
> replace it with a new boolean field. This patche replaces field rc
> by new field is_aborted.

Fixed commit message (original one contained several mistakes):

    sql: replace rc with is_abort status in stuct Parse
    
    Currently, field representing return code in struct Parse can take only
    two values: SQL_OK (successfully finished parsing) and
    SQL_TARANTOOL_ERROR (in case of any errors occurred). Therefore, it can
    be replaced with a boolean field. Patch provides straightforward
    refactoring.
    
    Part of #3965

> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 0c6296d..42737ff 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -100,15 +100,15 @@ sqlPrepare(sql * db,	/* Database handle. */
> 	}
> 	assert(0 == sParse.nQueryLoop);
> 
> -	if (sParse.rc == SQL_DONE)
> -		sParse.rc = SQL_OK;
> 	if (db->mallocFailed) {
> -		sParse.rc = SQL_NOMEM;
> +		diag_set(OutOfMemory, 0, "SQL", "db");
> +		sParse.is_aborted = true;

See comments for previous patches.

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 719b85e..42ff4b8 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -2643,7 +2643,6 @@ struct Parse {
> 	sql *db;		/* The main database structure */
> 	char *zErrMsg;		/* An error message */
> 	Vdbe *pVdbe;		/* An engine for executing database bytecode */
> -	int rc;			/* Return code from execution */
> 	u8 colNamesSet;		/* TRUE after OP_ColumnName has been issued to pVdbe */
> 	u8 nTempReg;		/* Number of temporary registers in aTempReg[] */
> 	u8 isMultiWrite;	/* True if statement may modify/insert multiple rows */
> @@ -2677,6 +2676,8 @@ struct Parse {
> 	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
> 	/** Region to make SQL temp allocations. */
> 	struct region region;
> +	/** Flag to show that parsing should be aborted. */

Comment is misleading: now we don’t abort parsing process,
but instead allow it to be finished and raise an error at the end.
Fix comment pls.

> +	bool is_aborted;
> 
>   /**************************************************************************
>   * Fields above must be initialized to zero.  The fields that follow,
> @@ -534,25 +533,17 @@ sqlRunParser(Parse * pParse, const char *zSql, char **pzErrMsg)
> 	sqlParserFree(pEngine, sql_free);
> 	if (db->mallocFailed) {
> 		diag_set(OutOfMemory, 0, "SQL", "db");
> -		pParse->rc = SQL_TARANTOOL_ERROR;
> -	}
> -	if (pParse->rc != SQL_OK && pParse->rc != SQL_DONE
> -	    && pParse->zErrMsg == 0) {
> -		const char *error;
> -		if (is_tarantool_error(pParse->rc) &&
> -		    tarantoolErrorMessage() != NULL)
> -			error = tarantoolErrorMessage();
> -		else
> -			error = sqlErrStr(pParse->rc);
> -		pParse->zErrMsg = sqlMPrintf(db, "%s", error);
> +		pParse->is_aborted = true;
> 	}
> +	if (pParse->is_aborted && pParse->zErrMsg == 0)
> +		pParse->zErrMsg = sqlMPrintf(db, "%s", tarantoolErrorMessage());
> 	assert(pzErrMsg != 0);
> 	if (pParse->zErrMsg) {
> 		*pzErrMsg = pParse->zErrMsg;
> -		sql_log(pParse->rc, "%s", *pzErrMsg);
> +		sql_log(SQL_TARANTOOL_ERROR, "%s", *pzErrMsg);

Do we need this call at all?

> diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
> index bab6493..0d8bf15 100755
> --- a/test/sql-tap/check.test.lua
> +++ b/test/sql-tap/check.test.lua
> @@ -516,7 +516,7 @@ test:do_catchsql_test(
>         );
>     ]], {
>         -- <check-5.1>
> -        1, "Wrong space options (field 5): invalid expression specified (SQL error: bindings are not allowed in DDL)"
> +        1, "Wrong space options (field 5): invalid expression specified (bindings are not allowed in DDL)"
>         -- </check-5.1>
>     })

Why test results have changed if you provided
non-functional refactoring?