[tarantool-patches] Re: [PATCH] sql: fix decode analyze sample

["n.pettik" <korablev@tarantool.org>]

> On 18 May 2018, at 16:47, AKhatskevich <avkhatskevich@tarantool.org> wrote:
> 
> branch: kh/gh-2860-skip-scan
> 
> Analyze samples are encoded with msgpack and should be decoded as it is
> a msgpack.
> 
> Closes #2860
> —

Put here link to the branch on GitHub, and link to the issue.

> src/box/sql/sqliteInt.h      |  2 +-
> src/box/sql/vdbemem.c        | 35 +++++++------------------
> src/box/sql/where.c          |  4 +--
> test/sql-tap/whereG.test.lua | 61 +++++++++++++++++++++++++++++++++++++++++++-
> 4 files changed, 72 insertions(+), 30 deletions(-)
> 
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 2800be583..a5b33fe71 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4020,7 +4020,7 @@ int sqlite3Stat4ProbeSetValue(Parse *, Index *, UnpackedRecord **, Expr *, int,
> 			      int, int *);
> int sqlite3Stat4ValueFromExpr(Parse *, Expr *, u8, sqlite3_value **);
> void sqlite3Stat4ProbeFree(UnpackedRecord *);
> -int sqlite3Stat4Column(sqlite3 *, const void *, int, int, sqlite3_value **);
> +int sqlite3Stat4Column(sqlite3 *, const void *, int, sqlite3_value **);
> char sqlite3IndexColumnAffinity(sqlite3 *, Index *, int);
> 
> /*
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index f91d7119c..70e03adda 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1589,7 +1589,7 @@ sqlite3Stat4ValueFromExpr(Parse * pParse,	/* Parse context */
> }
> 
> /*
> - * Extract the iCol-th column from the nRec-byte record in pRec.  Write
> + * Extract the iCol-th column from the record in pRec.  Write
>  * the column value into *ppVal.  If *ppVal is initially NULL then a new
>  * sqlite3_value object is allocated.
>  *
> @@ -1599,44 +1599,27 @@ sqlite3Stat4ValueFromExpr(Parse * pParse,	/* Parse context */
> int
> sqlite3Stat4Column(sqlite3 * db,	/* Database handle */
> 		   const void *pRec,	/* Pointer to buffer containing record */
> -		   int nRec,	/* Size of buffer pRec in bytes */
> 		   int iCol,	/* Column to extract */
> 		   sqlite3_value ** ppVal	/* OUT: Extracted value */
>     )

I see that provided changes significantly refactor this function.
Would you mind to fix code style for whole function to make it
look like function from server?

