Tarantool development patches archive
 help / color / mirror / Atom feed
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) !=

      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