[tarantool-patches] Re: [PATCH 2/2] sql: store statistics in statN as an array of integers

[n.pettik]

> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 51c63fa7a..a7aaf665d 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -664,27 +664,35 @@ statGet(sqlite3_context * context, int argc, sqlite3_value ** argv)
> 		 *
> 		 *        I = (K+D-1)/D
> 		 */
> -		char *z;
> 		int i;
> 
> -		char *zRet = sqlite3MallocZero((p->nKeyCol + 1) * 25);
> +		size_t size = mp_sizeof_array(p->nKeyCol + 1);
> +		size += mp_sizeof_uint(p->nRow);
> +		for (i = 0; i < p->nKeyCol; ++i) {
> +			uint64_t nDistinct = p->current.anDLt[i] + 1;
> +			uint64_t iVal = (p->nRow + nDistinct - 1) / nDistinct;

Use Tarantool-like names: distinct_count etc

> +			size += mp_sizeof_uint(iVal);
> +		}
> +		char *zRet = sqlite3MallocZero(size);
> 		if (zRet == 0) {
> 			sqlite3_result_error_nomem(context);
> 			return;
> 		}
> -
> -		sqlite3_snprintf(24, zRet, "%llu", (u64) p->nRow);
> -		z = zRet + sqlite3Strlen30(zRet);
> -		for (i = 0; i < p->nKeyCol; i++) {
> -			u64 nDistinct = p->current.anDLt[i] + 1;
> -			u64 iVal = (p->nRow + nDistinct - 1) / nDistinct;
> -			sqlite3_snprintf(24, z, " %llu", iVal);
> -			z += sqlite3Strlen30(z);
> +		char *z = zRet;
> +		z = mp_encode_array(z, p->nKeyCol + 1);
> +		z = mp_encode_uint(z, p->nRow);
> +		for (i = 0; i < p->nKeyCol; ++i) {
> +			uint64_t nDistinct = p->current.anDLt[i] + 1;
> +			uint64_t iVal = (p->nRow + nDistinct - 1) / nDistinct;
> +			z = mp_encode_uint(z, iVal);
> 			assert(p->current.anEq[i]);
> 		}
> -		assert(z[0] == '\0' && z > zRet);
> +		const char *b = zRet;
> +		int r = mp_check(&b, z);
> +		assert(!r);

Why not assert(mp_check(&b, z) == 0); ?

> +		assert(zRet != z);
> 
> -		sqlite3_result_text(context, zRet, -1, sqlite3_free);
> +		sqlite3_result_msgpack(context, zRet, size, sqlite3_free);
> 	} else if (eCall == STAT_GET_KEY) {
> 		if (p->iGet < 0) {
> 			samplePushPrevious(p, 0);
> @@ -713,19 +721,28 @@ statGet(sqlite3_context * context, int argc, sqlite3_value ** argv)
> 		}
> 	}
> 
> -	char *zRet = sqlite3MallocZero(p->nCol * 25);
> +	int i;
> +
> +	size_t size = mp_sizeof_array(p->nCol);
> +	for (i = 0; i < p->nCol; ++i) {
> +		size += mp_sizeof_uint(aCnt[i]);
> +	}
> +
> +	char *zRet = sqlite3MallocZero(size);
> 	if (zRet == 0) {
> 		sqlite3_result_error_nomem(context);
> 	} else {
> -		int i;
> 		char *z = zRet;
> +		z = mp_encode_array(z, p->nCol);
> 		for (i = 0; i < p->nCol; i++) {
> -			sqlite3_snprintf(24, z, "%llu ", (u64) aCnt[i]);
> -			z += sqlite3Strlen30(z);
> +			z = mp_encode_uint(z, aCnt[i]);
> 		}
> -		assert(z[0] == '\0' && z > zRet);
> -		z[-1] = '\0';
> -		sqlite3_result_text(context, zRet, -1, sqlite3_free);
> +		const char *b = zRet;
> +		int r = mp_check(&b, z);
> +		assert(!r);

The same: assert(mp_check() == 0);

> +		assert(zRet != z);
> +
> +		sqlite3_result_msgpack(context, zRet, size, sqlite3_free);
> 	}
> 
> }
> @@ -1166,35 +1183,33 @@ struct analysis_index_info {
> };
> 
> /**
> - * The first argument points to a nul-terminated string
> - * containing a list of space separated integers. Load
> - * the first stat_size of these into the output arrays.
> + * The first argument points to a msg_pack array
> + * containing a list of integers. Load the first
> + * stat_size of these into the output arrays.
>  *
> - * @param stat_string String containing array of integers.
> + * @param stat_array MP_ARRAY containing array of integers.
>  * @param stat_size Size of output arrays.
>  * @param[out] stat_exact Decoded array of statistics.
>  * @param[out] stat_log Decoded array of stat logariphms.
>  */
> static void
> -decode_stat_string(const char *stat_string, int stat_size, tRowcnt *stat_exact,
> -		   LogEst *stat_log) {
> -	const char *z = stat_string;
> +decode_stat_array(const char *stat_array, int stat_size, tRowcnt *stat_exact,
> +		   LogEst *stat_log, const char **offset) {

What is the offset param? Describe it in the comment.

A bit broken indentation; put bracer at the next line.

> +	const char *z = stat_array;
> +	mp_decode_array(&z);

mp_decode_array returns number of elements in array.
It is like rude to throw away that information.

> 	if (z == NULL)
> -		z = "";
> -	for (int i = 0; *z && i < stat_size; i++) {
> -		tRowcnt v = 0;
> -		int c;
> -		while ((c = z[0]) >= '0' && c <= '9') {
> -			v = v * 10 + c - '0';
> -			z++;
> -		}
> +		return;
> +	for (int i = 0; i < stat_size; i++) {
> +		tRowcnt v = (tRowcnt) mp_decode_uint(&z);
> 		if (stat_exact != NULL)
> 			stat_exact[i] = v;
> 		if (stat_log != NULL)
> 			stat_log[i] = sqlite3LogEst(v);
> -		if (*z == ' ')
> -			z++;
> 	}
> +	if (!offset)
> +		UNUSED_PARAMETER(offset);
> +	else
> +		offset = &z;

I don’t understand why you can’t do this:

if (offset != NULL)
	offset = &z;

> @@ -1268,22 +1283,23 @@ analysis_loader(void *data, int argc, char **argv, char **unused)
> 		diag_set(OutOfMemory, stat1_size, "region", "tuple_log_est");
> 		return -1;
> 	}
> -	decode_stat_string(argv[2], column_count, stat->tuple_stat1,
> -			   stat->tuple_log_est);
> +	const char *offset;
> +	decode_stat_array(argv[2], column_count, stat->tuple_stat1,
> +			   stat->tuple_log_est, &offset);

Again indentation is broken a bit. Please, make space and tabs
visible in your code redactor.

> 	stat->is_unordered = false;
> 	stat->skip_scan_enabled = true;
> -	char *z = argv[2];
> -	/* Position ptr at the end of stat string. */
> -	for (; *z == ' ' || (*z >= '0' && *z <= '9'); ++z);
> -	while (z[0]) {
> -		if (sql_strlike_cs("unordered%", z, '[') == 0)
> +	const char *z = argv[2];
> +	uint32_t keyword_count = mp_decode_array(&z) - column_count; 

Okay, there is some mess. Lets move this part to decode_stat_array.
There you already decoded that array, know its length and so on.
Instead of using offset output parameter, simply add one bit mask or two
bool output params.

> +	while (keyword_count) {
> +		const char *sval;
> +		uint32_t sval_len;
> +		sval = mp_decode_str(&offset, &sval_len);
> +		if (!sqlite3_stricmp(sval, "unordered")) {
> 			index->def->opts.stat->is_unordered = true;
> -		else if (sql_strlike_cs("noskipscan%", z, '[') == 0)
> +		} else if (!sqlite3_stricmp(sval, "noskipscan")) {
> 			index->def->opts.stat->skip_scan_enabled = false;
> -		while (z[0] != 0 && z[0] != ' ')
> -			z++;
> -		while (z[0] == ' ')
> -			z++;
> +		}
> +		keyword_count--;
> 	}
> 	return 0;
> }

Ok, I see below some commented tests, so I guess they fail
with segfaults or assertions. Please, dig into the problem.

> -test:do_execsql_test(
> -    "analyze-6.1.3",
> -    [[
> -            SELECT "tbl", "idx", "neq", "nlt", "ndlt" FROM "_sql_stat4" where "tbl"='T1' and "idx"='I1' ORDER BY "nlt" LIMIT 1;
> -    ]], {
> -    -- <analyze-6.1.3>
> -    "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10"
> -    -- </analyze-6.1.3>
> -})
> +-- test:do_execsql_test(
> +--     "analyze-6.1.3",
> +--     [[
> +--             SELECT "tbl", "idx", "neq", "nlt", "ndlt" FROM "_sql_stat4" where "tbl"='T1' and "idx"='I1' ORDER BY "nlt" LIMIT 1;
> +--     ]], {
> +--     -- <analyze-6.1.3>
> +--     "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10"
> +--     -- </analyze-6.1.3>
> +-- })