From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: imeevma@tarantool.org, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: set column types for EXPLAIN and PRAGMA
Date: Wed, 12 Dec 2018 16:21:16 +0300 [thread overview]
Message-ID: <df3da724-5e30-4f0f-796f-9ad01e7f5544@tarantool.org> (raw)
In-Reply-To: <bcaf7980a3299aa8bb81e81aa852b5402bf8e35e.1544560793.git.imeevma@gmail.com>
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 <v.shpilevoy@tarantool.org>
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) !=
prev parent reply other threads:[~2018-12-12 13:21 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-11 20:41 [tarantool-patches] " imeevma
2018-12-12 13:21 ` Vladislav Shpilevoy [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=df3da724-5e30-4f0f-796f-9ad01e7f5544@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=imeevma@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: set column types for EXPLAIN and PRAGMA' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox