From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id F301A469711
 for <tarantool-patches@dev.tarantool.org>;
 Mon,  1 Jun 2020 20:04:08 +0300 (MSK)
References: <cover.1590671266.git.imeevma@gmail.com>
 <40caa8fdc66d3d20aca72677e10014e749172f92.1590671266.git.imeevma@gmail.com>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <1fdaea95-07d2-72ac-82bc-24e0c8fa4ed0@tarantool.org>
Date: Mon, 1 Jun 2020 19:04:07 +0200
MIME-Version: 1.0
In-Reply-To: <40caa8fdc66d3d20aca72677e10014e749172f92.1590671266.git.imeevma@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 4/6] sql: remove mem_apply_type()
	from OP_MustBeInt
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Mergen Imeev <imeevma@tarantool.org>, tsafin@tarantool.org, tarantool-patches@dev.tarantool.org

Thanks for the patch!

See 3 comments below.

On 28/05/2020 16:17, Mergen Imeev wrote:
> This patch replaces mem_apply_type() by mem_check_types() in
> OP_MustBeInt, which allows to remove implicit case in some places,

1. 'case' -> 'cast'.

> for example in IN operator.
> 
> Part of #4230
> ---
> diff --git a/test/sql-tap/tkt-fc7bd6358f.test.lua b/test/sql-tap/tkt-fc7bd6358f.test.lua
> index fe5d6200f..f38ffa3d6 100755
> --- a/test/sql-tap/tkt-fc7bd6358f.test.lua
> +++ b/test/sql-tap/tkt-fc7bd6358f.test.lua
> @@ -80,7 +80,6 @@ for a, from in ipairs(froms) do
>              function()
>                  return test:execsql(string.format("SELECT t1.textid, i.intid, t2.textid %s %s", from, where))
>              end, {
> -                "12", 12, "12", "34", 34, "34"
>              })
>  
>          test:do_test(
> @@ -88,7 +87,6 @@ for a, from in ipairs(froms) do
>              function()
>                  return test:execsql(string.format("SELECT t1.textid, i.intid, t2.textid %s %s", from, where))
>              end, {
> -                "12", 12, "12", "34", 34, "34"
>              })

2. In the header of this file it is said, that the whole test's
purpose is to ensure, that these values are returned. Do we
need this test file now at all? Or can it be fixed in a way, that
the results are not changed?

>  
>      end
> diff --git a/test/sql-tap/whereB.test.lua b/test/sql-tap/whereB.test.lua
> index fe5e28c70..970ff1dec 100755
> --- a/test/sql-tap/whereB.test.lua
> +++ b/test/sql-tap/whereB.test.lua
> @@ -432,7 +432,6 @@ test:do_execsql_test(
>      ]],
>      {
>      -- <whereB-5.2>
> -    1, 2, true
>      -- </whereB-5.2>
>      })
>  
> @@ -443,7 +442,6 @@ test:do_execsql_test(
>      ]],
>      {
>      -- <whereB-5.3>
> -    1, 2, true

3. These tests also look useless now. Their comment
says:

	-- Because t2.b has a numeric affinity, type conversion should occur
	-- and the two fields should be equal.

And now they are not equal. Do we need these tests?