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

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

>> @@ -3992,6 +4008,18 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>>  				break;
>>  			}
>>  +			if (pDef->ret_type != AFFINITY_UNDEFINED) {
>> +				pExpr->affinity = pDef->ret_type;
>> +			} else {
>> +				/*
>> +				 * Otherwise, use first arg as
>> +				 * expression affinity.
>> +				 */
>> +				if (pFarg && pFarg->nExpr > 0) {
>> +					pExpr->affinity =
>> +						pFarg->a[0].pExpr->affinity;
>> +				}
> 
> 1. How is it possible that function return type is undefined if
> on the first commits it was forbidden?

It wasn’t. For example some of built-in functions are declared with that type:
min/max/total/sum etc.

> Assuming that void type
> is implied, I wonder why do you use first argument type as a
> function's one?

Not void type, but rather ‘any’. For instance, such functions as MAX()/MIN()
can return any type depending on their arguments. Yep, I’d rather agree that
ANY is more suitable here, but we don’t have such affinity. If you suggest better
way, I will do it. Note that underlined and blob affinities now are unacceptable.
I suggest to simply wait until affinity will be eliminated (I hope it is going to be
next chapter after this patch).

>> +			}
>>  			/* Attempt a direct implementation of the built-in COALESCE() and
>>  			 * IFNULL() functions.  This avoids unnecessary evaluation of
>>  			 * arguments past the first non-NULL argument.
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index 849c0f871..d6e04525b 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -1748,6 +1748,28 @@ generateColumnNames(Parse * pParse,	/* Parser context */
>>  		p = pEList->a[i].pExpr;
>>  		if (NEVER(p == 0))
>>  			continue;
>> +		switch (p->affinity) {
>> +		case AFFINITY_INTEGER:
>> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
>> +					      SQLITE_TRANSIENT);
>> +			break;
>> +		case AFFINITY_REAL:
>> +		case AFFINITY_NUMERIC:
>> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
>> +					      SQLITE_TRANSIENT);
>> +			break;
>> +		case AFFINITY_TEXT:
>> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT",
>> +					      SQLITE_TRANSIENT);
>> +			break;
>> +		case AFFINITY_BLOB:
>> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
>> +					      SQLITE_TRANSIENT);
>> +			break;
>> +		default:
>> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
>> +					      SQLITE_TRANSIENT);
>> +		}
> 
> 2. Why do you set types as names?

I thought it is OK: array containing column names also can fit their types.
pColName = &(p->aColName[idx + var * p->nResColumn]);

> Moreover, below sqlite3VdbeSetColName
> is done again with a real column name, so this code does nothing.

It is easy to check that code is not dead and works: just comment it
and iproto.test.lua starts to fail.

> 
>>  		if (pEList->a[i].zName) {
>>  			char *zName = pEList->a[i].zName;
>>  			sqlite3VdbeSetColName(v, i, COLNAME_NAME, zName,
>> diff --git a/test/sql-tap/in4.test.lua b/test/sql-tap/in4.test.lua
>> index 70fb207fd..ef426b092 100755
>> --- a/test/sql-tap/in4.test.lua
>> +++ b/test/sql-tap/in4.test.lua
>> @@ -673,7 +673,7 @@ test:do_execsql_test(
>>          SELECT c FROM t4b WHERE +b IN (a);
>>      ]], {
>>          -- <in4-4.19>
>> -
>> +        4
> 
> 3. Why does this patch change behavior? In the title I see
> "return result-set type via IProto" so this patch is supposed
> to just return some existing info.
> 
> I think, you should merge these affinity manipulations into the
> previous commit.

NP, see new patch below.

> 
>>          -- </in4-4.19>
>>      })
>>  diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index d46df2a26..16ffd0991 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> @@ -151,8 +161,11 @@ cn:execute('select ?, ?, ?', {1, 2, 3})
>>  ---
>>  - metadata:
>>    - name: '?'
>> +    type: UNKNOWN
>>    - name: '?'
>> +    type: UNKNOWN
>>    - name: '?'
>> +    type: UNKNOWN
>>    rows:
>>    - [1, 2, 3]
> 
> 4. Why is the type 'UNKNOWN' if the value is integer?

It is how bindings work.

> Also, I
> partially agree with Kostja - UNKNOWN is not a type. But ANY
> is not an option too - from SQL we can return only scalar types.

So that's a deal?

> My review fixes (for out of 80 symbols):

Thx, applied as usually.

Full diff of this patch:

