From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D14BE2D5B7 for ; Fri, 12 Oct 2018 07:19:13 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id atoNL6JMYxsz for ; Fri, 12 Oct 2018 07:19:13 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6500B2D5B6 for ; Fri, 12 Oct 2018 07:19:13 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 5/6] sql: return result-set type via IProto From: "n.pettik" In-Reply-To: <4c4651e2-ef29-6d82-a9de-25e67ba54ce5@tarantool.org> Date: Fri, 12 Oct 2018 14:19:10 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <4c4651e2-ef29-6d82-a9de-25e67ba54ce5@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >> @@ -3992,6 +4008,18 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * = pExpr, int target) >> break; >> } >> + if (pDef->ret_type !=3D AFFINITY_UNDEFINED) { >> + pExpr->affinity =3D pDef->ret_type; >> + } else { >> + /* >> + * Otherwise, use first arg as >> + * expression affinity. >> + */ >> + if (pFarg && pFarg->nExpr > 0) { >> + pExpr->affinity =3D >> + = pFarg->a[0].pExpr->affinity; >> + } >=20 > 1. How is it possible that function return type is undefined if > on the first commits it was forbidden? It wasn=E2=80=99t. 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 =E2=80=98any=E2=80=99. For instance, such = functions as MAX()/MIN() can return any type depending on their arguments. Yep, I=E2=80=99d = rather agree that ANY is more suitable here, but we don=E2=80=99t 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 =3D pEList->a[i].pExpr; >> if (NEVER(p =3D=3D 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); >> + } >=20 > 2. Why do you set types as names? I thought it is OK: array containing column names also can fit their = types. pColName =3D &(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. >=20 >> if (pEList->a[i].zName) { >> char *zName =3D 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); >> ]], { >> -- >> - >> + 4 >=20 > 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. >=20 > I think, you should merge these affinity manipulations into the > previous commit. NP, see new patch below. >=20 >> -- >> }) >> 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] >=20 > 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; =20 for (int i =3D 0; i < column_count; ++i) { - size_t size =3D mp_sizeof_map(1) + - mp_sizeof_uint(IPROTO_FIELD_NAME); + size_t size =3D mp_sizeof_map(2) + + mp_sizeof_uint(IPROTO_FIELD_NAME) + + mp_sizeof_uint(IPROTO_FIELD_TYPE); const char *name =3D sqlite3_column_name(stmt, i); + const char *type =3D 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 !=3D NULL); size +=3D mp_sizeof_str(strlen(name)); + size +=3D mp_sizeof_str(strlen(type)); char *pos =3D (char *) obuf_alloc(out, size); if (pos =3D=3D NULL) { diag_set(OutOfMemory, size, "obuf_alloc", = "pos"); return -1; } - pos =3D mp_encode_map(pos, 1); + pos =3D mp_encode_map(pos, 2); pos =3D mp_encode_uint(pos, IPROTO_FIELD_NAME); pos =3D mp_encode_str(pos, name, strlen(name)); + pos =3D mp_encode_uint(pos, IPROTO_FIELD_TYPE); + pos =3D 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 =3D 0, + IPROTO_FIELD_TYPE =3D 1, }; =20 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 =3D 0; i < count; ++i) { uint32_t map_size =3D mp_decode_map(data); - /* Only IPROTO_FIELD_NAME is available. */ - assert(map_size =3D=3D 1); + assert(map_size =3D=3D 2); (void) map_size; uint32_t key =3D mp_decode_uint(data); assert(key =3D=3D IPROTO_FIELD_NAME); @@ -655,6 +654,11 @@ netbox_decode_metadata(struct lua_State *L, const = char **data) const char *str =3D mp_decode_str(data, &len); lua_pushlstring(L, str, len); lua_setfield(L, -2, "name"); + key =3D mp_decode_uint(data); + assert(key =3D=3D IPROTO_FIELD_TYPE); + const char *type =3D 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 =3D pEList->a[i].pExpr; if (NEVER(p =3D=3D 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 =3D 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); =20 +const char * +sqlite3_column_datatype(sqlite3_stmt *, int N); + const char * sqlite3_errmsg(sqlite3 *); =20 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 =20 /* * 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); } =20 +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 !=3D 0); - assert(var =3D=3D COLNAME_NAME); + assert(var =3D=3D COLNAME_NAME || var =3D=3D COLNAME_DECLTYPE); pColName =3D &(p->aColName[idx + var * p->nResColumn]); rc =3D sqlite3VdbeMemSetStr(pColName, zName, -1, 1, xDel); assert(rc !=3D 0 || !zName || (pColName->flags & MEM_Term) !=3D = 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 = =3D 5;') --- - metadata: - name: IDENTIFIER + type: INTEGER rows: [] ... -- netbox API errors. @@ -124,8 +128,11 @@ cn:execute('select * from test where id =3D ?', = {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 =3D = :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 =3D :1', = {1}) --- - metadata: - name: ID + type: INTEGER - name: A + type: NUMERIC - name: B + type: TEXT rows: - [1, 2, '3'] ... @@ -518,8 +559,11 @@ res =3D 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]