[tarantool-patches] Re: [PATCH 5/6] sql: return result-set type via IProto
n.pettik
korablev at tarantool.org
Fri Oct 12 14:19:10 MSK 2018
>> @@ -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]
More information about the Tarantool-patches
mailing list