Re: [Tarantool-patches] [PATCH 1/1] sql: add another way to change SQL default engine

[Alexander Turenko <alexander.turenko@tarantool.org>]

I don't mind in general, but please consider several notes below.

WBR, Alexander Turenko.

On Fri, Dec 27, 2019 at 03:33:12PM +0300, imeevma@tarantool.org wrote:
> sql: add another way to change SQL default engine
>
> Due to the removal of the sql_default_engine pragma in 2.3.1, this
> patch adds another way to change this setting.

Nit: I would say 'implements' rather then 'adds', because the letter can
be read as something configurable by a user.

I would mention that it affects only *.test.sql tests, just in case.

I think it would be good to link the issue about it here.

> ---
> https://github.com/tarantool/test-run/tree/imeevma/fix_sql_default_engine
> 
>  lib/tarantool_server.py | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
> index 6f71b5b..20a398e 100644
> --- a/lib/tarantool_server.py
> +++ b/lib/tarantool_server.py
> @@ -145,12 +145,19 @@ class LuaTest(Test):
>              return True
>  
>          engine = self.run_params['engine']
> -        command = "pragma sql_default_engine='{}'".format(engine)
> -        result = self.send_command(command, ts, 'sql')
> -        result = result.replace('\r\n', '\n')
> -        if result != '---\n- row_count: 0\n...\n':
> -            sys.stdout.write(result)
> -            return False
> +
> +        command_new = ("UPDATE \"_session_settings\" SET \"value\" = '{}' " + \
> +                       "WHERE \"name\" = 'sql_default_engine'").format(engine)
> +        result_new = self.send_command(command_new, ts, 'sql')
> +        result_new = result_new.replace('\r\n', '\n')
> +
> +        command_old = "pragma sql_default_engine='{}'".format(engine)
> +        result_old = self.send_command(command_old, ts, 'sql')
> +        result_old = result_old.replace('\r\n', '\n')
> +        if result_new != '---\n- row_count: 1\n...\n':
> +            if result_old != '---\n- row_count: 0\n...\n':
> +                sys.stdout.write(result_old)
> +                return False

I would make it like so:

1. Send "pragma ...", await a result.
   - If succeeds, return True.
   - Otherwise proceed with the step 2.
2. Send "UPDATE ...", await a result.
   - If succeeds, return True.
   - Otherwise return False.

This way we'll save one extra hop on a fresh tarantool. Not so big
overhead for an action that is run once for a test, but we can overcome
it and pay nothing, so it looks worthful for me.

Please, look into CI: it reports a flake8 error. You can use `make lint`
locally to verify python code style.

Please, double-check that everything works as expected for both
tarantool versions (with pragma support and with _session_settings):
let's run a .test.sql test and verify that it creates spaces in an
expected engine. It should be easier using `\set language lua` command:
it is simple to assess a space engine and compare it with
test_run:get_cfg('engine').

README states that test-run use "pragma <...>" to set an engine. It
would be good to mention the new way together with the old one.

>  
>          return True
>  
> -- 
> 2.7.4

BTW, the subject of the email is the following:

 | [PATCH 1/1] sql: add another way to change SQL default engine

I suggest to add a repo prefix when it is not tarantool itself. Look as
Vlad does it:

 | [PATCH vshard 1/1] travis-ci: deploy packages from tagged revisions

https://lists.tarantool.org/pipermail/tarantool-patches/2019-August/001304.html