[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type

n.pettik korablev at tarantool.org
Mon Jul 22 13:20:30 MSK 2019



> On 19 Jul 2019, at 00:13, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Why are you keep trying not to fix this obvious bug?
> If that path is unreachable, then either delete it,
> or add an assertion. Or fix this place.

Sorry, message threads now seem to be entangled a bit,
so I guess you are talking about valueFromExpr() func.
I’ve removed the single reachable to this function path,
but left function itself (valueFromExpr()) since it seem
to be quite meaningful and can be applied somewhere
later. In the next patch I’ve added assertion fault to the
place of conversion of double negative values (-(-5)):

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index cf738dcc7..f296ed329 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1424,8 +1424,15 @@ valueFromExpr(sql * db,  /* The database connection */
                                mem_set_u64(pVal, (uint64_t)(-pVal->u.i));
                        } else if ((pVal->flags & MEM_UInt) != 0) {
                                if (pVal->u.u > (uint64_t) INT64_MAX + 1) {
-                                       pVal->u.r = -(double) pVal->u.u;
-                                       MemSetTypeFlag(pVal, MEM_Real);
+                                       /*
+                                        * FIXME: while resurrecting this func
+                                        * come up with way of dealing with
+                                        * this situation. In previous
+                                        * implementation it was conversion to
+                                        * double, but in this case
+                                        * -(UINT) x -> (DOUBLE) y and -y != x.
+                                        */
+                                       unreachable();
                                } else {
                                        mem_set_i64(pVal, (int64_t)(-pVal->u.u));
                                }

Author: Nikita Pettik <korablev at tarantool.org>
Date:   Mon Jul 22 02:54:25 2019 +0300

    sql: remove sqlColumnDefault() function
    
    This routine implements obsolete mechanism to retrieve default column
    value. Since it is not used anymore, let's remove it. Note that related
    functions valieFromExpr()/valueFromFunction() are not erased since they
    look pretty useful and may be involved later.

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 9af23e985..4623589f0 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -1025,9 +1025,8 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
                 */
                for (int i = 0; i < part_count; i++) {
                        uint32_t tabl_col = idx->def->key_def->parts[i].fieldno;
-                       sqlExprCodeGetColumnOfTable(v, space->def,
-                                                       tab_cursor, tabl_col,
-                                                       col_reg + i);
+                       sqlVdbeAddOp3(v, OP_Column, tab_cursor, tabl_col,
+                                     col_reg + i);
                }
                sqlVdbeAddOp3(v, OP_MakeRecord, col_reg, part_count,
                                  sample_reg);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ba0bc2c52..454899592 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -294,11 +294,8 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                if (!is_view) {
                        struct key_part_def *part = pk_info->parts;
                        for (int i = 0; i < pk_len; i++, part++) {
-                               struct space_def *def = space->def;
-                               sqlExprCodeGetColumnOfTable(v, def,
-                                                               tab_cursor,
-                                                               part->fieldno,
-                                                               reg_pk + i);
+                               sqlVdbeAddOp3(v, OP_Column, tab_cursor,
+                                             part->fieldno, reg_pk + i);
                        }
                } else {
                        for (int i = 0; i < pk_len; i++) {
@@ -482,10 +479,8 @@ sql_generate_row_delete(struct Parse *parse, struct space *space,
                sqlVdbeAddOp2(v, OP_Copy, reg_pk, first_old_reg);
                for (int i = 0; i < (int)space->def->field_count; i++) {
                        if (column_mask_fieldno_is_set(mask, i)) {
-                               sqlExprCodeGetColumnOfTable(v, space->def,
-                                                               cursor, i,
-                                                               first_old_reg +
-                                                               i + 1);
+                               sqlVdbeAddOp3(v, OP_Column, cursor, i,
+                                             first_old_reg + i + 1);
                        }
                }
 
@@ -579,8 +574,7 @@ sql_generate_index_key(struct Parse *parse, struct index *index, int cursor,
                        continue;
                }
                uint32_t tabl_col = index->def->key_def->parts[j].fieldno;
-               sqlExprCodeGetColumnOfTable(v, space->def, cursor, tabl_col,
-                                               reg_base + j);
+               sqlVdbeAddOp3(v, OP_Column, cursor, tabl_col, reg_base + j);
                /*
                 * If the column type is NUMBER but the number
                 * is an integer, then it might be stored in the
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index bf347b401..f4f4383ab 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3480,19 +3480,8 @@ sqlExprCachePinRegister(Parse * pParse, int iReg)
        }
 }
 
-void
-sqlExprCodeGetColumnOfTable(Vdbe *v, struct space_def *space_def,
-                               int iTabCur, int iCol, int regOut)
-{
-       sqlVdbeAddOp3(v, OP_Column, iTabCur, iCol, regOut);
-       if (iCol >= 0) {
-               sqlColumnDefault(v, space_def, iCol, regOut);
-       }
-}
-
 int
-sqlExprCodeGetColumn(Parse *pParse, struct space_def *space_def,
-                        int iColumn, int iTable, int iReg, u8 p5)
+sqlExprCodeGetColumn(Parse *pParse, int iColumn, int iTable, int iReg, u8 p5)
 {
        Vdbe *v = pParse->pVdbe;
        int i;
@@ -3507,7 +3496,7 @@ sqlExprCodeGetColumn(Parse *pParse, struct space_def *space_def,
                }
        }
        assert(v != 0);
-       sqlExprCodeGetColumnOfTable(v, space_def, iTable, iColumn, iReg);
+       sqlVdbeAddOp3(v, OP_Column, iTable, iColumn, iReg);
        if (p5) {
                sqlVdbeChangeP5(v, p5);
        } else {
@@ -3517,12 +3506,10 @@ sqlExprCodeGetColumn(Parse *pParse, struct space_def *space_def,
 }
 
 void
-sqlExprCodeGetColumnToReg(Parse * pParse, struct space_def * space_def,
-                             int iColumn, int iTable, int iReg)
+sqlExprCodeGetColumnToReg(Parse * pParse, int iColumn, int iTable, int iReg)
 {
        int r1 =
-               sqlExprCodeGetColumn(pParse, space_def, iColumn, iTable,
-                                        iReg, 0);
+               sqlExprCodeGetColumn(pParse, iColumn, iTable, iReg, 0);
        if (r1 != iReg)
                sqlVdbeAddOp2(pParse->pVdbe, OP_SCopy, r1, iReg);
 }
@@ -3721,10 +3708,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
                                        iTab = pParse->iSelfTab;
                                }
                        }
-                       return sqlExprCodeGetColumn(pParse,
-                                                       pExpr->space_def, col,
-                                                       iTab, target,
-                                                       pExpr->op2);
+                       return sqlExprCodeGetColumn(pParse, col, iTab, target,
+                                                   pExpr->op2);
                }
        case TK_INTEGER:{
                        expr_code_int(pParse, pExpr, false, target);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index a257e7204..68c5731bd 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6069,8 +6069,7 @@ sqlSelect(Parse * pParse,         /* The parser context */
                                        if (pCol->iSorterColumn >= j) {
                                                int r1 = j + regBase;
                                                sqlExprCodeGetColumnToReg
-                                                   (pParse, pCol->space_def,
-                                                    pCol->iColumn,
+                                                   (pParse, pCol->iColumn,
                                                     pCol->iTable, r1);
                                                j++;
                                        }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 98b4c651f..5842eeec6 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3183,7 +3183,6 @@ int sqlWhereOkOnePass(WhereInfo *, int *);
  * stored in any register.  But the result is guaranteed to land
  * in register iReg for GetColumnToReg().
  * @param pParse Parsing and code generating context.
- * @param space_def Space definition.
  * @param iColumn Index of the table column.
  * @param iTable The cursor pointing to the table.
  * @param iReg Store results here.
@@ -3191,31 +3190,19 @@ int sqlWhereOkOnePass(WhereInfo *, int *);
  * @return iReg value.
  */
 int
-sqlExprCodeGetColumn(Parse *, struct space_def *, int, int, int, u8);
+sqlExprCodeGetColumn(Parse *, int, int, int, u8);
 
 /**
  * Generate code that will extract the iColumn-th column from
  * table defined by space_def and store the column value in
  * a register, copy the result.
  * @param pParse Parsing and code generating context.
- * @param space_def Space definition.
  * @param iColumn Index of the table column.
  * @param iTable The cursor pointing to the table.
  * @param iReg Store results here.
  */
 void
-sqlExprCodeGetColumnToReg(Parse *, struct space_def *, int, int, int);
-
-/**
- * Generate code to extract the value of the iCol-th column of a table.
- * @param v  The VDBE under construction.
- * @param space_def Space definition.
- * @param iTabCur The PK cursor.
- * @param iCol Index of the column to extract.
- * @param regOut  Extract the value into this register.
- */
-void
-sqlExprCodeGetColumnOfTable(Vdbe *, struct space_def *, int, int, int);
+sqlExprCodeGetColumnToReg(Parse *, int, int, int);
 
 void sqlExprCodeMove(Parse *, int, int, int);
 void sqlExprCacheStore(Parse *, int, int, int);
@@ -4111,44 +4098,6 @@ int sqlResolveExprListNames(NameContext *, ExprList *);
 void sqlResolveSelectNames(Parse *, Select *, NameContext *);
 int sqlResolveOrderGroupBy(Parse *, Select *, ExprList *, const char *);
 
-/**
- * Generate code for default value.
- * The most recently coded instruction was an OP_Column to retrieve the
- * i-th column of table defined by space_def. This routine sets
- * the P4 parameter of the OP_Column to the default value, if any.
- *
- * The default value of a column is specified by a DEFAULT clause in the
- * column definition. This was either supplied by the user when the table
- * was created, or added later to the table definition by an ALTER TABLE
- * command. If the latter, then the row-records in the table btree on disk
- * may not contain a value for the column and the default value, taken
- * from the P4 parameter of the OP_Column instruction, is returned instead.
- * If the former, then all row-records are guaranteed to include a value
- * for the column and the P4 value is not required.
- *
- * Column definitions created by an ALTER TABLE command may only have
- * literal default values specified: a number, null or a string. (If a more
- * complicated default expression value was provided, it is evaluated
- * when the ALTER TABLE is executed and one of the literal values written
- * into the schema.)
- *
- * Therefore, the P4 parameter is only required if the default value for
- * the column is a literal number, string or null. The sqlValueFromExpr()
- * function is capable of transforming these types of expressions into
- * sql_value objects.
- *
- * If parameter iReg is not negative, code an OP_Realify instruction
- * on register iReg. This is used when an equivalent integer value is
- * stored in place of an 8-byte floating point value in order to save
- * space.
- * @param v Vdbe object.
- * @param def space definition object.
- * @param i column index.
- * @param iReg register index.
- */
-void
-sqlColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg);
-
 char* rename_trigger(sql *, char const *, char const *, bool *);
 /**
  * Find a collation by name. Set error in @a parser if not found.
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index d77bee051..c5ba56afa 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -38,31 +38,6 @@
 #include "box/tuple_format.h"
 #include "box/schema.h"
 
-void
-sqlColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg)
-{
-       assert(def != 0);
-       if (!def->opts.is_view) {
-               sql_value *pValue = 0;
-               enum field_type type = def->fields[i].type;
-               VdbeComment((v, "%s.%s", def->name, def->fields[i].name));
-               assert(i < (int)def->field_count);
-
-               Expr *expr = NULL;
-               assert(def->fields != NULL && i < (int)def->field_count);
-               if (def->fields != NULL)
-                       expr = def->fields[i].default_value_expr;
-               sqlValueFromExpr(sqlVdbeDb(v), expr, type,
-                                    &pValue);
-               if (pValue) {
-                       sqlVdbeAppendP4(v, pValue, P4_MEM);
-               }
-               if (type == FIELD_TYPE_NUMBER) {
-                       sqlVdbeAddOp1(v, OP_Realify, ireg);
-               }
-       }
-}
-
 /*
  * Process an UPDATE statement.
  *
@@ -266,10 +241,9 @@ sqlUpdate(Parse * pParse,          /* The parser context */
                }
        } else {
                for (i = 0; i < (int) pk_part_count; i++) {
-                       sqlExprCodeGetColumnOfTable(v, def, pk_cursor,
-                                                       pPk->def->key_def->
-                                                               parts[i].fieldno,
-                                                       iPk + i);
+                       sqlVdbeAddOp3(v, OP_Column, pk_cursor,
+                                     pPk->def->key_def->parts[i].fieldno,
+                                     iPk + i);
                }
        }
 
@@ -341,8 +315,8 @@ sqlUpdate(Parse * pParse,           /* The parser context */
                for (i = 0; i < (int)def->field_count; i++) {
                        if (column_mask_fieldno_is_set(oldmask, i) ||
                            sql_space_column_is_in_pk(space, i)) {
-                               sqlExprCodeGetColumnOfTable(v, def, pk_cursor,
-                                                           i, regOld + i);
+                               sqlVdbeAddOp3(v, OP_Column, pk_cursor, i,
+                                             regOld + i);
                        } else {
                                sqlVdbeAddOp2(v, OP_Null, 0, regOld + i);
                        }
@@ -377,8 +351,8 @@ sqlUpdate(Parse * pParse,           /* The parser context */
                         * if there are one or more BEFORE triggers that use this value via
                         * a new.* reference in a trigger program.
                         */
-                       sqlExprCodeGetColumnToReg(pParse, def, i,
-                                                     pk_cursor, regNew + i);
+                       sqlExprCodeGetColumnToReg(pParse, i, pk_cursor,
+                                                 regNew + i);
                } else {
                        sqlVdbeAddOp2(v, OP_Null, 0, regNew + i);
                }
@@ -416,9 +390,8 @@ sqlUpdate(Parse * pParse,           /* The parser context */
                 */
                for (i = 0; i < (int)def->field_count; i++) {
                        if (aXRef[i] < 0) {
-                               sqlExprCodeGetColumnOfTable(v, def,
-                                                               pk_cursor, i,
-                                                               regNew + i);
+                               sqlVdbeAddOp3(v, OP_Column, pk_cursor, i,
+                                             regNew + i);
                        }
                }
        }
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 7d5dd46b0..19c9740ae 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -1415,7 +1415,6 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo,      /* Complete information about the W
                                                                fieldno;
                                                        sqlExprCodeGetColumnToReg
                                                                (pParse,
-                                                                space->def,
                                                                 fieldno,
                                                                 iCur,
                                                                 r + iPk); 

Also, Konstantin asked me to fix typeof() behaviour
so that it returns “integer” for integer values > 0 as well
(since basically range of integers in Tarantool is
 up to 2^64 - 1). I’ve applied corresponding fixes
to the patch "sql: make built-in functions operate on unsigned values"

> On 18/07/2019 23:08, Vladislav Shpilevoy wrote:
>> 
>> 
>> On 18/07/2019 22:56, n.pettik wrote:
>>> 
>>> 
>>>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy <v.shpilevoy at tarantool.org <mailto:v.shpilevoy at tarantool.org>> wrote:
>>>> 
>>>> Hi!
>>>> 
>>>> Thanks for the fixes!
>>>> 
>>>>>> -------------------------
>>>>>> vdbe.c:307
>>>>>> 
>>>>>>> case FIELD_TYPE_INTEGER:
>>>>>>> case FIELD_TYPE_UNSIGNED:
>>>>>>> if ((record->flags & MEM_Int) == MEM_Int)
>>>>>>> return 0;
>>>>>>> if ((record->flags & MEM_UInt) == MEM_UInt)
>>>>>>> return 0;
>>>>>>> if ((record->flags & MEM_Real) == MEM_Real) {
>>>>>>> int64_t i = (int64_t) record->u.r;
>>>>>>> if (i == record->u.r)
>>>>>>> mem_set_int(record, record->u.r,
>>>>>>>     record->u.r <= -1);
>>>>>>> return 0;
>>>>>>> }
>>>>>> 
>>>>>> It is a part of function mem_apply_type. When target type is
>>>>>> UNSIGNED, and a value is MEM_Int, you do nothing. Why? Looks like
>>>>>> it is possible to pass here a negative value, and CAST UNSIGNED
>>>>>> would do nothing.
>>>>> 
>>>>> Basically, this function implements sort of implicit cast
>>>>> which takes place before comparison/assignment.
>>>>> For comparisons it makes no sense - we can compare
>>>>> integer with unsigned value - the latter is always greater.
>>>>> For assignment it is also meaningless: if we attempt
>>>>> at inserting negative values to unsigned field appropriate
>>>>> error will be raised anyway. If you can come up with
>>>>> specific example, let’s discuss it.
>>>>> 
>>>> 
>>>> I can't provide a test. But the function is named mem_apply_type,
>>>> and it doesn't apply type, when it is unsigned, and a value is
>>>> negative. Doesn't it look wrong to you?
>>>> 
>>>> If some code wants to get an integer, it can apply FIELD_TYPE_INTEGER
>>>> instead of FIELD_TYPE_UNSIGNED. IMO, an attempt to apply unsigned
>>>> to int should raise an error here. Otherwise this function can't
>>>> be named 'apply_type' because it ignores negative -> unsigned case.
>>> 
>>> Okay, let’s rename it. I can suggest these options:
>>> 
>>> mem_cast_implicit()
>>> mem_cast_implicit_to_type()
>>> mem_implicit_cast_to_type()
>>> mem_convert_implicit()
>>> mem_convert_to_type()
>>> mem_type_coerce_implicit()
>>> mem_type_implicit_coercion()
>>> mem_type_coercion_implicit()
>>> mem_implicit_type_juggling()
>>> mem_implicit_juggle_to_type()
>>> mem_do_implicit_conversion()
>>> mem_do_implicit_coercion()
>>> 
>>> Or any other combination :)
>>> 
>> 
>> But it is not implicit. It just does not work, when a value is negative,
>> and type is unsigned.





More information about the Tarantool-patches mailing list