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 8B2272EAB6 for ; Mon, 29 Oct 2018 17:32:56 -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 OfofcPA7hOtw for ; Mon, 29 Oct 2018 17:32:56 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 487062DDD3 for ; Mon, 29 Oct 2018 17:32:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool References: <9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.git.korablev@tarantool.org> <723E1B3C-AF45-4129-9C39-B5B2B04E3B28@tarantool.org> From: Vladislav Shpilevoy Message-ID: <2d06ce1c-bd65-221c-6946-bce549796925@tarantool.org> Date: Tue, 30 Oct 2018 00:32:54 +0300 MIME-Version: 1.0 In-Reply-To: <723E1B3C-AF45-4129-9C39-B5B2B04E3B28@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" 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 2 comments below. >>> + struct index_def *pk = pUseIndex; >>> assert(pk != NULL); >>> - uint32_t fieldno = pk->def->key_def->parts[0].fieldno; >>> + uint32_t fieldno = pk->key_def->parts[0].fieldno; >>> enum affinity_type affinity = >>> tab->def->fields[fieldno].affinity; >>> - if (pk->def->key_def->part_count == 1 && >>> + if (pk->key_def->part_count == 1 && >>> affinity == AFFINITY_INTEGER && (int)fieldno < nVector) { >>> int reg_pk = rLhs + (int)fieldno; >> >> 3. Why do we still need this MustBeInt here? As I understand, here we >> are trying to use an index for binary search of an LHS, but under the >> hood it is just tarantool iterators starting from certain keys, it is >> not? If it is, then tarantool iterators does key validation for us. Can >> you please check if this opcode is redundant? > > It doesn’t seem to be useless. Consider example: > > CREATE TABLE t1 (id INT PRIMARY KEY); > SELECT 1.23 IN t1; > > Now, OP_Affinity doesn’t cast REAL to INT - I described reason > in previous review. So, without OP_MustBeInt query above would > result in > > Execution failed: Supplied key type of part 0 does not match index part type: expected integer > > Since 1.23 is passed to iterator over integer field. But I guess query above > is fine - it should simply return 0, not an error since queries like > > SELECT * FROM t1 WHERE id = 1.123; > SELECT * FROM t1 WHERE id > 1.123; > INSERT INTO t1 VALUES (1.123); > > are allowed as well. 1. Then I am confused. Look at OP_MustBeInt. It calls applyAffinity(pIn1, AFFINITY_NUMERIC). NUMERIC is real, so 1.23 cast to NUMERIC should be unchanged, it is not? I tried this: tarantool> box.sql.execute("select CAST(1.23 as NUMERIC)") --- - - [1.23] ... So 1.23 is already numeric. But applyAffinity function for unknown reason does this: for target affinity == NUMERIC it sees that the original affinity is real and casts the result to integer. So applyAffinity(1.23, NUMERIC) == 1, but SELECT CAST 1.23 as NUMERIC == 1.23. ?! > >>> diff --git a/test/sql-tap/boundary3.test.lua b/test/sql-tap/boundary3.test.lua >>> index 5b63e0539..c5d605b65 100755 >>> --- a/test/sql-tap/boundary3.test.lua >>> +++ b/test/sql-tap/boundary3.test.lua >>> @@ -125,7 +125,7 @@ test:do_test( >>> -- >>> }) >>> -test:do_execsql_test( >>> +--[[test:do_execsql_test( >> >> 4. Do not silently comment tests. We should either delete it, or fix, or >> open an issue to fix it later. > > These tests are working. In previous review you asked to enable tests which > are under ‘if false’, so I forgot about simply commented tests. 2. Sorry, in3.test.lua still has a commented test. orderby5.test.lua too.