Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix quote() function

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Mergen,

Thanks for the fixes! The new patch below LGTM. I also tested the fix on
my M1 machine, and the issue is gone. JFYI, the whole sql-tap suite
works fine after your patch (with all LuaJIT-related patched applied).

On 18.08.21, Mergen Imeev wrote:
> Hi! Thank you for the review! My answers and new patch below. I did not include
> diff, since most of changes were in commit-message and there were only couple
> changes lines in the test.
> 
> On Wed, Aug 18, 2021 at 08:18:40PM +0300, Igor Munkin wrote:

<snipped>

> > > diff --git a/test/sql-tap/gh-6239-quote-with-double-arg.test.lua b/test/sql-tap/gh-6239-quote-with-double-arg.test.lua
> > > new file mode 100755
> > > index 000000000..60f85f20f
> > > --- /dev/null
> > > +++ b/test/sql-tap/gh-6239-quote-with-double-arg.test.lua
> > > @@ -0,0 +1,14 @@
> > > +#!/usr/bin/env tarantool
> > > +local test = require("sqltester")
> > > +test:plan(1)
> > > +
> > > +-- Make sure that QUOTE() returns DOUBLE in cast it receives DOUBLE.
> > > +test:do_execsql_test(
> > > +    "gh-6239",
> > > +    [[
> > > +        SELECT QUOTE(1.5);
> > > +    ]], {
> > > +        1.5
> > 
> > Do we need to check QUOTE preserves the type of the argument?
> > 
> Added check with typeof. I see no sense to add check with metadata, since there
> we will see "string". It will be fixed in patch-set about SQL built-in
> functions.

Great, thanks!

> 
> > > +    })
> > > +
> > > +test:finish_test()

<snipped>

> > > diff --git a/test/sql/types.result b/test/sql/types.result
> > > index 07d5b46e4..358667e94 100644
> > > --- a/test/sql/types.result
> > > +++ b/test/sql/types.result
> > > @@ -1908,10 +1908,10 @@ box.execute("SELECT quote(d) FROM t;")
> > >    - name: COLUMN_1
> > >      type: string
> > >    rows:
> > > -  - ['10.0']
> > > -  - ['-2.0']
> > > -  - ['3.3']
> > > -  - ['1.8e+19']
> > > +  - [10]
> > > +  - [-2]
> > > +  - [3.3]
> > 
> > Fraction is dropped by YAML serializer, right?
> > 
> Yes.

Got it, thanks!

> 
> > > +  - [18000000000000000000]
> > >  ...
> > >  box.execute("SELECT LEAST(d, 0) FROM t;")
> > >  ---
> > > -- 
> > > 2.25.1
> > > 
> > 
> > -- 
> > Best regards,
> > IM
> 
> 
> New patch:
> 
> 
> commit 2e2d979c53be8d9f97071d3003e1069fc8c078cf
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Fri Aug 13 05:50:06 2021 +0300
> 
>     sql: fix quote() function
>     
>     Ater this patch SQL built-in function QUOTE() will return argument in

Typo: s/Ater/After/.

>     case DOUBLE values is given. If the argument is not number, string

Typo: s/values/value/.

>     representation of the argument will be returned.
>     
>     Closes #6239
>     
>     @TarantoolBot document
>     Title: QUOTE() and DOUBLE argument
>     
>     After this patch function QUOTE() will return argument in case it
>     receives DOUBLE value as an argument. The same for all other numeric
>     types. In case it was given value of non-numeric type, it will return
>     string representation of the argument.
> 

<snipped>

>  ---

-- 
Best regards,
IM