Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: decrease SELECT_COMPOUND_LIMIT threshold
Date: Thu, 20 Sep 2018 11:49:01 +0300	[thread overview]
Message-ID: <3E57AE93-5749-44DB-9146-034CBCD18896@tarantool.org> (raw)
In-Reply-To: <eaf45667-f956-d4ed-c368-3f11dec82be2@tarantool.org>


>> Hard limit 500 seems to be too much. Lets set hard limit 50 (as it was before).
>> If user want to set more than 50, he can set it to 0 (according to comments
>> it means no limit at all).
> Ok, set 50.
> 
>> Ok, you may skip it, but anyway, I would add tests where you set
>> limit to 0 and process compound select with 31 parts.
> Introduced such test.
> 
>> You forgot to fix commit message. See previous review comments.
> Updated.
> 
>> This pragma status is not displayed via simple ‘pragma’.
> It is ok, the other non-boolean pragmas have same behavior. 
> Displaying sql_default_engine was a big mistake. I've just implemented requirement
> from review comment that time, but it was not correct. It looks out-of-order,
> as an exception in code there.

I don’t think so. It looks OK and it (default engine) seems to
be very useful for user. However, I don’t insist on this change,
so let it be as is.

> @@ -193,7 +193,7 @@ test:do_execsql_test(
>         INSERT INTO t3 VALUES(56.0);
>     ]], {
>         -- <select7-7.1>
> -        
> +
>         -- </select7-7.1>
>     })
> 
> @@ -216,7 +216,7 @@ test:do_execsql_test(
>         INSERT INTO t4 VALUES( 3.0 );
>     ]], {
>         -- <select7-7.3>
> -        
> +
>         -- </select7-7.3>
>     })
> 
> @@ -233,7 +233,7 @@ test:do_execsql_test(
> test:do_execsql_test(
>     "select7-7.5",
>     [[
> -        SELECT a=0, typeof(a) FROM t4 
> +        SELECT a=0, typeof(a) FROM t4
>     ]], {
>         -- <select7-7.5>
>         0, "real", 0, "real"
> @@ -243,7 +243,7 @@ test:do_execsql_test(
> test:do_execsql_test(
>     "select7-7.6",
>     [[
> -        SELECT a=0, typeof(a) FROM t4 GROUP BY a 
> +        SELECT a=0, typeof(a) FROM t4 GROUP BY a
>     ]], {

Garbage diff.

Otherwise, patch LGTM.

  reply	other threads:[~2018-09-20  8:49 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11 11:47 [tarantool-patches] " Kirill Shcherbatov
2018-09-11 21:33 ` [tarantool-patches] " n.pettik
2018-09-13  8:42   ` Kirill Shcherbatov
2018-09-19 21:29     ` n.pettik
2018-09-20  8:32       ` Kirill Shcherbatov
2018-09-20  8:49         ` n.pettik [this message]
2018-09-20 15:58 ` Kirill Yukhin

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=3E57AE93-5749-44DB-9146-034CBCD18896@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: decrease SELECT_COMPOUND_LIMIT threshold' \
    /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