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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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@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;