[tarantool-patches] Re: [PATCH 5/6] sql: return result-set type via IProto
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Sep 27 23:24:13 MSK 2018
See 4 comments below, my review fixes on the branch and
at the end of the email.
On 17/09/2018 23:32, Nikita Pettik wrote:
> From: Georgy Kirichenko <georgy at tarantool.org>
>
> 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.
>
> Part of #2620
> ---
> src/box/execute.c | 11 +++++++---
> src/box/iproto_constants.h | 1 +
> src/box/lua/net_box.c | 8 ++++++--
> src/box/sql/expr.c | 37 +++++++++++++++++++++++++++++++++-
> src/box/sql/select.c | 22 ++++++++++++++++++++
> src/box/sql/sqliteInt.h | 3 +++
> src/box/sql/vdbe.h | 4 ----
> src/box/sql/vdbeapi.c | 7 +++++++
> src/box/sql/vdbeaux.c | 2 +-
> test/sql-tap/in4.test.lua | 2 +-
> test/sql-tap/tkt3493.test.lua | 2 +-
> test/sql-tap/where2.test.lua | 4 ++--
> test/sql-tap/whereB.test.lua | 3 ++-
> test/sql/errinj.result | 1 +
> test/sql/iproto.result | 47 +++++++++++++++++++++++++++++++++++++++++++
> 15 files changed, 138 insertions(+), 16 deletions(-)
>
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 86029b74e..534d856eb 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -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? Assuming that void type
is implied, I wonder why do you use first argument type as a
function's one?
> + }
> /* 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? Moreover, below sqlite3VdbeSetColName
is done again with a real column name, so this code does nothing.
> 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.
> -- </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? 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.
My review fixes (for out of 80 symbols):
===============================================================
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 534d856eb..d5cd562da 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3723,10 +3723,11 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
}
case TK_COLUMN:{
int iTab = pExpr->iTable;
+ int col = pExpr->iColumn;
if (iTab < 0) {
if (pParse->ckBase > 0) {
/* Generating CHECK constraints. */
- return pExpr->iColumn + pParse->ckBase;
+ return col + pParse->ckBase;
} else {
/* Coding an expression that is part of an index where column names
* in the index refer to the table to which the index belongs
@@ -3735,10 +3736,11 @@ sqlite3ExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
}
}
pExpr->affinity =
- pExpr->space_def->fields[pExpr->iColumn].affinity;
- return sqlite3ExprCodeGetColumn(pParse, pExpr->space_def,
- pExpr->iColumn, iTab,
- target, pExpr->op2);
+ pExpr->space_def->fields[col].affinity;
+ return sqlite3ExprCodeGetColumn(pParse,
+ pExpr->space_def, col,
+ iTab, target,
+ pExpr->op2);
}
case TK_INTEGER:{
pExpr->affinity = AFFINITY_INTEGER;
More information about the Tarantool-patches
mailing list