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

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

[Mergen Imeev via Tarantool-patches]

Ater this patch SQL built-in function QUOTE() will return the same
DOUBLE in case it receives DOUBLE as an argument. If the argument is
not number, string representation of the argument will be returned.

Closes #6239
---
https://github.com/tarantool/tarantool/issues/6239
https://github.com/tarantool/tarantool/tree/imeevma/gh-6239-quote-with-double-arg

 src/box/sql/func.c                                | 15 +--------------
 test/sql-tap/engine.cfg                           |  3 +++
 .../gh-6239-quote-with-double-arg.test.lua        | 14 ++++++++++++++
 test/sql-tap/trigger5.test.lua                    |  3 ++-
 test/sql/types.result                             |  8 ++++----
 5 files changed, 24 insertions(+), 19 deletions(-)
 create mode 100755 test/sql-tap/gh-6239-quote-with-double-arg.test.lua

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 1622104d3..2aa637fbc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1096,26 +1096,13 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
 	switch (argv[0]->type) {
-	case MEM_TYPE_DOUBLE:{
-			double r1, r2;
-			char zBuf[50];
-			r1 = mem_get_double_unsafe(argv[0]);
-			sql_snprintf(sizeof(zBuf), zBuf, "%!.15g", r1);
-			sqlAtoF(zBuf, &r2, 20);
-			if (r1 != r2) {
-				sql_snprintf(sizeof(zBuf), zBuf, "%!.20e",
-						 r1);
-			}
-			sql_result_text(context, zBuf, -1,
-					    SQL_TRANSIENT);
-			break;
-		}
 	case MEM_TYPE_UUID: {
 		char buf[UUID_STR_LEN + 1];
 		tt_uuid_to_string(&argv[0]->u.uuid, &buf[0]);
 		sql_result_text(context, buf, UUID_STR_LEN, SQL_TRANSIENT);
 		break;
 	}
+	case MEM_TYPE_DOUBLE:
 	case MEM_TYPE_UINT:
 	case MEM_TYPE_INT: {
 			sql_result_value(context, argv[0]);
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 94b0bb1f6..3196c4054 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -23,6 +23,9 @@
     "gh-6164-uuid-follow-ups.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "gh-6239-quote-with-double-arg.test.lua": {
+        "memtx": {"engine": "memtx"}
+    },
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
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
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/trigger5.test.lua b/test/sql-tap/trigger5.test.lua
index 8336cdcf2..7f3762126 100755
--- a/test/sql-tap/trigger5.test.lua
+++ b/test/sql-tap/trigger5.test.lua
@@ -31,7 +31,8 @@ test:do_execsql_test(
           INSERT INTO Undo VALUES
              ((SELECT coalesce(max(id),0) + 1 FROM Undo),
               (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || CAST(coalesce(old.a,'NULL') AS TEXT)
-                  || ',' || quote(old.b) || ',' || CAST(old.c AS TEXT) || ');'));
+                  || ',' || CAST(quote(old.b) AS STRING) || ',' ||
+                  CAST(old.c AS TEXT) || ');'));
         END;
         DELETE FROM Item WHERE a = 1;
         SELECT * FROM Undo;
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]
+  - [18000000000000000000]
 ...
 box.execute("SELECT LEAST(d, 0) FROM t;")
 ---
-- 
2.25.1

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

[Igor Munkin via Tarantool-patches]

Mergen,

Thanks for the patch! Please consider minor comments below.

On 17.08.21, imeevma@tarantool.org wrote:
> Ater this patch SQL built-in function QUOTE() will return the same
> DOUBLE in case it receives DOUBLE as an argument. If the argument is

Minor: it's better to say QUOTE() returns the argument in case DOUBLE
value is given. Since we're here: do we need a docbot request for this?

> not number, string representation of the argument will be returned.
> 
> Closes #6239
> ---
> https://github.com/tarantool/tarantool/issues/6239
> https://github.com/tarantool/tarantool/tree/imeevma/gh-6239-quote-with-double-arg
> 
>  src/box/sql/func.c                                | 15 +--------------
>  test/sql-tap/engine.cfg                           |  3 +++
>  .../gh-6239-quote-with-double-arg.test.lua        | 14 ++++++++++++++
>  test/sql-tap/trigger5.test.lua                    |  3 ++-
>  test/sql/types.result                             |  8 ++++----
>  5 files changed, 24 insertions(+), 19 deletions(-)
>  create mode 100755 test/sql-tap/gh-6239-quote-with-double-arg.test.lua
> 

<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?

> +    })
> +
> +test:finish_test()
> diff --git a/test/sql-tap/trigger5.test.lua b/test/sql-tap/trigger5.test.lua
> index 8336cdcf2..7f3762126 100755
> --- a/test/sql-tap/trigger5.test.lua
> +++ b/test/sql-tap/trigger5.test.lua
> @@ -31,7 +31,8 @@ test:do_execsql_test(
>            INSERT INTO Undo VALUES
>               ((SELECT coalesce(max(id),0) + 1 FROM Undo),
>                (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || CAST(coalesce(old.a,'NULL') AS TEXT)
> -                  || ',' || quote(old.b) || ',' || CAST(old.c AS TEXT) || ');'));
> +                  || ',' || CAST(quote(old.b) AS STRING) || ',' ||
> +                  CAST(old.c AS TEXT) || ');'));

[Hm. Expecting nothing is completely broken, since test results are not
changed.]

>          END;
>          DELETE FROM Item WHERE a = 1;
>          SELECT * FROM Undo;
> 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?

> +  - [18000000000000000000]
>  ...
>  box.execute("SELECT LEAST(d, 0) FROM t;")
>  ---
> -- 
> 2.25.1
> 

-- 
Best regards,
IM

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

[Igor Munkin via Tarantool-patches]

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

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

[Mergen Imeev via Tarantool-patches]

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:
> Mergen,
> 
> Thanks for the patch! Please consider minor comments below.
> 
> On 17.08.21, imeevma@tarantool.org wrote:
> > Ater this patch SQL built-in function QUOTE() will return the same
> > DOUBLE in case it receives DOUBLE as an argument. If the argument is
> 
> Minor: it's better to say QUOTE() returns the argument in case DOUBLE
> value is given. Since we're here: do we need a docbot request for this?
> 
Fixed. Added a docbot request.

> > not number, string representation of the argument will be returned.
> > 
> > Closes #6239
> > ---
> > https://github.com/tarantool/tarantool/issues/6239
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-6239-quote-with-double-arg
> > 
> >  src/box/sql/func.c                                | 15 +--------------
> >  test/sql-tap/engine.cfg                           |  3 +++
> >  .../gh-6239-quote-with-double-arg.test.lua        | 14 ++++++++++++++
> >  test/sql-tap/trigger5.test.lua                    |  3 ++-
> >  test/sql/types.result                             |  8 ++++----
> >  5 files changed, 24 insertions(+), 19 deletions(-)
> >  create mode 100755 test/sql-tap/gh-6239-quote-with-double-arg.test.lua
> > 
> 
> <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.

> > +    })
> > +
> > +test:finish_test()
> > diff --git a/test/sql-tap/trigger5.test.lua b/test/sql-tap/trigger5.test.lua
> > index 8336cdcf2..7f3762126 100755
> > --- a/test/sql-tap/trigger5.test.lua
> > +++ b/test/sql-tap/trigger5.test.lua
> > @@ -31,7 +31,8 @@ test:do_execsql_test(
> >            INSERT INTO Undo VALUES
> >               ((SELECT coalesce(max(id),0) + 1 FROM Undo),
> >                (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || CAST(coalesce(old.a,'NULL') AS TEXT)
> > -                  || ',' || quote(old.b) || ',' || CAST(old.c AS TEXT) || ');'));
> > +                  || ',' || CAST(quote(old.b) AS STRING) || ',' ||
> > +                  CAST(old.c AS TEXT) || ');'));
> 
> [Hm. Expecting nothing is completely broken, since test results are not
> changed.]
> 
> >          END;
> >          DELETE FROM Item WHERE a = 1;
> >          SELECT * FROM Undo;
> > 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.

> > +  - [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
    case DOUBLE values is given. If the argument is not number, string
    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.

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 1551d3ef2..3da608c0b 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1091,26 +1091,13 @@ quoteFunc(sql_context * context, int argc, sql_value ** argv)
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
 	switch (argv[0]->type) {
-	case MEM_TYPE_DOUBLE:{
-			double r1, r2;
-			char zBuf[50];
-			r1 = mem_get_double_unsafe(argv[0]);
-			sql_snprintf(sizeof(zBuf), zBuf, "%!.15g", r1);
-			sqlAtoF(zBuf, &r2, 20);
-			if (r1 != r2) {
-				sql_snprintf(sizeof(zBuf), zBuf, "%!.20e",
-						 r1);
-			}
-			sql_result_text(context, zBuf, -1,
-					    SQL_TRANSIENT);
-			break;
-		}
 	case MEM_TYPE_UUID: {
 		char buf[UUID_STR_LEN + 1];
 		tt_uuid_to_string(&argv[0]->u.uuid, &buf[0]);
 		sql_result_text(context, buf, UUID_STR_LEN, SQL_TRANSIENT);
 		break;
 	}
+	case MEM_TYPE_DOUBLE:
 	case MEM_TYPE_UINT:
 	case MEM_TYPE_INT: {
 			sql_result_value(context, argv[0]);
diff --git a/test/sql-tap/engine.cfg b/test/sql-tap/engine.cfg
index 820c72b00..5799780dc 100644
--- a/test/sql-tap/engine.cfg
+++ b/test/sql-tap/engine.cfg
@@ -26,6 +26,9 @@
     "metatypes.test.lua": {
         "memtx": {"engine": "memtx"}
     },
+    "gh-6239-quote-with-double-arg.test.lua": {
+        "memtx": {"engine": "memtx"}
+    },
     "gh-4077-iproto-execute-no-bind.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
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..dab12253e
--- /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), TYPEOF(QUOTE(1.5));
+    ]], {
+        1.5, "double"
+    })
+
+test:finish_test()
diff --git a/test/sql-tap/trigger5.test.lua b/test/sql-tap/trigger5.test.lua
index 8336cdcf2..7f3762126 100755
--- a/test/sql-tap/trigger5.test.lua
+++ b/test/sql-tap/trigger5.test.lua
@@ -31,7 +31,8 @@ test:do_execsql_test(
           INSERT INTO Undo VALUES
              ((SELECT coalesce(max(id),0) + 1 FROM Undo),
               (SELECT 'INSERT INTO Item (a,b,c) VALUES (' || CAST(coalesce(old.a,'NULL') AS TEXT)
-                  || ',' || quote(old.b) || ',' || CAST(old.c AS TEXT) || ');'));
+                  || ',' || CAST(quote(old.b) AS STRING) || ',' ||
+                  CAST(old.c AS TEXT) || ');'));
         END;
         DELETE FROM Item WHERE a = 1;
         SELECT * FROM Undo;
diff --git a/test/sql/types.result b/test/sql/types.result
index 68bdcd62e..fa78e8d9a 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1909,10 +1909,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]
+  - [18000000000000000000]
 ...
 box.execute("SELECT LEAST(d, 0) FROM t;")
 ---