Re: [Tarantool-patches] [PATCH 2/6] sql: remove mem_apply_type() from OP_MakeRecord

[Mergen Imeev <imeevma@tarantool.org>]

Thank you for the review! New patch below.


On 01.06.2020 20:03, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 2a941025c..fc41ee0d6 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -2902,7 +2902,6 @@ case OP_CheckType: {
>>    * into ephemeral space. Thus, sort of memory optimization can be performed.
>>    */
> P4 should be removed from the comment too.
Fixed.
>
>>   case OP_MakeRecord: {
>> -	Mem *pRec;             /* The new record */
>>   	Mem *pData0;           /* First field to be combined into the record */
>>   	Mem MAYBE_UNUSED *pLast;  /* Last field of the record */
>>   	int nField;            /* Number of fields in the record */



New patch:


 From f87e9b757844bde856bc41de5ad90754c66b0ef3 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Wed, 27 May 2020 11:16:43 +0300
Subject: [PATCH] sql: remove mem_apply_type() from OP_MakeRecord

This patch removes type changing from OP_MakeRecord.

Part of #4230

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index f74f9b358..23fbb15de 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -965,12 +965,7 @@ vdbe_emit_analyze_space(struct Parse *parse, struct 
space *space)
          sqlVdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
          /* Add the entry to the stat1 table. */
          callStatGet(v, stat4_reg, STAT_GET_STAT1, stat1_reg);
-        enum field_type types[4] = { FIELD_TYPE_STRING,
-                         FIELD_TYPE_STRING,
-                         FIELD_TYPE_STRING,
-                         field_type_MAX };
-        sqlVdbeAddOp4(v, OP_MakeRecord, tab_name_reg, 4, tmp_reg,
-                  (char *)types, sizeof(types));
+        sqlVdbeAddOp3(v, OP_MakeRecord, tab_name_reg, 4, tmp_reg);
          sqlVdbeAddOp4(v, OP_IdxInsert, tmp_reg, 0, 0,
                    (char *)stat1, P4_SPACEPTR);
          /* Add the entries to the stat4 table. */
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 68abd1f58..0b6f0bd62 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -328,12 +328,8 @@ sql_table_delete_from(struct Parse *parse, struct 
SrcList *tab_list,
               * key.
               */
              key_len = 0;
-            struct index *pk = space_index(space, 0);
-            enum field_type *types = is_view ? NULL :
-                         sql_index_type_str(parse->db,
-                                    pk->def);
-            sqlVdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-                      reg_key, (char *)types, P4_DYNAMIC);
+            sqlVdbeAddOp3(v, OP_MakeRecord, reg_pk, pk_len,
+                      reg_key);
              /* Set flag to save memory allocating one
               * by malloc.
               */
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index bc2182446..019628a26 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2859,8 +2859,6 @@ sqlCodeSubselect(Parse * pParse,    /* Parsing 
context */
                  struct ExprList_item *pItem;
                  int r1, r2, r3;

-                enum field_type lhs_type =
-                    sql_expr_type(pLeft);
                  bool unused;
                  struct coll *unused_coll;
                  if (sql_expr_coll(pParse, pExpr->pLeft, &unused,
@@ -2886,11 +2884,7 @@ sqlCodeSubselect(Parse * pParse,    /* Parsing 
context */
                          jmpIfDynamic = -1;
                      }
                      r3 = sqlExprCodeTarget(pParse, pE2, r1);
-                    enum field_type types[2] =
-                        { lhs_type, field_type_MAX };
-                     sqlVdbeAddOp4(v, OP_MakeRecord, r3,
-                              1, r2, (char *)types,
-                              sizeof(types));
+                    sqlVdbeAddOp3(v, OP_MakeRecord, r3, 1, r2);
                      sql_expr_type_cache_change(pParse,
                                     r3, 1);
                      sqlVdbeAddOp2(v, OP_IdxInsert, r2,
diff --git a/src/box/sql/fk_constraint.c b/src/box/sql/fk_constraint.c
index 482220a95..43471d51a 100644
--- a/src/box/sql/fk_constraint.c
+++ b/src/box/sql/fk_constraint.c
@@ -262,13 +262,8 @@ fk_constraint_lookup_parent(struct Parse 
*parse_context, struct space *parent,
                        link->child_field + 1 + reg_data,
                        temp_regs + i);
          }
-        struct index *idx = space_index(parent, referenced_idx);
-        assert(idx != NULL);
-        sqlVdbeAddOp4(v, OP_MakeRecord, temp_regs, field_count,
-                  rec_reg,
-                  (char *) sql_index_type_str(parse_context->db,
-                                  idx->def),
-                  P4_DYNAMIC);
+        sqlVdbeAddOp3(v, OP_MakeRecord, temp_regs, field_count,
+                  rec_reg);
          sqlVdbeAddOp4Int(v, OP_Found, cursor, ok_label, rec_reg, 0);
          sqlReleaseTempReg(parse_context, rec_reg);
          sqlReleaseTempRange(parse_context, temp_regs, field_count);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 0b7358af4..f3f5dad76 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1270,13 +1270,8 @@ selectInnerLoop(Parse * pParse,        /* The 
parser context */
                             regOrig, nResultCol, nPrefixReg);
              } else {
                  int r1 = sqlGetTempReg(pParse);
-                enum field_type *types =
-                    field_type_sequence_dup(pParse,
-                                pDest->dest_type,
-                                nResultCol);
-                sqlVdbeAddOp4(v, OP_MakeRecord, regResult,
-                          nResultCol, r1, (char *)types,
-                          P4_DYNAMIC);
+                sqlVdbeAddOp3(v, OP_MakeRecord, regResult,
+                          nResultCol, r1);
                  sql_expr_type_cache_change(pParse,
                                 regResult,
                                 nResultCol);
@@ -1693,12 +1688,8 @@ generateSortTail(Parse * pParse,    /* Parsing 
context */
              break;
          }
      case SRT_Set:{
-            enum field_type *types =
-                field_type_sequence_dup(pParse, pDest->dest_type,
-                            nColumn);
-            sqlVdbeAddOp4(v, OP_MakeRecord, regRow, nColumn,
-                      regTupleid, (char *)types,
-                      P4_DYNAMIC);
+            sqlVdbeAddOp3(v, OP_MakeRecord, regRow, nColumn,
+                      regTupleid);
              sql_expr_type_cache_change(pParse, regRow, nColumn);
              sqlVdbeAddOp2(v, OP_IdxInsert, regTupleid, pDest->reg_eph);
              break;
@@ -3136,12 +3127,8 @@ generateOutputSubroutine(struct Parse *parse, 
struct Select *p,
              int r1;
              testcase(in->nSdst > 1);
              r1 = sqlGetTempReg(parse);
-            enum field_type *types =
-                field_type_sequence_dup(parse, dest->dest_type,
-                            in->nSdst);
-            sqlVdbeAddOp4(v, OP_MakeRecord, in->iSdst,
-                      in->nSdst, r1, (char *)types,
-                      P4_DYNAMIC);
+            sqlVdbeAddOp3(v, OP_MakeRecord, in->iSdst, in->nSdst,
+                      r1);
              sql_expr_type_cache_change(parse, in->iSdst,
                             in->nSdst);
              sqlVdbeAddOp2(v, OP_IdxInsert, r1, dest->reg_eph);
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 24c7cfa27..22f82390c 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -251,11 +251,7 @@ sqlUpdate(Parse * pParse,        /* The parser 
context */
          nKey = pk_part_count;
          regKey = iPk;
      } else {
-        enum field_type *types = is_view ? NULL :
-                     sql_index_type_str(pParse->db,
-                                pPk->def);
-        sqlVdbeAddOp4(v, OP_MakeRecord, iPk, pk_part_count,
-                  regKey, (char *) types, P4_DYNAMIC);
+        sqlVdbeAddOp3(v, OP_MakeRecord, iPk, pk_part_count, regKey);
          /*
           * Set flag to save memory allocating one by
           * malloc.
@@ -420,12 +416,8 @@ sqlUpdate(Parse * pParse,        /* The parser 
context */
              int key_reg;
              if (okOnePass) {
                  key_reg = sqlGetTempReg(pParse);
-                enum field_type *types =
-                    sql_index_type_str(pParse->db,
-                               pPk->def);
-                sqlVdbeAddOp4(v, OP_MakeRecord, iPk,
-                          pk_part_count, key_reg,
-                          (char *) types, P4_DYNAMIC);
+                sqlVdbeAddOp3(v, OP_MakeRecord, iPk,
+                          pk_part_count, key_reg);
              } else {
                  assert(nKey == 0);
                  key_reg = regKey;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ae2622c9e..eb5c89e9d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2908,24 +2908,17 @@ case OP_CheckType: {
      break;
  }

-/* Opcode: MakeRecord P1 P2 P3 P4 P5
+/* Opcode: MakeRecord P1 P2 P3 * P5
   * Synopsis: r[P3]=mkrec(r[P1@P2])
   *
   * Convert P2 registers beginning with P1 into the [record format]
   * use as a data record in a database table or as a key
   * in an index.  The OP_Column opcode can decode the record later.
   *
- * P4 may be a string that is P2 characters long.  The nth character of the
- * string indicates the column type that should be used for the nth
- * field of the index key.
- *
- * If P4 is NULL then all index fields have type SCALAR.
- *
   * If P5 is not NULL then record under construction is intended to be 
inserted
   * into ephemeral space. Thus, sort of memory optimization can be 
performed.
   */
  case OP_MakeRecord: {
-    Mem *pRec;             /* The new record */
      Mem *pData0;           /* First field to be combined into the 
record */
      Mem MAYBE_UNUSED *pLast;  /* Last field of the record */
      int nField;            /* Number of fields in the record */
@@ -2947,7 +2940,6 @@ case OP_MakeRecord: {
       * of the record to data0.
       */
      nField = pOp->p1;
-    enum field_type *types = pOp->p4.types;
      bIsEphemeral = pOp->p5;
      assert(nField>0 && pOp->p2>0 && pOp->p2+nField<=(p->nMem+1 - 
p->nCursor)+1);
      pData0 = &aMem[nField];
@@ -2958,15 +2950,6 @@ case OP_MakeRecord: {
      assert(pOp->p3<pOp->p1 || pOp->p3>=pOp->p1+pOp->p2);
      pOut = vdbe_prepare_null_out(p, pOp->p3);

-    /* Apply the requested types to all inputs */
-    assert(pData0<=pLast);
-    if (types != NULL) {
-        pRec = pData0;
-        do {
-            mem_apply_type(pRec++, *(types++));
-        } while(types[0] != field_type_MAX);
-    }
-
      struct region *region = &fiber()->gc;
      size_t used = region_used(region);
      uint32_t tuple_size;
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index e29db9d93..f7681640e 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -354,7 +354,7 @@ test:do_test(
          return exec_neph(" SELECT y IN (SELECT a FROM t1) FROM t2 ")
      end, {
          -- <in3-3.5>
-        1, true
+        1, false
          -- </in3-3.5>
      })

@@ -378,7 +378,7 @@ test:do_test(
          return exec_neph(" SELECT y IN (SELECT c FROM t1) FROM t2 ")
      end, {
          -- <in3-3.7>
-        1, true
+        1, false
          -- </in3-3.7>
      })