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 3E4E928F9F for ; Sun, 2 Jun 2019 14:11:13 -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 9Zk1nQ97vnKb for ; Sun, 2 Jun 2019 14:11:13 -0400 (EDT) Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 783A228DBC for ; Sun, 2 Jun 2019 14:11:12 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator References: <789091b7acd99c908d26689f27c55f8b6dba3d16.1558700151.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <91e73947-30b8-c172-f52c-be109034e0dd@tarantool.org> Date: Sun, 2 Jun 2019 20:11:07 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the fixes! See 3 comments below. > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index 3025e5e6b..107b9802d 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1115,7 +1115,8 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > int seek_addr = 0; > for (int i = 0; i < nEq; i++) { > enum field_type type = idx_def->key_def->parts[i].type; > - if (type == FIELD_TYPE_INTEGER) { > + if (type == FIELD_TYPE_INTEGER || > + type == FIELD_TYPE_UNSIGNED) { > /* > * OP_MustBeInt consider NULLs as > * non-integer values, so firstly > @@ -1128,8 +1129,9 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > } > } > /* Inequality constraint comes always at the end of list. */ > - if (pRangeStart != NULL && > - idx_def->key_def->parts[nEq].type == FIELD_TYPE_INTEGER) > + enum field_type ineq_type = idx_def->key_def->parts[nEq].type; > + if (pRangeStart != NULL && (ineq_type == FIELD_TYPE_INTEGER || > + ineq_type == FIELD_TYPE_UNSIGNED)) 1. Please, add a test for unsigned. I thought that it is quite obvious - everything needs a test, especially if the bug was found during a review. > force_integer_reg = regBase + nEq; > emit_apply_type(pParse, regBase, nConstraint - bSeekPastNull, > start_types); > >>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c >>> index 977c0fced..e779729c7 100644 >>> --- a/src/box/sql/wherecode.c >>> +++ b/src/box/sql/wherecode.c >>> @@ -1086,31 +1086,61 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W >>> start_constraints = 1; >>> } >>> struct index_def *idx_pk = space->index[0]->def; >>> - int fieldno = idx_pk->key_def->parts[0].fieldno; >>> - >>> uint32_t pk_part_count = idx_pk->key_def->part_count; >>> - if (pk_part_count == 1 && >>> - space->def->fields[fieldno].type == FIELD_TYPE_INTEGER) { >>> - /* Right now INTEGER PRIMARY KEY is the only option to >>> - * get Tarantool's INTEGER column type. Need special handling >>> - * here: try to loosely convert FLOAT to INT. If RHS type >>> - * is not INT or FLOAT - skip this ites, i.e. goto addrNxt. >>> - */ >>> - int limit = pRangeStart == NULL ? nEq : nEq + 1; >>> - for (int i = 0; i < limit; i++) { >>> - if (idx_def->key_def->parts[i].fieldno == >>> - idx_pk->key_def->parts[0].fieldno) { >>> - /* Here: we know for sure that table has INTEGER >>> - PRIMARY KEY, single column, and Index we're >>> - trying to use for scan contains this column. */ >>> - if (i < nEq) >>> - sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, addrNxt); >>> - else >>> - force_integer_reg = regBase + i; >>> - break; >>> - } >>> + /* >>> + * Tarantool's iterator over integer fields doesn't >>> + * tolerate floating point values. Hence, if term >>> + * is equality comparison and value of operand is >>> + * not integer, we can skip it since it always >>> + * results in false: INT a == 0.5 -> false; >>> + * It is done using OP_MustBeInt facilities. >>> + * In case term is greater comparison (a > ?), we >>> + * should notify OP_SeekGT to process truncation of >>> + * floating point value: a > 0.5 -> a >= 1; >>> + * It is done by setting P5 flag for OP_Seek*. >>> + * It is worth mentioning that we do not need >>> + * this step when it comes for less comparison: >>> + * key is NULL in this case: values are ordered >>> + * as NULL, ... NULL, min_value, ..., so to >>> + * fetch min value we pass NULL to GT iterator. >> >> 4. I did not get the 'less comparator == compare with NULL' >> idea. Firstly you describe how to compare 'a > ?', this is >> fully understandable. Then you, as I see, go to the 'a < ?' >> case, but somewhy say, that it is the same as 'NULL < ?'. >> Do not understand. > > I say that in this case key which is passed to iterator is NULL > and type of iterator is GE. 2. Why? I still do not understand. What if I wrote 'a < 100'? you do not replace 100 with NULL. But the comment says, that for any 'less' operator a key is NULL: "when it comes for less comparison key is NULL". > Consequently, pRangeStart is NULL. > I’ve slightly updated this part of comment: > > diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c > index 3025e5e6b..01ee9ce90 100644 > --- a/src/box/sql/wherecode.c > +++ b/src/box/sql/wherecode.c > @@ -1089,10 +1089,10 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > * floating point value: a > 0.5 -> a >= 1; > * It is done by setting P5 flag for OP_Seek*. > * It is worth mentioning that we do not need > - * this step when it comes for less comparison: > - * key is NULL in this case: values are ordered > - * as NULL, ... NULL, min_value, ..., so to > - * fetch min value we pass NULL to GT iterator. > + * this step when it comes for less (<) comparison > + * of nullable field. Key is NULL in this case: > + * values are ordered as NULL, ... NULL, min_value, > + * so to fetch min value we pass NULL to GT iterator. > > > @@ -1112,24 +1112,33 @@ sqlWhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about the W > /* > * OP_MustBeInt consider NULLs as > * non-integer values, so firstly > * check whether value is NULL or not. > */ > - seek_addr = sqlVdbeAddOp1(v, OP_IsNull, > + seek_addrs[i] = sqlVdbeAddOp1(v, OP_IsNull, > regBase); 3. Now the indentation is broken. > sqlVdbeAddOp2(v, OP_MustBeInt, regBase + i, > addrNxt); > } > } > /* Inequality constraint comes always at the end of list. */ >