Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Mergen Imeev <imeevma@tarantool.org>,
	tsafin@tarantool.org, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 4/6] sql: remove mem_apply_type() from OP_MustBeInt
Date: Mon, 1 Jun 2020 19:04:07 +0200	[thread overview]
Message-ID: <1fdaea95-07d2-72ac-82bc-24e0c8fa4ed0@tarantool.org> (raw)
In-Reply-To: <40caa8fdc66d3d20aca72677e10014e749172f92.1590671266.git.imeevma@gmail.com>

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?

  reply	other threads:[~2020-06-01 17:04 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28 14:17 [Tarantool-patches] [PATCH 0/6] Remove implicit cast Mergen Imeev
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 1/6] sql: remove implicit cast for assignment Mergen Imeev
2020-06-01 17:03   ` Vladislav Shpilevoy
2020-06-09 11:41     ` Mergen Imeev
2020-06-09 22:28       ` Vladislav Shpilevoy
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 2/6] sql: remove mem_apply_type() from OP_MakeRecord Mergen Imeev
2020-06-01 17:03   ` Vladislav Shpilevoy
2020-06-09 11:43     ` Mergen Imeev
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 3/6] sql: replace ApplyType by CheckType for IN operator Mergen Imeev
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 4/6] sql: remove mem_apply_type() from OP_MustBeInt Mergen Imeev
2020-06-01 17:04   ` Vladislav Shpilevoy [this message]
2020-06-09 11:47     ` Mergen Imeev
2020-06-09 22:28       ` Vladislav Shpilevoy
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison Mergen Imeev
2020-06-01 17:04   ` Vladislav Shpilevoy
2020-06-09 11:51     ` Mergen Imeev
2020-06-09 22:29       ` Vladislav Shpilevoy
2020-05-28 14:17 ` [Tarantool-patches] [PATCH 6/6] sql: remove OP_ApplyType Mergen Imeev
2020-06-09 22:29   ` Vladislav Shpilevoy
2020-06-01 17:03 ` [Tarantool-patches] [PATCH 0/6] Remove implicit cast Vladislav Shpilevoy
2020-06-09 11:25   ` Mergen Imeev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1fdaea95-07d2-72ac-82bc-24e0c8fa4ed0@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 4/6] sql: remove mem_apply_type() from OP_MustBeInt' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox