From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id CB5C72456E for ; Mon, 22 Jul 2019 06:20:33 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id oPfN0dQscsuy for ; Mon, 22 Jul 2019 06:20:33 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 61F6D2455B for ; Mon, 22 Jul 2019 06:20:33 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type From: "n.pettik" In-Reply-To: Date: Mon, 22 Jul 2019 13:20:30 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <77AF4BF0-D597-425F-A4AF-EDA7EB3901C6@tarantool.org> References: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> <552F96C1-DAC5-4F18-9F5A-BF50C6BBC205@tarantool.org> <8e4feefd-7bfb-18af-fd0f-b45384e5d2ef@tarantool.org> <127420CE-540E-439C-B2BD-20007EE98328@tarantool.org> <989f9710-043f-447a-0cc4-76eb317bc1e9@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 19 Jul 2019, at 00:13, Vladislav Shpilevoy = wrote: >=20 > 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=E2=80=99ve 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=E2=80=99ve 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) !=3D 0) { if (pVal->u.u > (uint64_t) INT64_MAX + = 1) { - pVal->u.r =3D -(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 !=3D x. + */ + unreachable(); } else { mem_set_i64(pVal, = (int64_t)(-pVal->u.u)); } Author: Nikita Pettik Date: Mon Jul 22 02:54:25 2019 +0300 sql: remove sqlColumnDefault() function =20 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 =3D 0; i < part_count; i++) { uint32_t tabl_col =3D = 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 =3D pk_info->parts; for (int i =3D 0; i < pk_len; i++, part++) { - struct space_def *def =3D 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 =3D 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 =3D 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); } } =20 @@ -579,8 +574,7 @@ sql_generate_index_key(struct Parse *parse, struct = index *index, int cursor, continue; } uint32_t tabl_col =3D = 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) } } =20 -void -sqlExprCodeGetColumnOfTable(Vdbe *v, struct space_def *space_def, - int iTabCur, int iCol, int regOut) -{ - sqlVdbeAddOp3(v, OP_Column, iTabCur, iCol, regOut); - if (iCol >=3D 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 =3D pParse->pVdbe; int i; @@ -3507,7 +3496,7 @@ sqlExprCodeGetColumn(Parse *pParse, struct = space_def *space_def, } } assert(v !=3D 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, } =20 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 =3D - sqlExprCodeGetColumn(pParse, space_def, iColumn, iTable, - iReg, 0); + sqlExprCodeGetColumn(pParse, iColumn, iTable, iReg, 0); if (r1 !=3D iReg) sqlVdbeAddOp2(pParse->pVdbe, OP_SCopy, r1, iReg); } @@ -3721,10 +3708,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) iTab =3D 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 >=3D j) = { int r1 =3D 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); =20 /** * 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); =20 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 = *); =20 -/** - * 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" =20 -void -sqlColumnDefault(Vdbe *v, struct space_def *def, int i, int ireg) -{ - assert(def !=3D 0); - if (!def->opts.is_view) { - sql_value *pValue =3D 0; - enum field_type type =3D def->fields[i].type; - VdbeComment((v, "%s.%s", def->name, = def->fields[i].name)); - assert(i < (int)def->field_count); - - Expr *expr =3D NULL; - assert(def->fields !=3D NULL && i < = (int)def->field_count); - if (def->fields !=3D NULL) - expr =3D def->fields[i].default_value_expr; - sqlValueFromExpr(sqlVdbeDb(v), expr, type, - &pValue); - if (pValue) { - sqlVdbeAppendP4(v, pValue, P4_MEM); - } - if (type =3D=3D 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 =3D 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); } } =20 @@ -341,8 +315,8 @@ sqlUpdate(Parse * pParse, /* The parser = context */ for (i =3D 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 =3D 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);=20 Also, Konstantin asked me to fix typeof() behaviour so that it returns =E2=80=9Cinteger=E2=80=9D for integer values > 0 as = well (since basically range of integers in Tarantool is up to 2^64 - 1). I=E2=80=99ve applied corresponding fixes to the patch "sql: make built-in functions operate on unsigned values" > On 18/07/2019 23:08, Vladislav Shpilevoy wrote: >>=20 >>=20 >> On 18/07/2019 22:56, n.pettik wrote: >>>=20 >>>=20 >>>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy = > wrote: >>>>=20 >>>> Hi! >>>>=20 >>>> Thanks for the fixes! >>>>=20 >>>>>> ------------------------- >>>>>> vdbe.c:307 >>>>>>=20 >>>>>>> case FIELD_TYPE_INTEGER: >>>>>>> case FIELD_TYPE_UNSIGNED: >>>>>>> if ((record->flags & MEM_Int) =3D=3D MEM_Int) >>>>>>> return 0; >>>>>>> if ((record->flags & MEM_UInt) =3D=3D MEM_UInt) >>>>>>> return 0; >>>>>>> if ((record->flags & MEM_Real) =3D=3D MEM_Real) { >>>>>>> int64_t i =3D (int64_t) record->u.r; >>>>>>> if (i =3D=3D record->u.r) >>>>>>> mem_set_int(record, record->u.r, >>>>>>> record->u.r <=3D -1); >>>>>>> return 0; >>>>>>> } >>>>>>=20 >>>>>> 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. >>>>>=20 >>>>> 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=E2=80=99s discuss it. >>>>>=20 >>>>=20 >>>> 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? >>>>=20 >>>> 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. >>>=20 >>> Okay, let=E2=80=99s rename it. I can suggest these options: >>>=20 >>> 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() >>>=20 >>> Or any other combination :) >>>=20 >>=20 >> But it is not implicit. It just does not work, when a value is = negative, >> and type is unsigned.