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 AA52220E58 for ; Wed, 12 Dec 2018 08:21:24 -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 8LRk00wksx4N for ; Wed, 12 Dec 2018 08:21:24 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 2644C207EA for ; Wed, 12 Dec 2018 08:21:23 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: set column types for EXPLAIN and PRAGMA References: From: Vladislav Shpilevoy Message-ID: Date: Wed, 12 Dec 2018 16:21:16 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: imeevma@tarantool.org, tarantool-patches@freelists.org Hi! Thanks for the patch! See 3 comments below, review fixes at the end of the email and on the branch. On 11/12/2018 23:41, imeevma@tarantool.org wrote: > Currently, EXPLAIN and PRAGMA do not set the column types for the > result. This is incorrect, since any returned row must have a > column type. This patch defines the types for these columns. > > Closes #3832 > --- > https://github.com/tarantool/tarantool/issues/3832 > https://github.com/tarantool/tarantool/tree/imeevma/gh-3832-no-column-types > > src/box/execute.c | 5 +- > src/box/sql/pragma.c | 29 +++---- > src/box/sql/pragma.h | 214 +++++++++++++++++++++++++++++++++-------------- > src/box/sql/prepare.c | 52 ++++++++---- > test/sql/iproto.result | 69 +++++++++++++++ > test/sql/iproto.test.lua | 18 +++- > 6 files changed, 295 insertions(+), 92 deletions(-) > > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index 5c35017..e1598d9 100644 > --- a/src/box/sql/pragma.c > +++ b/src/box/sql/pragma.c > @@ -112,25 +112,26 @@ sqlite3GetBoolean(const char *z, u8 dflt) > * the rest of the file if PRAGMAs are omitted from the build. > */ > > -/* > - * Set result column names for a pragma. > +/** > + * Set result column names and types for a pragma. > + * > + * @param v the query under construction. > + * @param pPragma the pragma. > */ > static void > -setPragmaResultColumnNames(Vdbe * v, /* The query under construction */ > - const PragmaName * pPragma /* The pragma */ > - ) > +setPragmaResultColumnNames(Vdbe *v, const PragmaName *pPragma) > { > u8 n = pPragma->nPragCName; > - sqlite3VdbeSetNumCols(v, n == 0 ? 1 : n); > - if (n == 0) { > - sqlite3VdbeSetColName(v, 0, COLNAME_NAME, pPragma->zName, > + assert(n > 0); > + sqlite3VdbeSetNumCols(v, n); > + int i, j; > + for (i = 0, j = pPragma->iPragCName; i < n; i++, j++) { > + /* Value of j is index of column name in array */ > + sqlite3VdbeSetColName(v, i, COLNAME_NAME, pragCName[j++], > + SQLITE_STATIC); > + /* Value of j is index of column type in array */ > + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, pragCName[j], > SQLITE_STATIC); > - } else { > - int i, j; > - for (i = 0, j = pPragma->iPragCName; i < n; i++, j++) { > - sqlite3VdbeSetColName(v, i, COLNAME_NAME, pragCName[j], > - SQLITE_STATIC); > - } > } > } > 1. You rewrote this function almost completely. Usually in such a case we reformat the function into Tarantool code style. > diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h > index e608016..c6c3e63 100644 > --- a/src/box/sql/pragma.h > +++ b/src/box/sql/pragma.h > @@ -27,50 +27,142 @@ > #define PragFlg_SchemaOpt 0x40 /* Schema restricts name search if present */ > #define PragFlg_SchemaReq 0x80 /* Schema required - "main" is default */ > > -/* Names of columns for pragmas that return multi-column result > - * or that return single-column results where the name of the > - * result column is different from the name of the pragma > +/** > + * Column names and types for pragmas. The type of the column is > + * the following value after its name. > */ > static const char *const pragCName[] = { > /* Used by: table_info */ > /* 0 */ "cid", > - /* 1 */ "name", > - /* 2 */ "type", > - /* 3 */ "notnull", > - /* 4 */ "dflt_value", > - /* 5 */ "pk", > + /* 1 */ "INTEGER", > + /* 2 */ "name", > + /* 3 */ "TEXT", > + /* 4 */ "type", > + /* 3 */ "TEXT", > + /* 6 */ "notnull", > + /* 1 */ "INTEGER", > + /* 8 */ "dflt_value", > + /* 9 */ "TEXT", > + /* 10 */ "pk", > + /* 11 */ "INTEGER", > /* Used by: stats */ > - /* 6 */ "table", > - /* 7 */ "index", > - /* 8 */ "width", > - /* 9 */ "height", > + /* 12 */ "table", > + /* 13 */ "TEXT", > + /* 14 */ "index", > + /* 15 */ "TEXT", > + /* 16 */ "width", > + /* 17 */ "INTEGER", > + /* 18 */ "height", > + /* 19 */ "INTEGER", 2. I see that sql_pragma_table_stats returns rows of different types of the same columns. First row has 4 columns with types "ssii", other rows have 3 columns "sii". I think, all rows should have the same number of columns of the same types. Also, the first row actually duplicates the second - they both describe the primary index. Please, consult server team chat what to do with it. I think, that we should always return 3 columns "sii": index name, avg tuple size, index size. So we just should delete the current first row. > /* Used by: index_info */ > - /* 10 */ "seqno", > - /* 11 */ "cid", > - /* 12 */ "name", > - /* 13 */ "desc", > - /* 14 */ "coll", > - /* 15 */ "type", > + /* 20 */ "seqno", > + /* 21 */ "INTEGER", > + /* 22 */ "cid", > + /* 23 */ "INTEGER", > + /* 24 */ "name", > + /* 25 */ "TEXT", > + /* 26 */ "desc", > + /* 27 */ "INTEGER", > + /* 28 */ "coll", > + /* 29 */ "TEXT", > + /* 30 */ "type", > + /* 31 */ "TEXT", > /* Used by: index_list */ > - /* 16 */ "seq", > - /* 17 */ "name", > - /* 18 */ "unique", > - /* 19 */ "origin", > - /* 20 */ "partial", > + /* 32 */ "seq", > + /* 33 */ "INTEGER", > + /* 34 */ "name", > + /* 35 */ "TEXT", > + /* 36 */ "unique", > + /* 37 */ "INTEGER", > + /* 38 */ "origin", > + /* 39 */ "TEXT", > + /* 40 */ "partial", > + /* 41 */ "INTEGER", > /* Used by: collation_list */ > - /* 21 */ "seq", > - /* 22 */ "name", > + /* 42 */ "seq", > + /* 43 */ "INTEGER", > + /* 44 */ "name", > + /* 45 */ "TEXT", > /* Used by: foreign_key_list */ > - /* 23 */ "id", > - /* 24 */ "seq", > - /* 25 */ "table", > - /* 26 */ "from", > - /* 27 */ "to", > - /* 28 */ "on_update", > - /* 29 */ "on_delete", > - /* 30 */ "match", > + /* 46 */ "id", > + /* 47 */ "INTEGER", > + /* 48 */ "seq", > + /* 49 */ "INTEGER", > + /* 50 */ "table", > + /* 51 */ "TEXT", > + /* 52 */ "from", > + /* 53 */ "TEXT", > + /* 54 */ "to", > + /* 55 */ "TEXT", > + /* 56 */ "on_update", > + /* 57 */ "TEXT", > + /* 58 */ "on_delete", > + /* 59 */ "TEXT", > + /* 60 */ "match", > + /* 61 */ "TEXT", > /* Used by: busy_timeout */ > - /* 31 */ "timeout", > + /* 62 */ "timeout", > + /* 63 */ "INTEGER", > + /* Used by: case_sensitive_like */ > + /* 64 */ "case_sensitive_like", > + /* 65 */ "INTEGER", 3. I do not see where case_sensitive_like returns anything. Looks like it is has 'void' return type. So it should not have any out names/types. The same about all other pragmas below. Or am I wrong? > + /* Used by: count_changes */ > + /* 66 */ "count_changes", > + /* 67 */ "INTEGER", > + /* Used by: defer_foreign_keys */ > + /* 68 */ "defer_foreign_keys", > + /* 69 */ "INTEGER", > + /* Used by: full_column_names */ > + /* 70 */ "full_column_names", > + /* 71 */ "INTEGER", > + /* Used by: parser_trace */ > + /* 72 */ "parser_trace", > + /* 73 */ "INTEGER", > + /* Used by: query_only */ > + /* 74 */ "query_only", > + /* 75 */ "INTEGER", > + /* Used by: read_uncommitted */ > + /* 76 */ "read_uncommitted", > + /* 77 */ "INTEGER", > + /* Used by: recursive_triggers */ > + /* 78 */ "recursive_triggers", > + /* 79 */ "INTEGER", > + /* Used by: reverse_unordered_selects */ > + /* 80 */ "reverse_unordered_selects", > + /* 81 */ "INTEGER", > + /* Used by: select_trace */ > + /* 82 */ "select_trace", > + /* 83 */ "INTEGER", > + /* Used by: short_column_names */ > + /* 84 */ "short_column_names", > + /* 85 */ "INTEGER", > + /* Used by: sql_compound_select_limit */ > + /* 86 */ "sql_compound_select_limit", > + /* 87 */ "INTEGER", > + /* Used by: sql_default_engine */ > + /* 88 */ "sql_default_engine", > + /* 89 */ "INTEGER", > + /* Used by: sql_trace */ > + /* 90 */ "sql_trace", > + /* 91 */ "INTEGER", > + /* Used by: vdbe_addoptrace */ > + /* 92 */ "vdbe_addoptrace", > + /* 93 */ "INTEGER", > + /* Used by: vdbe_debug */ > + /* 94 */ "vdbe_debug", > + /* 95 */ "INTEGER", > + /* Used by: vdbe_eqp */ > + /* 96 */ "vdbe_eqp", > + /* 97 */ "INTEGER", > + /* Used by: vdbe_listing */ > + /* 98 */ "vdbe_listing", > + /* 99 */ "INTEGER", > + /* Used by: vdbe_trace */ > + /* 100 */ "vdbe_trace", > + /* 101 */ "INTEGER", > + /* Used by: where_trace */ > + /* 102 */ "where_trace", > + /* 103 */ "INTEGER", > }; ======================================================= commit 2b019a9be547484036a8e666a2cf8b1941ce0b30 Author: Vladislav Shpilevoy Date: Wed Dec 12 16:19:27 2018 +0300 Review fixes diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index e1598d951..ef80bf801 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -112,25 +112,19 @@ sqlite3GetBoolean(const char *z, u8 dflt) * the rest of the file if PRAGMAs are omitted from the build. */ -/** - * Set result column names and types for a pragma. - * - * @param v the query under construction. - * @param pPragma the pragma. - */ +/** Set result column names and types for a pragma. */ static void -setPragmaResultColumnNames(Vdbe *v, const PragmaName *pPragma) +vdbe_set_pragma_result_columns(struct Vdbe *v, const struct PragmaName *pragma) { - u8 n = pPragma->nPragCName; + int n = pragma->nPragCName; assert(n > 0); sqlite3VdbeSetNumCols(v, n); - int i, j; - for (i = 0, j = pPragma->iPragCName; i < n; i++, j++) { + for (int i = 0, j = pragma->iPragCName; i < n; ++i) { /* Value of j is index of column name in array */ sqlite3VdbeSetColName(v, i, COLNAME_NAME, pragCName[j++], SQLITE_STATIC); /* Value of j is index of column type in array */ - sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, pragCName[j], + sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, pragCName[j++], SQLITE_STATIC); } } @@ -461,17 +455,15 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ } /* Register the result column names for pragmas that return results */ if ((pPragma->mPragFlg & PragFlg_NoColumns) == 0 - && ((pPragma->mPragFlg & PragFlg_NoColumns1) == 0 || zRight == 0) - ) { - setPragmaResultColumnNames(v, pPragma); - } + && ((pPragma->mPragFlg & PragFlg_NoColumns1) == 0 || zRight == 0)) + vdbe_set_pragma_result_columns(v, pPragma); /* Jump to the appropriate pragma handler */ switch (pPragma->ePragTyp) { #ifndef SQLITE_OMIT_FLAG_PRAGMAS case PragTyp_FLAG:{ if (zRight == 0) { - setPragmaResultColumnNames(v, pPragma); + vdbe_set_pragma_result_columns(v, pPragma); returnSingleInt(v, (user_session-> sql_flags & pPragma->iArg) !=