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

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

Except for comments below, pls rebase on fresh 2.0.
Your branch seems to be way behind current master.

> int sqlite3Stat4ValueFromExpr(Parse *, Expr *, u8, sqlite3_value **);
> void sqlite3Stat4ProbeFree(UnpackedRecord *);
> -int sqlite3Stat4Column(sqlite3 *, const void *, int, int, sqlite3_value **);
> +
> +/*

Start comment with /**

> + * Extract the iCol-th column from the record in pRec.  Write
> + * the column value into *ppVal.  If *ppVal is initially NULL

Lets move from Hungarian notation to Tarantool-style naming.
Moreover, you didn’t specify args names in func prototype.

> + * then a new sqlite3_value object is allocated.
> + *
> + * If *ppVal is initially NULL then the caller is responsible for
> + * ensuring that the value written into *ppVal is eventually
> + * freed.
> + *
> + * @param db Database handle.
> + * @param pRec Pointer to buffer containing record.
> + * @param iCol Column to extract.
> + * @param[out] ppVal Extracted value.
> + *
> + * @retval Error code or SQLITE_OK.

Or SQLITE_OK on success (or 0 after fixes).

> + */
> +int
> +sql_stat4_column(struct 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..821d4db74 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1588,55 +1588,29 @@ sqlite3Stat4ValueFromExpr(Parse * pParse,	/* Parse context */
> 	return stat4ValueFromExpr(pParse, pExpr, affinity, 0, ppVal);
> }
> 
> -/*
> - * Extract the iCol-th column from the nRec-byte record in pRec.  Write
> - * the column value into *ppVal.  If *ppVal is initially NULL then a new
> - * sqlite3_value object is allocated.
> - *
> - * If *ppVal is initially NULL then the caller is responsible for
> - * ensuring that the value written into *ppVal is eventually freed.
> - */
> 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 */
> -    )
> +sql_stat4_column(struct sqlite3 *db, const void *record, int col_num,
> +		 sqlite3_value **res)
> {
> -	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 */
> -	Mem *pMem = *ppVal;	/* Write result into this Mem object */
> -
> -	assert(iCol > 0);
> -	iHdr = getVarint32(a, nHdr);
> -	if (nHdr > nRec || iHdr >= nHdr)
> -		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;
> -	}
> -	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;
> +	assert(col_num >= 0);
> +	/* Write result into this Mem object */

Put dots at the end of sentences.

> +	Mem *mem = *res;

Lets use struct prefix.

> +	/* Typecast byte array */
> +	const char *a = (const char *) record;

Lets pass already const char*. AFAIK all routine connected with MsgPack
accepts const char *, but not void *.

> +	assert(mp_typeof(a[0]) == MP_ARRAY);
> +	int col_cnt = mp_decode_array(&a);
> +	assert(col_cnt > col_num);
> +	for (int i = 0; i < col_num; i++)
> +		mp_next(&a);
> +	if (mem == 0) {

== NULL

> +		mem = *res = sqlite3ValueNew(db);

Don’t use double assignments.

> +		if (mem == 0) {

== NULL

> +			diag_set(OutOfMemory, sizeof(Mem), "sqlite3ValeuNew”,

sqlite3ValueNew

> +				 "mem");
> +			return -1;
> +		}
> 	}
> -	sqlite3VdbeSerialGet(&a[iField - szField], t, pMem);
> +	sqlite3VdbeMsgpackGet((const unsigned char *) record, mem);
> 	return SQLITE_OK;

Just return 0 (in fact, they are the same thing).