Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr
Date: Wed, 30 Jan 2019 16:04:31 +0300	[thread overview]
Message-ID: <aa4d3548-87bc-d738-8c65-84a51cacf6e7@tarantool.org> (raw)
In-Reply-To: <7FD8757A-713C-4887-9268-42849FFEE3A8@tarantool.org>

Thanks for the fixes!

>>>> 7. Why did not you move this conversion to sql_expr_type? I mean
>>>> ANY -> SCALAR.
>>> Unfortunately, right now I can’t replace it with ANY. It is still used
>>> when IN operator comes with one operand (see EP_Generic flag).
>>> I have already sent patch which removes it:
>>> https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix
>>
>> I've already acked the patch. Please, hurry up Kirill Y. to push it,
>> rebase this branch, and replace ANY with SCALAR.
> 
> I had to add some other fixes to make this happen:
> 
> Changes to sql: replace affinity with field type for func:
> 
> diff --git a/test-run b/test-run
> index 02207efd2..4c3f11812 160000
> --- a/test-run
> +++ b/test-run
> @@ -1 +1 @@
> -Subproject commit 02207efd2ca44067b76c79bf012f142016d929ae
> +Subproject commit 4c3f11812c0c17f08780f31b6a748eef1126b2e2

1. ???

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index fdf00273a..e1a0dfa73 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -92,16 +111,14 @@ sql_expr_type(struct Expr *pExpr)
>                   * return INTEGER.
>                   */
>                  return FIELD_TYPE_INTEGER;
> -       }
> -       /*
> -        * In case of unary plus we shouldn't discard
> -        * type of operand (since plus always features
> -        * NUMERIC type).
> -        */
> -       if (op == TK_UPLUS) {
> +       case TK_UMINUS:
> +       case TK_UPLUS:
> +       case TK_NO:
> +       case TK_BITNOT:
>                  assert(pExpr->pRight == NULL);
> -               return pExpr->pLeft->type;
> +               return sql_expr_type(pExpr->pLeft);
>          }
> +

2. Stray empty line.

>          return pExpr->type;
>>>> ANY differs from SCALAR in only one thing - it is
>>>> able to store MP_MAP and MP_ARRAY. So I am slightly bent upon
>>>> frequency of ANY usage in SQL, wherein MAP/ARRAY does not exist.
>>> But still we can SELECT data from spaces created from Lua.
>>> For instance, _fk_constraint features type ARRAY in its format,
>>> so we can’t ignore this type even now (IMHO).
>>
>> Despite our ability to select complex types, in SQL they are all
>> mere binary data with *sub*type msgpack, not array/map type. So
>> in SQL we never truly work with anything but SCALAR.
> 
> Wait, at the stage of query compiling we don’t operate on
> any subtypes. We simply fetch space_def->fields->type,
> which may turn out to be FIELD_TYPE_MAP/ARRAY or
> whatever.

Hmm, yes, you are right. We should consider array and map too.

See 4 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index d263c1bb5..e1a0dfa73 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -81,27 +57,46 @@ sqlite3ExprAffinity(Expr * pExpr)
>  	case TK_SELECT:
>  		assert(pExpr->flags & EP_xIsSelect);
>  		el = pExpr->x.pSelect->pEList;
> -		return sqlite3ExprAffinity(el->a[0].pExpr);
> +		return sql_expr_type(el->a[0].pExpr);
>  	case TK_CAST:
>  		assert(!ExprHasProperty(pExpr, EP_IntValue));
> -		return pExpr->affinity;
> +		return pExpr->type;
>  	case TK_AGG_COLUMN:
>  	case TK_COLUMN:
> +	case TK_TRIGGER:
>  		assert(pExpr->iColumn >= 0);
> -		return sqlite3TableColumnAffinity(pExpr->space_def,
> -						  pExpr->iColumn);
> +		return pExpr->space_def->fields[pExpr->iColumn].type;
>  	case TK_SELECT_COLUMN:
>  		assert(pExpr->pLeft->flags & EP_xIsSelect);
>  		el = pExpr->pLeft->x.pSelect->pEList;
> -		return sqlite3ExprAffinity(el->a[pExpr->iColumn].pExpr);
> +		return sql_expr_type(el->a[pExpr->iColumn].pExpr);
>  	case TK_PLUS:
>  	case TK_MINUS:
>  	case TK_STAR:
>  	case TK_SLASH:
> +	case TK_REM:
> +	case TK_BITAND:
> +	case TK_BITOR:
> +	case TK_LSHIFT:
> +	case TK_RSHIFT:
>  		assert(pExpr->pRight != NULL && pExpr->pLeft != NULL);
> -		enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft);
> -		enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight);
> -		return sql_affinity_result(rhs_aff, lhs_aff);
> +		enum field_type lhs_type = sql_expr_type(pExpr->pLeft);
> +		enum field_type rhs_type = sql_expr_type(pExpr->pRight);
> +		return sql_type_result(rhs_type, lhs_type);
> +	case TK_CONCAT:
> +			return FIELD_TYPE_STRING;

