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 4752C201ED for ; Tue, 19 Jun 2018 05:08:17 -0400 (EDT) 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 GyN77eRwWJFU for ; Tue, 19 Jun 2018 05:08:16 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 4DEFB27050 for ; Tue, 19 Jun 2018 04:18:59 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2] sql: fix decode analyze sample References: <20180601165014.30131-1-avkhatskevich@tarantool.org> From: Alex Khatskevich Message-ID: <82a5964f-ccf6-1d29-78e0-186fa1ebcac0@tarantool.org> Date: Tue, 19 Jun 2018 11:18:50 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org > Except for comments below, pls rebase on fresh 2.0. > Your branch seems to be way behind current master > 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. done > Or SQLITE_OK on success (or 0 after fixes). ok, sorry > Put dots at the end of sentences. > Lets use struct prefix. > Lets pass already const char*. AFAIK all routine connected with MsgPack > accepts const char *, but not void *. > > == NULL > Don’t use double assignments. > == NULL > sqlite3ValueNew > Just return 0 (in fact, they are the same thing). done Here is a full diff commit c4e932a313cc7cd01db611856cf277df9e73305d Author: AKhatskevich Date:   Fri May 18 13:56:53 2018 +0300     sql: fix decode analyze sample     Analyze samples are encoded with msgpack and should be decoded as a     msgpack.     sqlite3Stat4Column sqlite-style function converted to sql_stat4_column     Tarantool-styled.     Closes #2860 diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 878daa8df..837371aa7 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -4383,7 +4383,26 @@ 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 **); + +/** + * Extract the col_num-th column from the record.  Write + * the column value into *res.  If *res is initially NULL + * then a new sqlite3_value object is allocated. + * + * If *res is initially NULL then the caller is responsible for + * ensuring that the value written into *res is eventually + * freed. + * + * @param db Database handle. + * @param record Pointer to buffer containing record. + * @param col_num Column to extract. + * @param[out] res Extracted value. + * + * @retval -1 on error or 0. + */ +int +sql_stat4_column(struct sqlite3 *db, const char *record, int col_num, +         sqlite3_value **res);  char sqlite3IndexColumnAffinity(sqlite3 *, Index *, int);  /* diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index f408b7701..017109359 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -1588,56 +1588,31 @@ 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 char *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. */ +    struct Mem *mem = *res; +    /* Typecast byte array. */ +    const char *a = record; +    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 == NULL) { +        mem = sqlite3ValueNew(db); +        *res = mem; +        if (mem == NULL) { +            diag_set(OutOfMemory, sizeof(Mem), "sqlite3ValueNew", +                 "mem"); +            return -1; +        }      } -    sqlite3VdbeSerialGet(&a[iField - szField], t, pMem); -    return SQLITE_OK; +    sqlite3VdbeMsgpackGet((const unsigned char *) a, mem); +    return 0;  }  /* diff --git a/src/box/sql/where.c b/src/box/sql/where.c index f017384b5..4bbd39a73 100644 --- a/src/box/sql/where.c +++ b/src/box/sql/where.c @@ -1245,8 +1245,8 @@ whereRangeSkipScanEst(Parse * pParse,        /* Parsing & code generating context */          struct index_sample *samples = index->def->opts.stat->samples;          uint32_t sample_count = index->def->opts.stat->sample_count;          for (i = 0; rc == SQLITE_OK && i < (int) sample_count; i++) { -            rc = sqlite3Stat4Column(db, samples[i].sample_key, -                        samples[i].key_size, nEq, &pVal); +            rc = sql_stat4_column(db, samples[i].sample_key, 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..13cef16c8 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(23)  --!./tcltestrunner.lua  -- 2013-09-05 @@ -388,5 +388,71 @@ test:do_execsql_test(          --      }) +-- 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); +]]) + +test:do_execsql_test( +    "7.1", +    [[ +        SELECT name FROM people WHERE height>=180 order by name; +    ]], +    {"David","Jack","Patrick","Quiana","Xavier"}) + +test:do_execsql_test( +    "7.2", +    [[ +        EXPLAIN QUERY PLAN SELECT name FROM people WHERE height>=180; +    ]], +    {0,0,0,"SCAN TABLE PEOPLE"}) + +test:do_execsql_test( +    "7.3", +    [[ +        ANALYZE; +        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()