From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 1999227661 for ; Mon, 11 Feb 2019 18:54:02 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cplV0xYfvIQi for ; Mon, 11 Feb 2019 18:54:02 -0500 (EST) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 87CF92108B for ; Mon, 11 Feb 2019 18:54:01 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: store statistics in statN as an array of integers From: "n.pettik" In-Reply-To: Date: Tue, 12 Feb 2019 02:53:59 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Roman Khabibov > 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 =3D (K+D-1)/D > */ > - char *z; > int i; >=20 > - char *zRet =3D sqlite3MallocZero((p->nKeyCol + 1) * 25); > + size_t size =3D mp_sizeof_array(p->nKeyCol + 1); > + size +=3D mp_sizeof_uint(p->nRow); > + for (i =3D 0; i < p->nKeyCol; ++i) { > + uint64_t nDistinct =3D p->current.anDLt[i] + 1; > + uint64_t iVal =3D (p->nRow + nDistinct - 1) / = nDistinct; Use Tarantool-like names: distinct_count etc > + size +=3D mp_sizeof_uint(iVal); > + } > + char *zRet =3D sqlite3MallocZero(size); > if (zRet =3D=3D 0) { > sqlite3_result_error_nomem(context); > return; > } > - > - sqlite3_snprintf(24, zRet, "%llu", (u64) p->nRow); > - z =3D zRet + sqlite3Strlen30(zRet); > - for (i =3D 0; i < p->nKeyCol; i++) { > - u64 nDistinct =3D p->current.anDLt[i] + 1; > - u64 iVal =3D (p->nRow + nDistinct - 1) / = nDistinct; > - sqlite3_snprintf(24, z, " %llu", iVal); > - z +=3D sqlite3Strlen30(z); > + char *z =3D zRet; > + z =3D mp_encode_array(z, p->nKeyCol + 1); > + z =3D mp_encode_uint(z, p->nRow); > + for (i =3D 0; i < p->nKeyCol; ++i) { > + uint64_t nDistinct =3D p->current.anDLt[i] + 1; > + uint64_t iVal =3D (p->nRow + nDistinct - 1) / = nDistinct; > + z =3D mp_encode_uint(z, iVal); > assert(p->current.anEq[i]); > } > - assert(z[0] =3D=3D '\0' && z > zRet); > + const char *b =3D zRet; > + int r =3D mp_check(&b, z); > + assert(!r); Why not assert(mp_check(&b, z) =3D=3D 0); ? > + assert(zRet !=3D z); >=20 > - sqlite3_result_text(context, zRet, -1, sqlite3_free); > + sqlite3_result_msgpack(context, zRet, size, = sqlite3_free); > } else if (eCall =3D=3D STAT_GET_KEY) { > if (p->iGet < 0) { > samplePushPrevious(p, 0); > @@ -713,19 +721,28 @@ statGet(sqlite3_context * context, int argc, = sqlite3_value ** argv) > } > } >=20 > - char *zRet =3D sqlite3MallocZero(p->nCol * 25); > + int i; > + > + size_t size =3D mp_sizeof_array(p->nCol); > + for (i =3D 0; i < p->nCol; ++i) { > + size +=3D mp_sizeof_uint(aCnt[i]); > + } > + > + char *zRet =3D sqlite3MallocZero(size); > if (zRet =3D=3D 0) { > sqlite3_result_error_nomem(context); > } else { > - int i; > char *z =3D zRet; > + z =3D mp_encode_array(z, p->nCol); > for (i =3D 0; i < p->nCol; i++) { > - sqlite3_snprintf(24, z, "%llu ", (u64) aCnt[i]); > - z +=3D sqlite3Strlen30(z); > + z =3D mp_encode_uint(z, aCnt[i]); > } > - assert(z[0] =3D=3D '\0' && z > zRet); > - z[-1] =3D '\0'; > - sqlite3_result_text(context, zRet, -1, sqlite3_free); > + const char *b =3D zRet; > + int r =3D mp_check(&b, z); > + assert(!r); The same: assert(mp_check() =3D=3D 0); > + assert(zRet !=3D z); > + > + sqlite3_result_msgpack(context, zRet, size, = sqlite3_free); > } >=20 > } > @@ -1166,35 +1183,33 @@ struct analysis_index_info { > }; >=20 > /** > - * 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 =3D 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 =3D 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 =3D=3D NULL) > - z =3D ""; > - for (int i =3D 0; *z && i < stat_size; i++) { > - tRowcnt v =3D 0; > - int c; > - while ((c =3D z[0]) >=3D '0' && c <=3D '9') { > - v =3D v * 10 + c - '0'; > - z++; > - } > + return; > + for (int i =3D 0; i < stat_size; i++) { > + tRowcnt v =3D (tRowcnt) mp_decode_uint(&z); > if (stat_exact !=3D NULL) > stat_exact[i] =3D v; > if (stat_log !=3D NULL) > stat_log[i] =3D sqlite3LogEst(v); > - if (*z =3D=3D ' ') > - z++; > } > + if (!offset) > + UNUSED_PARAMETER(offset); > + else > + offset =3D &z; I don=E2=80=99t understand why you can=E2=80=99t do this: if (offset !=3D NULL) offset =3D &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 =3D false; > stat->skip_scan_enabled =3D true; > - char *z =3D argv[2]; > - /* Position ptr at the end of stat string. */ > - for (; *z =3D=3D ' ' || (*z >=3D '0' && *z <=3D '9'); ++z); > - while (z[0]) { > - if (sql_strlike_cs("unordered%", z, '[') =3D=3D 0) > + const char *z =3D argv[2]; > + uint32_t keyword_count =3D mp_decode_array(&z) - column_count;=20= 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 =3D mp_decode_str(&offset, &sval_len); > + if (!sqlite3_stricmp(sval, "unordered")) { > index->def->opts.stat->is_unordered =3D true; > - else if (sql_strlike_cs("noskipscan%", z, '[') =3D=3D 0) > + } else if (!sqlite3_stricmp(sval, "noskipscan")) { > index->def->opts.stat->skip_scan_enabled =3D = false; > - while (z[0] !=3D 0 && z[0] !=3D ' ') > - z++; > - while (z[0] =3D=3D ' ') > - 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"=3D'T1' and "idx"=3D'I1' ORDER BY "nlt" LIMIT 1; > - ]], { > - -- > - "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10" > - -- > -}) > +-- test:do_execsql_test( > +-- "analyze-6.1.3", > +-- [[ > +-- SELECT "tbl", "idx", "neq", "nlt", "ndlt" FROM = "_sql_stat4" where "tbl"=3D'T1' and "idx"=3D'I1' ORDER BY "nlt" LIMIT 1; > +-- ]], { > +-- -- > +-- "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10" > +-- -- > +-- })