[tarantool-patches] Re: [PATCH 5/6] sql: return result-set type via IProto

[n.pettik <korablev@tarantool.org>]

>>> 4. Why is the type 'UNKNOWN' if the value is integer?
>> It is how bindings work.
> 
> No, bindings as an API have nothing to do with types.
> It is Vdbe's issue, into which the values are inserted,
> and it should detect their type. It is not affinity
> issue, but Vdbe changing after its compilation. Anyway,
> a user placed a number as a parameter and got UNKNOWN -
> it is extra weird.
> 
> By the way, '? as <column_name>' works ok - the result

Yep, but we can’t substitute ‘?' with real column name, only
with constant alias (at least I didn’t manage to find the way
how to do it). I want to do this:

tarantool> cn:execute('select ?, ?, 3 from t1', {1, ‘id'})   
---
- metadata:
  - name: '?'
    type: UNKNOWN
  - name: '?'
    type: UNKNOWN
  - name: '3'
    type: INTEGER
  rows:
  - [1, 'id', 3]
…

But I meant not string literal “id”, but column id.
I don’t see such test case in sql/iproto.test.lua as well.
Also, I can ask similar question: why in the example above
name of bound literals 1 and ‘abc' is ‘?’ but for non-binded
literal 3 is ‘3’? I believe it was made on purpose, and I don’t
mind to display correct type of bound values but it would
look a little bit confusing (I mean real name is not substituted,
but real type is revealed).

> set contains real name instead of '?'. I think, it should
> work for types too.

You are talking about the case when name (or alias) is known
at compilation stage:

cn:execute(’select ? as param1’, {1})

It is quite different case - we don’t know type of parameter
at compilation stage.

Nevertheless, I agree with you that we should
display correct type instead of UNKNOWN.

> If you want to do it as a separate ticket, I do not mind,
> but please consult Kirill to be sure.

I think this issue rather relates to current patch,
so I’ve fixed it right here:

*Extended commit message*

    sql: return result-set type via IProto
    
    Lets evaluate an expression type during processing of expression's AST
    and code generation. It allows to calculate resulting columns data types
    and export them as IProto meta alongside with columns' names.
    Also, correct types are also returned for binding parameters as well.
    
    Note that NULL literal has type "BOOLEAN". It was made on purpose -
    different DBs interpret NULL's type in different ways: some of them
    use INT; others - VARCHAR; still others - UNKNOWN. We've decided that
    NULL is rather of type "BOOLEAN", since NULL is kind if subset of
    "BOOLEAN" values: any comparison with NULL results in neither TRUE nor
    FALSE, but in NULL.
    
    Part of #2620

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 3dcce078a..16b1044d6 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -36,6 +36,7 @@
 #include "coll.h"
 #include "sqliteInt.h"
 #include "tarantoolInt.h"
+#include "vdbeInt.h"
 #include "box/box.h"
 #include "box/coll_id_cache.h"
 #include "box/schema.h"
@@ -1687,11 +1688,19 @@ generateColumnNames(Parse * pParse,     /* Parser context */
        if (pParse->colNamesSet || db->mallocFailed)
                return;
        assert(v != 0);
+       size_t var_pos_sz =  pParse->nVar * sizeof(uint32_t);
+       uint32_t *var_pos = (uint32_t *) region_alloc(&pParse->region,
+                                                     var_pos_sz);
+       if (var_pos == NULL) {
+               diag_set(OutOfMemory, var_pos_sz, "region", "var_pos");
+               return;
+       }
        assert(pTabList != 0);
        pParse->colNamesSet = 1;
        fullNames = (user_session->sql_flags & SQLITE_FullColNames) != 0;
        shortNames = (user_session->sql_flags & SQLITE_ShortColNames) != 0;
        sqlite3VdbeSetNumCols(v, pEList->nExpr);
+       uint32_t var_count = 0;
        for (i = 0; i < pEList->nExpr; i++) {
                Expr *p;
                p = pEList->a[i].pExpr;
@@ -1715,8 +1724,21 @@ generateColumnNames(Parse * pParse,      /* Parser context */
                        sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
                                              SQLITE_TRANSIENT);
                        break;
-               default:
-                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
+               default: ;
+                       char *type;
+                       /*
+                        * For variables we set BOOLEAN type since
+                        * unassigned bindings will be replaced
+                        * with NULL automatically, i.e. without
+                        * explicit call of sql_bind_value().
+                        */
+                       if (p->op == TK_VARIABLE) {
+                               var_pos[var_count++] = i;
+                               type = "BOOLEAN";
+                       } else {
n:tarantool n.pettik$ git diff
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 3dcce078a..16b1044d6 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -36,6 +36,7 @@
 #include "coll.h"
 #include "sqliteInt.h"
 #include "tarantoolInt.h"
+#include "vdbeInt.h"
 #include "box/box.h"
 #include "box/coll_id_cache.h"
 #include "box/schema.h"
@@ -1687,11 +1688,19 @@ generateColumnNames(Parse * pParse,     /* Parser context */
        if (pParse->colNamesSet || db->mallocFailed)
                return;
        assert(v != 0);
+       size_t var_pos_sz =  pParse->nVar * sizeof(uint32_t);
+       uint32_t *var_pos = (uint32_t *) region_alloc(&pParse->region,
+                                                     var_pos_sz);
+       if (var_pos == NULL) {
+               diag_set(OutOfMemory, var_pos_sz, "region", "var_pos");
+               return;
+       }
        assert(pTabList != 0);
        pParse->colNamesSet = 1;
        fullNames = (user_session->sql_flags & SQLITE_FullColNames) != 0;
        shortNames = (user_session->sql_flags & SQLITE_ShortColNames) != 0;
        sqlite3VdbeSetNumCols(v, pEList->nExpr);
+       uint32_t var_count = 0;
        for (i = 0; i < pEList->nExpr; i++) {
                Expr *p;
                p = pEList->a[i].pExpr;
@@ -1715,8 +1724,21 @@ generateColumnNames(Parse * pParse,      /* Parser context */
                        sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
                                              SQLITE_TRANSIENT);
                        break;
-               default:
-                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
+               default: ;
+                       char *type;
+                       /*
+                        * For variables we set BOOLEAN type since
+                        * unassigned bindings will be replaced
+                        * with NULL automatically, i.e. without
+                        * explicit call of sql_bind_value().
+                        */
+                       if (p->op == TK_VARIABLE) {
+                               var_pos[var_count++] = i;
+                               type = "BOOLEAN";
+                       } else {
+                               type = "UNKNOWN";
+                       }
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, type,
                                              SQLITE_TRANSIENT);
                }
                if (pEList->a[i].zName) {
@@ -1760,6 +1782,16 @@ generateColumnNames(Parse * pParse,      /* Parser context */
                                              SQLITE_DYNAMIC);
                }
        }
+       if (var_count == 0)
+               return;
+       v->var_pos = (uint32_t *) malloc(var_count * sizeof(uint32_t));
+       if (v->var_pos ==  NULL) {
+               diag_set(OutOfMemory, var_count * sizeof(uint32_t),
+                        "malloc", "v->var_pos");
+               return;
+       }
+       memcpy(v->var_pos, var_pos, var_count * sizeof(uint32_t));
+       v->res_var_count = var_count;
 }
 
 /*
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 174f5980f..5ad1e0a80 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -380,6 +380,19 @@ struct Vdbe {
        char *zErrMsg;          /* Error message written here */
        VdbeCursor **apCsr;     /* One element of this array for each open cursor */
        Mem *aVar;              /* Values for the OP_Variable opcode. */
+       /**
+        * Array which contains positions of variables to be
+        * bound in resulting set of SELECT.
+        **/
+       uint32_t *var_pos;
+       /**
+        * Number of variables to be bound in result set.
+        * In other words - size of @var_pos array.
+        * For example:
+        * SELECT ?, ? WHERE id = ?;
+        * Result set consists of two binding variables.
+        */
+       uint32_t res_var_count;
        VList *pVList;          /* Name of variables */
 #ifndef SQLITE_OMIT_TRACE
        i64 startTime;          /* Time when query started - used for profiling */
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index b16e916e9..9e57af051 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1231,6 +1231,50 @@ vdbeUnbind(Vdbe * p, int i)
        return SQLITE_OK;
 }
 
+/**
+ * This function sets type for bound variable.
+ * We should bind types only for variables which occur in
+ * result set of SELECT query. For example:
+ *
+ * SELECT id, ?, ?, a WHERE id = ?;
+ *
+ * In this case we should set types only for two variables.
+ * That one which is situated under WHERE condition - is out
+ * of our interest.
+ *
+ * For named binding parameters we should propagate type
+ * for all occurrences of this parameter - since binding
+ * routine takes place only once for each DISTINCT parameter
+ * from list.
+ *
+ * @param v Current VDBE.
+ * @param position Ordinal position of binding parameter.
+ * @param type String literal representing type of binding param.
+ * @retval 0 on success.
+ */
+static int
+sql_bind_type(struct Vdbe *v, uint32_t position, const char *type)
+{
+       if (v->res_var_count < position)
+               return 0;
+       int rc = sqlite3VdbeSetColName(v, v->var_pos[position - 1],
+                                      COLNAME_DECLTYPE, type,
+                                      SQLITE_TRANSIENT);
+       const char *bind_name = v->aColName[position - 1].z;
+       if (strcmp(bind_name, "?") == 0)
+               return rc;
+       for (uint32_t i = position; i < v->res_var_count; ++i) {
+               if (strcmp(bind_name,  v->aColName[i].z) == 0) {
+                       rc = sqlite3VdbeSetColName(v, v->var_pos[i],
+                                                  COLNAME_DECLTYPE, type,
+                                                  SQLITE_TRANSIENT);
+                       if (rc != 0)
+                               return rc;
+               }
+       }
+       return 0;
+}
+
 /*
  * Bind a text or BLOB value.
  */
@@ -1251,6 +1295,8 @@ bindText(sqlite3_stmt * pStmt,    /* The statement to bind against */
                if (zData != 0) {
                        pVar = &p->aVar[i - 1];
                        rc = sqlite3VdbeMemSetStr(pVar, zData, nData, 1, xDel);
+                       if (rc == SQLITE_OK)
+                               rc = sql_bind_type(p, i, "TEXT");
                        sqlite3Error(p->db, rc);
                        rc = sqlite3ApiExit(p->db, rc);
                }
@@ -1297,6 +1343,7 @@ sqlite3_bind_double(sqlite3_stmt * pStmt, int i, double rValue)
        Vdbe *p = (Vdbe *) pStmt;
        rc = vdbeUnbind(p, i);
        if (rc == SQLITE_OK) {
+               rc = sql_bind_type(p, i, "NUMERIC");
                sqlite3VdbeMemSetDouble(&p->aVar[i - 1], rValue);
        }
        return rc;
@@ -1315,6 +1362,7 @@ sqlite3_bind_int64(sqlite3_stmt * pStmt, int i, sqlite_int64 iValue)
        Vdbe *p = (Vdbe *) pStmt;
        rc = vdbeUnbind(p, i);
        if (rc == SQLITE_OK) {
+               rc = sql_bind_type(p, i, "INTEGER");
                sqlite3VdbeMemSetInt64(&p->aVar[i - 1], iValue);
        }
        return rc;
@@ -1326,6 +1374,8 @@ sqlite3_bind_null(sqlite3_stmt * pStmt, int i)
        int rc;
        Vdbe *p = (Vdbe *) pStmt;
        rc = vdbeUnbind(p, i);
+       if (rc == SQLITE_OK)
+               rc = sql_bind_type(p, i, "BOOLEAN");
        return rc;
 }
 
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 4bc81bd03..088ac3b3e 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2780,6 +2780,7 @@ sqlite3VdbeDelete(Vdbe * p)
        }
        p->magic = VDBE_MAGIC_DEAD;
        p->db = 0;
+       free(p->var_pos);
        sqlite3DbFree(db, p);
 }
 
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 16ffd0991..e8b53bf11 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -161,11 +161,11 @@ cn:execute('select ?, ?, ?', {1, 2, 3})
 ---
 - metadata:
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - [1, 2, 3]
 ...