diff --git a/src/box/execute.c b/src/box/execute.c
index 24459b4b9..a5326fc53 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -528,9 +528,11 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
                return -1;
 
        for (int i = 0; i < column_count; ++i) {
-               size_t size = mp_sizeof_map(1) +
-                             mp_sizeof_uint(IPROTO_FIELD_NAME);
+               size_t size = mp_sizeof_map(2) +
+                             mp_sizeof_uint(IPROTO_FIELD_NAME) +
+                             mp_sizeof_uint(IPROTO_FIELD_TYPE);
                const char *name = sqlite3_column_name(stmt, i);
+               const char *type = sqlite3_column_datatype(stmt, i);
                /*
                 * Can not fail, since all column names are
                 * preallocated during prepare phase and the
@@ -538,14 +540,17 @@ sql_get_description(struct sqlite3_stmt *stmt, struct obuf *out,
                 */
                assert(name != NULL);
                size += mp_sizeof_str(strlen(name));
+               size += mp_sizeof_str(strlen(type));
                char *pos = (char *) obuf_alloc(out, size);
                if (pos == NULL) {
                        diag_set(OutOfMemory, size, "obuf_alloc", "pos");
                        return -1;
                }
-               pos = mp_encode_map(pos, 1);
+               pos = mp_encode_map(pos, 2);
                pos = mp_encode_uint(pos, IPROTO_FIELD_NAME);
                pos = mp_encode_str(pos, name, strlen(name));
+               pos = mp_encode_uint(pos, IPROTO_FIELD_TYPE);
+               pos = mp_encode_str(pos, type, strlen(type));
        }
        return 0;
 }
diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
index f571375ee..770784004 100644
--- a/src/box/iproto_constants.h
+++ b/src/box/iproto_constants.h
@@ -123,6 +123,7 @@ enum iproto_key {
  */
 enum iproto_metadata_key {
        IPROTO_FIELD_NAME = 0,
+       IPROTO_FIELD_TYPE = 1,
 };
 
 enum iproto_ballot_key {
diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index a928a4cf2..4d7d8c6b2 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -644,8 +644,7 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
        lua_createtable(L, count, 0);
        for (uint32_t i = 0; i < count; ++i) {
                uint32_t map_size = mp_decode_map(data);
-               /* Only IPROTO_FIELD_NAME is available. */
-               assert(map_size == 1);
+               assert(map_size == 2);
                (void) map_size;
                uint32_t key = mp_decode_uint(data);
                assert(key == IPROTO_FIELD_NAME);
@@ -655,6 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const char **data)
                const char *str = mp_decode_str(data, &len);
                lua_pushlstring(L, str, len);
                lua_setfield(L, -2, "name");
+               key = mp_decode_uint(data);
+               assert(key == IPROTO_FIELD_TYPE);
+               const char *type = mp_decode_str(data, &len);
+               lua_pushlstring(L, type, len);
+               lua_setfield(L, -2, "type");
                lua_rawseti(L, -2, i + 1);
        }
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 505c0616c..3dcce078a 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1697,6 +1697,28 @@ generateColumnNames(Parse * pParse,      /* Parser context */
                p = pEList->a[i].pExpr;
                if (NEVER(p == 0))
                        continue;
+               switch (p->affinity) {
+               case AFFINITY_INTEGER:
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
+                                             SQLITE_TRANSIENT);
+                       break;
+               case AFFINITY_REAL:
+               case AFFINITY_NUMERIC:
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
+                                             SQLITE_TRANSIENT);
+                       break;
+               case AFFINITY_TEXT:
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT",
+                                             SQLITE_TRANSIENT);
+                       break;
+               case AFFINITY_BLOB:
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
+                                             SQLITE_TRANSIENT);
+                       break;
+               default:
+                       sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
+                                             SQLITE_TRANSIENT);
+               }
                if (pEList->a[i].zName) {
                        char *zName = pEList->a[i].zName;
                        sqlite3VdbeSetColName(v, i, COLNAME_NAME, zName,
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 82bc343e3..6b1c7c987 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -677,6 +677,9 @@ sqlite3_column_count(sqlite3_stmt * pStmt);
 const char *
 sqlite3_column_name(sqlite3_stmt *, int N);
 
+const char *
+sqlite3_column_datatype(sqlite3_stmt *, int N);
+
 const char *
 sqlite3_errmsg(sqlite3 *);
 
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index d5da5d571..f1fcb382b 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -150,12 +150,8 @@ struct SubProgram {
 #ifdef SQLITE_ENABLE_COLUMN_METADATA
 #define COLNAME_N        5     /* Number of COLNAME_xxx symbols */
 #else
-#ifdef SQLITE_OMIT_DECLTYPE
-#define COLNAME_N      1       /* Store only the name */
-#else
 #define COLNAME_N      2       /* Store the name and decltype */
 #endif
-#endif
 
 /*
  * The following macro converts a relative address in the p2 field
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 04e60a079..b16e916e9 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -1111,6 +1111,13 @@ sqlite3_column_name(sqlite3_stmt * pStmt, int N)
                          COLNAME_NAME);
 }
 
+const char *
+sqlite3_column_datatype(sqlite3_stmt *pStmt, int N)
+{
+       return columnName(pStmt, N, (const void *(*)(Mem *))sqlite3_value_text,
+                         COLNAME_DECLTYPE);
+}
+
 /*
  * Constraint:  If you have ENABLE_COLUMN_METADATA then you must
  * not define OMIT_DECLTYPE.
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 6e42d0596..4bc81bd03 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2168,7 +2168,7 @@ sqlite3VdbeSetColName(Vdbe * p,                   /* Vdbe being configured */
                return SQLITE_NOMEM_BKPT;
        }
        assert(p->aColName != 0);
-       assert(var == COLNAME_NAME);
+       assert(var == COLNAME_NAME || var == COLNAME_DECLTYPE);
        pColName = &(p->aColName[idx + var * p->nResColumn]);
        rc = sqlite3VdbeMemSetStr(pColName, zName, -1, 1, xDel);
        assert(rc != 0 || !zName || (pColName->flags & MEM_Term) != 0);
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 2bcfdb7db..cb993f8ce 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -79,6 +79,7 @@ select_res
 ---
 - metadata:
   - name: '1'
+    type: INTEGER
   rows:
   - [1]
 ...
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index d46df2a26..16ffd0991 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -55,8 +55,11 @@ ret
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: NUMERIC
   - name: B
+    type: TEXT
   rows:
   - [1, 2, '3']
   - [4, 5, '6']
@@ -97,6 +100,7 @@ cn:execute('select id as identifier from test where a = 5;')
 ---
 - metadata:
   - name: IDENTIFIER
+    type: INTEGER
   rows: []
 ...
 -- netbox API errors.
@@ -124,8 +128,11 @@ cn:execute('select * from test where id = ?', {1})
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: NUMERIC
   - name: B
+    type: TEXT
   rows:
   - [1, 2, '3']
 ...
@@ -142,8 +149,11 @@ cn:execute('select * from test where id = :value', parameters)
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: NUMERIC
   - name: B
+    type: TEXT
   rows:
   - [1, 2, '3']
 ...
@@ -151,8 +161,11 @@ cn:execute('select ?, ?, ?', {1, 2, 3})
 ---
 - metadata:
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   rows:
   - [1, 2, 3]
 ...
@@ -178,8 +191,11 @@ cn:execute('select ?, :value1, @value2', parameters)
 ---
 - metadata:
   - name: '?'
+    type: UNKNOWN
   - name: :value1
+    type: UNKNOWN
   - name: '@value2'
+    type: UNKNOWN
   rows:
   - [10, 11, 12]
 ...
@@ -217,13 +233,21 @@ cn:execute('select :value3, ?, :value1, ?, ?, @value2, ?, :value3', parameters)
 ---
 - metadata:
   - name: :value3
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: :value1
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '@value2'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: :value3
+    type: UNKNOWN
   rows:
   - [1, 2, 3, 4, 5, 6, null, 1]
 ...
@@ -235,10 +259,15 @@ cn:execute('select ?, ?, ?, ?, ?', {'abc', -123.456, msgpack.NULL, true, false})
 ---
 - metadata:
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   - name: '?'
+    type: UNKNOWN
   rows:
   - ['abc', -123.456, null, 1, 0]
 ...
@@ -247,7 +276,9 @@ cn:execute('select ? as kek, ? as kek2', {1, 2})
 ---
 - metadata:
   - name: KEK
+    type: UNKNOWN
   - name: KEK2
+    type: UNKNOWN
   rows:
   - [1, 2]
 ...
@@ -344,9 +375,13 @@ cn:execute('select * from test2')
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: INTEGER
   - name: B
+    type: INTEGER
   - name: C
+    type: INTEGER
   rows:
   - [1, 1, 1, 1]
 ...
@@ -494,8 +529,11 @@ cn:execute('select $2, $1, $3', parameters)
 ---
 - metadata:
   - name: $2
+    type: UNKNOWN
   - name: $1
+    type: UNKNOWN
   - name: $3
+    type: UNKNOWN
   rows:
   - [22, 11, 33]
 ...
@@ -503,8 +541,11 @@ cn:execute('select * from test where id = :1', {1})
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: NUMERIC
   - name: B
+    type: TEXT
   rows:
   - [1, 2, '3']
 ...
@@ -518,8 +559,11 @@ res = cn:execute('select * from test')
 res.metadata
 ---
 - - name: ID
+    type: INTEGER
   - name: A
+    type: NUMERIC
   - name: B
+    type: TEXT
 ...
 box.sql.execute('drop table test')
 ---
@@ -567,8 +611,11 @@ future4:wait_result()
 ---
 - metadata:
   - name: ID
+    type: INTEGER
   - name: A
+    type: INTEGER
   - name: B
+    type: INTEGER
   rows:
   - [1, 1, 1]
   - [2, 2, 2]