1. Indentation.

> +	case TK_CASE:
> +		assert(pExpr->x.pList->nExpr >= 2);
> +		/*
> +		 * CASE expression comes at least with one
> +		 * WHEN and one THEN clauses. So, first
> +		 * expression always represents WHEN
> +		 * argument, and the second one - THEN.
> +		 *
> +		 * TODO: We must ensure that all THEN clauses
> +		 * have arguments of the same type.
> +		 */
> +		return sql_expr_type(pExpr->x.pList->a[1].pExpr);
>  	case TK_LT:
>  	case TK_GT:
>  	case TK_EQ:
> @@ -252,74 +245,57 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
> -/*
> - * pExpr is a comparison expression, eg. '=', '<', IN(...) etc.
> - * idx_affinity is the affinity of an indexed column. Return true
> - * if the index with affinity idx_affinity may be used to implement
> - * the comparison in pExpr.
> +/**
> + * @param expr is a comparison expression, eg. '=', '<', IN(...) etc.
> + * @param field_type is the type of an indexed column.
> + * @retval Return true if the index with @field_type may be used to
> + * implement the comparison in expr.
>   */
> -int
> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity)
> +enum field_type
> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)

2. Returns 'bool true', but the return value type is enum field_type?
Please, change to bool.

>  {
> -	char aff = comparisonAffinity(pExpr);
> -	switch (aff) {
> -	case AFFINITY_BLOB:
> +	enum field_type type = expr_cmp_mutual_type(expr);
> +	switch (type) {
> +	case FIELD_TYPE_SCALAR:
>  		return 1;

3. 1 -> true

> -	case AFFINITY_TEXT:
> -		return idx_affinity == AFFINITY_TEXT;
> +	case FIELD_TYPE_STRING:
> +		return field_type == FIELD_TYPE_STRING;
>  	default:
> -		return sqlite3IsNumericAffinity(idx_affinity);
> +		return sql_type_is_numeric(field_type);
>  	}
>  }
>  
> @@ -2140,21 +2116,11 @@ sqlite3ExprCanBeNull(const Expr * p)
>  	}
>  }
>  
> -/*
> - * Return TRUE if the given expression is a constant which would be
> - * unchanged by OP_ApplyType with the type given in the second
> - * argument.
> - *
> - * This routine is used to determine if the OP_ApplyType operation
> - * can be omitted.  When in doubt return FALSE.  A false negative
> - * is harmless.  A false positive, however, can result in the wrong
> - * answer.
> - */
>  int
> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
> +sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)

4. The same. Anyway you changed the function name, so without
increasing the diff you can change its return value to bool,
and the values below 0 -> false, 1 -> true.

>  {
>  	u8 op;
> -	if (aff == AFFINITY_BLOB)
> +	if (type == FIELD_TYPE_SCALAR)
>  		return 1;
>  	while (p->op == TK_UPLUS || p->op == TK_UMINUS) {
>  		p = p->pLeft;

  reply	other threads:[~2019-01-30 13:04 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-28  9:34 [tarantool-patches] [PATCH 0/8] Eliminate affinity from source code Nikita Pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 1/8] sql: remove SQLITE_ENABLE_UPDATE_DELETE_LIMIT define Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:25     ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 2/8] sql: use field type instead of affinity for type_def Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 3/8] sql: remove numeric affinity Nikita Pettik
2018-12-29  9:01   ` [tarantool-patches] " Konstantin Osipov
2018-12-29 17:42   ` Vladislav Shpilevoy
2019-01-09  8:26     ` Konstantin Osipov
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-01-09  8:20   ` Konstantin Osipov
2018-12-28  9:34 ` [tarantool-patches] [PATCH 4/8] sql: replace affinity with field type for func Nikita Pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 5/8] sql: replace field type with affinity for VDBE runtime Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 6/8] sql: replace affinity with field type in struct Expr Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:39         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy [this message]
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 7/8] sql: clean-up affinity from SQL source code Nikita Pettik
2018-12-29 17:42   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-16 14:26     ` n.pettik
2019-01-22 15:41       ` Vladislav Shpilevoy
2019-01-28 16:40         ` n.pettik
2019-01-30 13:04           ` Vladislav Shpilevoy
2019-02-01 16:39             ` n.pettik
2019-02-05 15:08               ` Vladislav Shpilevoy
2019-02-05 17:46                 ` n.pettik
2018-12-28  9:34 ` [tarantool-patches] [PATCH 8/8] Remove affinity from field definition Nikita Pettik
2019-02-05 19:41 ` [tarantool-patches] Re: [PATCH 0/8] Eliminate affinity from source code Vladislav Shpilevoy
2019-02-08 13:37 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aa4d3548-87bc-d738-8c65-84a51cacf6e7@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox