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 D2C4A30986 for ; Thu, 6 Jun 2019 15:07: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 adVYs1rB6peW for ; Thu, 6 Jun 2019 15:07:13 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 872D3306C9 for ; Thu, 6 Jun 2019 15:07:13 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: fix passing FP values to integer iterator References: <789091b7acd99c908d26689f27c55f8b6dba3d16.1558700151.git.korablev@tarantool.org> <91e73947-30b8-c172-f52c-be109034e0dd@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 6 Jun 2019 21:07:12 +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, Kirill Yukhin Thanks for the fixes! >>>>> 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”. > > Consider example: > > box.execute("CREATE TABLE t1(id INT PRIMARY KEY, a INT UNIQUE);") > box.execute("INSERT INTO t1 VALUES (1, 1), (2, 2), (3, -1), (4, NULL), (5, NULL);”) > box.execute("SELECT a FROM t1 WHERE a < 100;”) > > Bytecode for this query is: > > 102> 0 Init 0 1 0 00 Start at 1 > 102> 1 IteratorOpen 1 1 0 space 00 index id = 1, space ptr = space or reg[0]; unique_unnamed_T1_2 > 102> 2 Explain 0 0 0 SEARCH TABLE T1 USING COVERING INDEX unique_unnamed_T1_2 (A 102> 3 Null 0 1 0 00 r[1]=NULL > 102> 4 SeekGT 1 10 1 1 00 key=r[1] > 102> 5 Integer 100 1 0 00 r[1]=100 > 102> 6 IdxGE 1 10 1 1 00 key=r[1] > 102> 7 Column 1 1 2 00 r[2]=T1.A > 102> 8 ResultRow 2 1 0 00 output=r[2] > 102> 9 Next 1 6 0 00 > 102> 10 Halt 0 0 0 00 > > As you can see, SeekGT accepts NULL as a key for iterator. > Then we iterate until search condition is true. So, basically > 100 is replaced with NULL and not used as a key of iterator. > I wanted to underline this fact in my comment. Ah, thanks, now I got it. Kirill, the patchset LGTM.