> {
> -	u32 t;			/* a column type code */
> -	int nHdr;		/* Size of the header in the record */
> -	int iHdr;		/* Next unread header byte */
> -	int iField;		/* Next unread data byte */
> -	int szField = 0;	/* Size of the current data field */
> 	int i;			/* Column index */
> -	u8 *a = (u8 *) pRec;	/* Typecast byte array */
> +	const char *a = (const char *) pRec;	/* Typecast byte array */
> 	Mem *pMem = *ppVal;	/* Write result into this Mem object */
> -
> -	assert(iCol > 0);
> -	iHdr = getVarint32(a, nHdr);
> -	if (nHdr > nRec || iHdr >= nHdr)
> +	assert(iCol >= 0);
> +	assert(mp_typeof(a[0]) == MP_ARRAY);
> +	int n_col = mp_decode_array(&a);
> +	if (n_col <= iCol)
> 		return SQLITE_CORRUPT_BKPT;

Lest just return 0 in case of success, and -1 otherwise.

> -	iField = nHdr;
> -	for (i = 0; i <= iCol; i++) {
> -		iHdr += getVarint32(&a[iHdr], t);
> -		testcase(iHdr == nHdr);
> -		testcase(iHdr == nHdr + 1);
> -		if (iHdr > nHdr)
> -			return SQLITE_CORRUPT_BKPT;
> -		szField = sqlite3VdbeSerialTypeLen(t);
> -		iField += szField;
> +	for (i = 0; i < iCol; i++) {
> +		mp_next(&a);
> 	}

We don’t surround one-line if/for with brackets.

> -	testcase(iField == nRec);
> -	testcase(iField == nRec + 1);
> -	if (iField > nRec)
> -		return SQLITE_CORRUPT_BKPT;
> 	if (pMem == 0) {
> 		pMem = *ppVal = sqlite3ValueNew(db);
> 		if (pMem == 0)
> 			return SQLITE_NOMEM_BKPT;
> 	}
> -	sqlite3VdbeSerialGet(&a[iField - szField], t, pMem);
> +	sqlite3VdbeMsgpackGet((const unsigned char *) a, pMem);
> 	return SQLITE_OK;
> }
> 
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index cde38152a..a4a1aee98 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -1183,8 +1183,8 @@ whereRangeSkipScanEst(Parse * pParse,		/* Parsing & code generating context */
> 		int i;
> 		int nDiff;
> 		for (i = 0; rc == SQLITE_OK && i < p->nSample; i++) {
> -			rc = sqlite3Stat4Column(db, p->aSample[i].p,
> -						p->aSample[i].n, nEq, &pVal);
> +			rc = sqlite3Stat4Column(db, p->aSample[i].p, nEq,
> +						&pVal);
> 			if (rc == SQLITE_OK && p1) {
> 				int res = sqlite3MemCompare(p1, pVal, pColl);
> 				if (res >= 0)
> diff --git a/test/sql-tap/whereG.test.lua b/test/sql-tap/whereG.test.lua
> index 1842f2ae8..68b8ce488 100755
> --- a/test/sql-tap/whereG.test.lua
> +++ b/test/sql-tap/whereG.test.lua
> @@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
> -test:plan(20)
> +test:plan(22)
> 
> --!./tcltestrunner.lua
> -- 2013-09-05
> @@ -388,5 +388,64 @@ test:do_execsql_test(
>         -- </7.3>
>     })
> 
> +-- gh-2860 skip-scan plan
> +test:execsql([[
> +    CREATE TABLE people(name TEXT PRIMARY KEY, role TEXT NOT NULL,
> +        height INT NOT NULL, CHECK(role IN ('student','teacher')));
> +    CREATE INDEX people_idx1 ON people(role, height);
> +    INSERT INTO people VALUES('Alice','student',156);
> +    INSERT INTO people VALUES('Bob','student',161);
> +    INSERT INTO people VALUES('Cindy','student',155);
> +    INSERT INTO people VALUES('David','student',181);
> +    INSERT INTO people VALUES('Emily','teacher',158);
> +    INSERT INTO people VALUES('Fred','student',163);
> +    INSERT INTO people VALUES('Ginny','student',169);
> +    INSERT INTO people VALUES('Harold','student',172);
> +    INSERT INTO people VALUES('Imma','student',179);
> +    INSERT INTO people VALUES('Jack','student',181);
> +    INSERT INTO people VALUES('Karen','student',163);
> +    INSERT INTO people VALUES('Logan','student',177);
> +    INSERT INTO people VALUES('Megan','teacher',159);
> +    INSERT INTO people VALUES('Nathan','student',163);
> +    INSERT INTO people VALUES('Olivia','student',161);
> +    INSERT INTO people VALUES('Patrick','teacher',180);
> +    INSERT INTO people VALUES('Quiana','student',182);
> +    INSERT INTO people VALUES('Robert','student',159);
> +    INSERT INTO people VALUES('Sally','student',166);
> +    INSERT INTO people VALUES('Tom','student',171);
> +    INSERT INTO people VALUES('Ursula','student',170);
> +    INSERT INTO people VALUES('Vance','student',179);
> +    INSERT INTO people VALUES('Willma','student',175);
> +    INSERT INTO people VALUES('Xavier','teacher',185);
> +    INSERT INTO people VALUES('Yvonne','student',149);
> +    INSERT INTO people VALUES('Zach','student',170);
> +    INSERT INTO people VALUES('Angie','student',166);
> +    INSERT INTO people VALUES('Brad','student',176);
> +    INSERT INTO people VALUES('Claire','student',168);
> +    INSERT INTO people VALUES('Donald','student',162);
> +    INSERT INTO people VALUES('Elaine','student',177);
> +    INSERT INTO people VALUES('Frazier','student',159);
> +    INSERT INTO people VALUES('Grace','student',179);
> +    INSERT INTO people VALUES('Horace','student',166);
> +    INSERT INTO people VALUES('Ingrad','student',155);
> +    INSERT INTO people VALUES('Jacob','student',179);
> +    INSERT INTO people VALUES('John', 'student', 154);
> +    ANALYZE;
> +]])
> +
> +test:do_execsql_test(
> +    "7.1",
> +    [[
> +        SELECT name FROM people WHERE height>=180;
> +    ]],
> +    {"David","Jack","Quiana","Patrick","Xavier"})
> +
> +test:do_execsql_test(
> +    "7.2",
> +    [[
> +        EXPLAIN QUERY PLAN SELECT name FROM people WHERE height>=180;
> +    ]],

Lets execute the same select before ANALYZE to make sure that plan has changed.

> +    {0,0,0,"SEARCH TABLE PEOPLE USING COVERING INDEX PEOPLE_IDX1" ..
> +        " (ANY(ROLE) AND HEIGHT>?)"})
> 
> test:finish_test()
> -- 
> 2.14.1
> 
>