@@ -191,11 +191,11 @@ cn:execute('select ?, :value1, @value2', parameters)
 ---
 - metadata:
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: :value1
-    type: UNKNOWN
+    type: INTEGER
   - name: '@value2'
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - [10, 11, 12]
 ...
@@ -233,21 +233,21 @@ cn:execute('select :value3, ?, :value1, ?, ?, @value2, ?, :value3', parameters)
 ---
 - metadata:
   - name: :value3
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: :value1
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: '@value2'
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: BOOLEAN
   - name: :value3
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - [1, 2, 3, 4, 5, 6, null, 1]
 ...
@@ -259,15 +259,15 @@ cn:execute('select ?, ?, ?, ?, ?', {'abc', -123.456, msgpack.NULL, true, false})
 ---
 - metadata:
   - name: '?'
-    type: UNKNOWN
+    type: TEXT
   - name: '?'
-    type: UNKNOWN
+    type: NUMERIC
   - name: '?'
-    type: UNKNOWN
+    type: BOOLEAN
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   - name: '?'
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - ['abc', -123.456, null, 1, 0]
 ...
@@ -276,9 +276,9 @@ cn:execute('select ? as kek, ? as kek2', {1, 2})
 ---
 - metadata:
   - name: KEK
-    type: UNKNOWN
+    type: INTEGER
   - name: KEK2
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - [1, 2]
 ...
@@ -529,11 +529,11 @@ cn:execute('select $2, $1, $3', parameters)
 ---
 - metadata:
   - name: $2
-    type: UNKNOWN
+    type: INTEGER
   - name: $1
-    type: UNKNOWN
+    type: INTEGER
   - name: $3
-    type: UNKNOWN
+    type: INTEGER
   rows:
   - [22, 11, 33]
 ...