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

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

[AKhatskevich]

branch: kh/gh-2860-skip-scan

Analyze samples are encoded with msgpack and should be decoded as it is
a msgpack.

Closes #2860
---
 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 */
     )
 {
-	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;
-	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);
 	}
-	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;
+    ]],
+    {0,0,0,"SEARCH TABLE PEOPLE USING COVERING INDEX PEOPLE_IDX1" ..
+        " (ANY(ROLE) AND HEIGHT>?)"})
 
 test:finish_test()
-- 
2.14.1

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

[n.pettik]

> 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
> 
> 

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

[n.pettik]

> On 1 Jun 2018, at 14:41, Alex Khatskevich <avkhatskevich@tarantool.org> wrote:
> 
> Hi
>> Put here link to the branch on GitHub, and link to the issue.
> Branch: https://github.com/tarantool/tarantool/tree/kh/gh-2860-skip-scan
> Issue: https://github.com/tarantool/tarantool/issues/2860
>> 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?
> Done.

According to SOP conventions, you should attach diff of provided changes,
or send next version of patch. Otherwise, it is quite complicated to comment your fixes…

Firstly, lets move function’s comment to header and fix length:
comments should fit into 66 chars. Then, rename function to sql_stat4_column();
add struct prefixes to types which are really structs; don’t use Hungarian notation;
put comments above the code to be commented; don’t use beforehand declaration
for cycle counters (until it is really vital).

With these changes code would look more elegant

>> Lest just return 0 in case of success, and -1 otherwise.
> I do not like this approach.

Actually, it is not a suggestion: in server almost all functions obey this rule (AFAIK).
If function may fail due to different reasons, you can set error message explicitly.
For instance, on OOM: diag_set(OutOfMemory, …);

Except for these nitpickings, patch LGTM.