[tarantool-patches] Re: [PATCH 7/8] sql: clean-up affinity from SQL source code

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

Thanks for the fixes!

> Still some comments and variables contain ‘affinity’ term.
> We can either accept it as a synonym to type, or fix
> it within only code-style-fix commit.

I think, we should rename. As soon as possible. Otherwise we
will have 'affinity' in our code base for ages. Together with
comments, dead code, already changed in this patch code, and
the code you remove in the next commit my text editor found
199 'affinity' usages. It is not too much. Also, I am almost
sure, that you will find some bugs during the renaming process.

> commit 53561119ee99de9d5a781c5e83c49374634c4b97
> Author: Nikita Pettik <korablev@tarantool.org>
> Date:   Sun Dec 23 18:10:11 2018 +0200
> 
>      sql: clean-up affinity from SQL source code
>      
>      Replace remains of affinity usage in SQL parser, query optimizer and
>      VDBE. Don't add affinity to field definition when table is encoded into
>      msgpack.  Remove field type <-> affinity converters, since now we can
>      operate directly on field type.
>      
>      Part of #3698

See 6 comments below.

> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index d3a8644ce..b0a595b97 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -3213,7 +3226,9 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
>          if (rLhs != rLhsOrig)
>                  sqlite3ReleaseTempReg(pParse, rLhs);
>          sqlite3ExprCachePop(pParse);
> +       sqlite3DbFree(pParse->db, aiMap);
>          VdbeComment((v, "end IN expr"));
> +       return;

1. What was wrong with the previous way of the function finalization?
I do not see any functional changes in this function, but logic of
freeing is different. Why? Because zAff should not be freed? Then please,
fix it in the previous commit. And to do not change logic of
"sqlite3ExprCodeIN_finished:" you can nullify zAff after passing it to
OP_ApplyType above.

>    sqlite3ExprCodeIN_oom_error:
>          sqlite3DbFree(pParse->db, aiMap);
>          sqlite3DbFree(pParse->db, zAff);
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 209e7bf0f..822cc40df 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1807,7 +1801,7 @@ struct Savepoint {
>    * operator is NULL.  It is added to certain comparison operators to
>    * prove that the operands are always NOT NULL.
>    */
> -#define SQLITE_KEEPNULL     0x08       /* Used by vector == or <> */
> +#define SQLITE_KEEPNULL     0x40       /* Used by vector == or <> */
>   #define SQLITE_JUMPIFNULL   0x10       /* jumps if either operand is NULL */
>   #define SQLITE_STOREP2      0x20       /* Store result in reg[P2] rather than jump */
>   #define SQLITE_NULLEQ       0x80       /* NULL=NULL */

2. Please, keep them sorted. Cuts the eye.

> @@ -2615,7 +2609,8 @@ struct Select {
>    */
>   struct SelectDest {
>          u8 eDest;               /* How to dispose of the results.  On of SRT_* above. */
> -       char *zAffSdst;         /* Affinity used when eDest==SRT_Set */
> +       /* Affinity used when eDest==SRT_Set */
> +       enum field_type *zAffSdst;

3. Not affinity.

>          int iSDParm;            /* A parameter used by the eDest disposal method */
>          /** Register containing ephemeral's space pointer. */
>          int reg_eph;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 61d73b676..c3f596d4f 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2163,9 +2151,16 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>                          break;
>                  }
>          } else {
> -               /* Neither operand is NULL.  Do a comparison. */
> -               affinity = pOp->p5 & AFFINITY_MASK;
> -               if (affinity>=AFFINITY_INTEGER) {
> +               /*
> +                * Neither operand is NULL. Do a comparison.
> +                * 15 is 1111 in a binary form.
> +                * Since all existing types can be fitted in 4 bits
> +                * (field_type_MAX == 10), it is enough to 'and'
> +                * p5 with this constant. Node that all other flags
> +                * that can be stored in p5 are >= 16.
> +                */
> +               affinity = pOp->p5 & 15;

4. Honestly, I did not expect it from you. Please, never use such
constants and assumptions about them in code. field_type_MAX = 10 can
be changed any day (for example, when we introduce decimal as a native
Tarantool data type, or decide to add datetime). In fact, it is already
not 10 - it is 9.

I propose you to either come up with a better solution or to use one of
my solutions:

* return AFFINITY_MASK as P5_FIELD_TYPE_MASK, and add a static assertion
   that P5_FIELD_TYPE_MASK < (1 << bit_count(field_type)).

   Or write in field_def.h something like this:

       static_assert(bit_count(field_type) <= 4,
                     "size of enum field_type is used in "\
                     "VdbeOp.p5 to fit it into 4 bits");

   To get bit_count during compilation you can specify it explicitly right
   after field_type_MAX in enum field_type so as to change it always
   together with field_type_MAX.

* split u16 VdbeOp.p5 into u8 VdbeOp.p6 and u8 VdbeOp.p5. But I am not
   sure if all p5 flags fit into u8.

> +               if (sql_type_is_numeric(affinity)) {
>                          if ((flags1 | flags3)&MEM_Str) {
>                                  if ((flags1 & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
>                                          applyNumericAffinity(pIn1,0);
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 33b860f36..01c1a2c6c 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -367,15 +367,15 @@ disableTerm(WhereLevel * pLevel, WhereTerm * pTerm)
>    * Code an OP_ApplyType opcode to apply the column type string types
>    * to the n registers starting at base.
>    *
> - * As an optimization, AFFINITY_BLOB entries (which are no-ops) at the
> + * As an optimization, SCALAR entries (which are no-ops) at the
>    * beginning and end of zAff are ignored.  If all entries in zAff are
> - * AFFINITY_BLOB, then no code gets generated.
> + * SCALAR, then no code gets generated.
>    *
>    * This routine makes its own copy of zAff so that the caller is free
>    * to modify zAff after this routine returns.
>    */
>   static void
> -codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
> +codeApplyAffinity(Parse * pParse, int base, int n, enum field_type *zAff)

5. Not affinity.

>   {
>          Vdbe *v = pParse->pVdbe;
>          if (zAff == 0) {
> @@ -419,16 +419,15 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
>   static void
>   updateRangeAffinityStr(Expr * pRight,  /* RHS of comparison */
>                         int n,           /* Number of vector elements in comparison */
> -                      char *zAff)      /* Affinity string to modify */
> +                      enum field_type *zAff)   /* Affinity string to modify */

6. The same. In many other places too. Where you changed type and
usages of a variable, but kept its name.

>   {
>          int i;
>          for (i = 0; i < n; i++) {
> -               enum field_type type = sql_affinity_to_field_type(zAff[i]);
>                  Expr *p = sqlite3VectorFieldSubexpr(pRight, i);
>                  enum field_type expr_type = sql_expr_type(p);
> -               if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
> -                       sql_expr_needs_no_type_change(p, type)) {
> -                       zAff[i] = AFFINITY_BLOB;
> +               if (sql_type_result(expr_type, zAff[i]) == FIELD_TYPE_SCALAR ||
> +                   sql_expr_needs_no_type_change(p, zAff[i])) {
> +                       zAff[i] = FIELD_TYPE_SCALAR;
>                  }
>          }
>   }