[tarantool-patches] Re: [PATCH v2] sql: prohibit negative values under LIMIT clause

[n.pettik]

> If LIMIT or OFFSET expressions can be casted to the
> negative integer value VDBE returns an error.
> If expression in the LIMIT clause can't be converted
> into integer without data loss the VDBE instead of
> SQLITE_MISMATCH returns SQL_TARANTOOL_ERROR with
> message "Only positive numbers are allowed in the
> LIMIT clause". The same for OFFSET clause.
> 
> Closes #3467

Fix please commit message subject: now not only negative
values are rejected, but everything except for positive integers.

Below a few minor comments. Don’t send new version of patch,
just answer to this letter with provided fixes.

> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 02ee225f1..a8ed3c346 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -2074,7 +2074,8 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 	Vdbe *v = 0;
> 	int iLimit = 0;
> 	int iOffset;
> -	int n;
> +	const char *negativeLimitError = "Only positive numbers are allowed "
> +					 "in the LIMIT clause”;

Nit: now this variable is used only once, so I suggest to inline it
or at least move closer to its usage. SQLite uses very ancient
way of declaring variables at the beginning of function
(I guess it is due to C 89 standard).

One more nit:

select * from t1 limit 0.5;
---
- error: Only positive numbers are allowed in the LIMIT clause
…

However, 0.5 is a positive number. Lets re-phrase error message a bit.
Like: Only positive integers are allowed in the LIMIT clause

> 	if (p->iLimit)
> 		return;
> 
> @@ -2097,24 +2098,29 @@ computeLimitRegisters(Parse * pParse, Select * p, int iBreak)
> 		p->iLimit = iLimit = ++pParse->nMem;
> 		v = sqlite3GetVdbe(pParse);
> 		assert(v != 0);
> -		if (sqlite3ExprIsInteger(p->pLimit, &n)) {
> -			sqlite3VdbeAddOp2(v, OP_Integer, n, iLimit);
> -			VdbeComment((v, "LIMIT counter"));
> -			if (n == 0) {
> -				sqlite3VdbeGoto(v, iBreak);
> -			} else if (n >= 0
> -				   && p->nSelectRow > sqlite3LogEst((u64) n)) {
> -				p->nSelectRow = sqlite3LogEst((u64) n);
> -				p->selFlags |= SF_FixedLimit;
> -			}
> -		} else {
> -			sqlite3ExprCode(pParse, p->pLimit, iLimit);
> -			sqlite3VdbeAddOp1(v, OP_MustBeInt, iLimit);
> -			VdbeCoverage(v);
> -			VdbeComment((v, "LIMIT counter"));
> -			sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
> -			VdbeCoverage(v);
> -		}
> +		int lPosLimitValue = sqlite3VdbeMakeLabel(v);
> +		int labelHalt = sqlite3VdbeMakeLabel(v);

Could you please rename these two variables according to our
code style (as I pointed in previous review)? For example:

int IPosLimitValue -> positive_limit_label; 
int labelHalt -> int halt_label;

The same for code concerning LIMIT clause.

(Yep, it is kind of messing code styles, but we decided to use
our codestyle in freshly added/refactored code).

> +		sqlite3ExprCode(pParse, p->pLimit, iLimit);
> +		sqlite3VdbeAddOp2(v, OP_MustBeInt, iLimit, labelHalt);
> +		/* If LIMIT clause >= 0 continue execution */
> +		int r1 = sqlite3GetTempReg(pParse);
> +		sqlite3VdbeAddOp2(v, OP_Integer, 0, r1);
> +		sqlite3VdbeAddOp3(v, OP_Ge, r1, lPosLimitValue, iLimit);
> +		/* Otherwise return an error and stop */
> +		sqlite3VdbeResolveLabel(v, labelHalt);
> +		sqlite3VdbeAddOp4(v, OP_Halt,
> +				  SQL_TARANTOOL_ERROR,
> +				  0, 0,
> +				  negativeLimitError,
> +				  P4_STATIC);
> +
> +		sqlite3VdbeResolveLabel(v, lPosLimitValue);
> +		sqlite3ReleaseTempReg(pParse, r1);
> +		VdbeCoverage(v);
> +		VdbeComment((v, "LIMIT counter"));
> +		sqlite3VdbeAddOp2(v, OP_IfNot, iLimit, iBreak);
> +		VdbeCoverage(v);
> +
> 
> diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
> index 664090368..919ddcb3c 100644
> cn:execute('select * from test where id = ?', {1})

Add a short comment like:
gh-3467: allow only positive integers under limit clause.

> +cn:execute('select * from test limit ?', {2})
> +cn:execute('select * from test limit ?', {-2})
> +cn:execute('select * from test limit ?', {2.7})
> +cn:execute('select * from test limit ?', {'Hello'})